Date: Mon, 16 Aug 2010 05:15:21 +0200 From: Max Laier <max@love2party.net> To: freebsd-arch@freebsd.org Cc: ups@freebsd.org Subject: rmlock(9) two additions Message-ID: <201008160515.21412.max@love2party.net>
next in thread | raw e-mail | index | archive | help
--Boundary-00=_J1KaMsyD3+FF4Et
Content-Type: Text/Plain;
charset="us-ascii"
Content-Transfer-Encoding: 7bit
Hi,
I'd like to run two additions to rmlock(9) by you:
1) See the attached patch. It adds rm_try_rlock() which avoids taking the
mutex if the lock is currently in a write episode. The only overhead to the
hot path is an additional argument and a return value for _rm_rlock*. If you
are worried about that, it can obviously be done in a separate code path, but
I reckon it not worth the code crunch. Finally, there is one additional
branch to check the "trylock" argument, but that's well past the hot path.
2) No code for this yet - testing the waters first. I'd like to add the
ability to replace the mutex for writer synchronization with a general lock -
especially to be able to use a sx(9) lock here.
The reason for #2 is the following use case in a debugging facility:
"reader":
if (rm_try_rlock()) {
grab_per_cpu_buffer();
fill_per_cpu_buffer();
rm_runlock();
}
"writer" - better exclusive access thread:
rm_wlock();
collect_buffers_and_copy_out_to_userspace();
rm_wunlock();
This is much cleaner and possibly cheaper than the various hand rolled
versions I've come across, that try to get the same synchronization with
atomic operations. If we could sleep with the wlock held, we can also avoid
copying the buffer contents, or swapping buffers.
Is there any concern about either of this? Any objection? Input?
Thanks,
Max Laier
--Boundary-00=_J1KaMsyD3+FF4Et
Content-Type: text/x-patch; charset="ISO-8859-1"; name="rm_try_rlock.diff"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename="rm_try_rlock.diff"
diff --git a/sys/kern/kern_rmlock.c b/sys/kern/kern_rmlock.c
index a6a622e..9b91fb1 100644
--- a/sys/kern/kern_rmlock.c
+++ b/sys/kern/kern_rmlock.c
@@ -241,8 +241,8 @@ rm_sysinit_flags(void *arg)
rm_init_flags(args->ra_rm, args->ra_desc, args->ra_opts);
}
-static void
-_rm_rlock_hard(struct rmlock *rm, struct rm_priotracker *tracker)
+static int
+_rm_rlock_hard(struct rmlock *rm, struct rm_priotracker *tracker, int trylock)
{
struct pcpu *pc;
struct rm_queue *queue;
@@ -254,7 +254,7 @@ _rm_rlock_hard(struct rmlock *rm, struct rm_priotracker *tracker)
/* Check if we just need to do a proper critical_exit. */
if (0 == rm->rm_noreadtoken) {
critical_exit();
- return;
+ return (1);
}
/* Remove our tracker from the per-cpu list. */
@@ -265,7 +265,7 @@ _rm_rlock_hard(struct rmlock *rm, struct rm_priotracker *tracker)
/* Just add back tracker - we hold the lock. */
rm_tracker_add(pc, tracker);
critical_exit();
- return;
+ return (1);
}
/*
@@ -289,7 +289,7 @@ _rm_rlock_hard(struct rmlock *rm, struct rm_priotracker *tracker)
mtx_unlock_spin(&rm_spinlock);
rm_tracker_add(pc, tracker);
critical_exit();
- return;
+ return (1);
}
}
}
@@ -297,6 +297,9 @@ _rm_rlock_hard(struct rmlock *rm, struct rm_priotracker *tracker)
sched_unpin();
critical_exit();
+ if (trylock)
+ return (0);
+
mtx_lock(&rm->rm_lock);
rm->rm_noreadtoken = 0;
critical_enter();
@@ -307,10 +310,12 @@ _rm_rlock_hard(struct rmlock *rm, struct rm_priotracker *tracker)
critical_exit();
mtx_unlock(&rm->rm_lock);
+
+ return (1);
}
-void
-_rm_rlock(struct rmlock *rm, struct rm_priotracker *tracker)
+int
+_rm_rlock(struct rmlock *rm, struct rm_priotracker *tracker, int trylock)
{
struct thread *td = curthread;
struct pcpu *pc;
@@ -338,10 +343,10 @@ _rm_rlock(struct rmlock *rm, struct rm_priotracker *tracker)
* conditional jump.
*/
if (0 == (td->td_owepreempt | rm->rm_noreadtoken))
- return;
+ return (1);
/* We do not have a read token and need to acquire one. */
- _rm_rlock_hard(rm, tracker);
+ return _rm_rlock_hard(rm, tracker, trylock);
}
static void
@@ -470,20 +475,23 @@ _rm_wunlock_debug(struct rmlock *rm, const char *file, int line)
_rm_wunlock(rm);
}
-void
+int
_rm_rlock_debug(struct rmlock *rm, struct rm_priotracker *tracker,
- const char *file, int line)
+ int trylock, const char *file, int line)
{
-
WITNESS_CHECKORDER(&rm->lock_object, LOP_NEWORDER, file, line, NULL);
- _rm_rlock(rm, tracker);
+ if (_rm_rlock(rm, tracker, trylock)) {
+ LOCK_LOG_LOCK("RMRLOCK", &rm->lock_object, 0, 0, file, line);
- LOCK_LOG_LOCK("RMRLOCK", &rm->lock_object, 0, 0, file, line);
+ WITNESS_LOCK(&rm->lock_object, 0, file, line);
- WITNESS_LOCK(&rm->lock_object, 0, file, line);
+ curthread->td_locks++;
- curthread->td_locks++;
+ return (1);
+ }
+
+ return (0);
}
void
@@ -517,12 +525,12 @@ _rm_wunlock_debug(struct rmlock *rm, const char *file, int line)
_rm_wunlock(rm);
}
-void
+int
_rm_rlock_debug(struct rmlock *rm, struct rm_priotracker *tracker,
- const char *file, int line)
+ int trylock, const char *file, int line)
{
- _rm_rlock(rm, tracker);
+ return _rm_rlock(rm, tracker, trylock);
}
void
diff --git a/sys/sys/rmlock.h b/sys/sys/rmlock.h
index 9766f67..5261bcf 100644
--- a/sys/sys/rmlock.h
+++ b/sys/sys/rmlock.h
@@ -53,14 +53,15 @@ void rm_sysinit_flags(void *arg);
void _rm_wlock_debug(struct rmlock *rm, const char *file, int line);
void _rm_wunlock_debug(struct rmlock *rm, const char *file, int line);
-void _rm_rlock_debug(struct rmlock *rm, struct rm_priotracker *tracker,
- const char *file, int line);
+int _rm_rlock_debug(struct rmlock *rm, struct rm_priotracker *tracker,
+ int trylock, const char *file, int line);
void _rm_runlock_debug(struct rmlock *rm, struct rm_priotracker *tracker,
const char *file, int line);
void _rm_wlock(struct rmlock *rm);
void _rm_wunlock(struct rmlock *rm);
-void _rm_rlock(struct rmlock *rm, struct rm_priotracker *tracker);
+int _rm_rlock(struct rmlock *rm, struct rm_priotracker *tracker,
+ int trylock);
void _rm_runlock(struct rmlock *rm, struct rm_priotracker *tracker);
/*
@@ -74,14 +75,17 @@ void _rm_runlock(struct rmlock *rm, struct rm_priotracker *tracker);
#define rm_wlock(rm) _rm_wlock_debug((rm), LOCK_FILE, LOCK_LINE)
#define rm_wunlock(rm) _rm_wunlock_debug((rm), LOCK_FILE, LOCK_LINE)
#define rm_rlock(rm,tracker) \
- _rm_rlock_debug((rm),(tracker), LOCK_FILE, LOCK_LINE )
+ ((void)_rm_rlock_debug((rm),(tracker), 0, LOCK_FILE, LOCK_LINE ))
+#define rm_try_rlock(rm,tracker) \
+ _rm_rlock_debug((rm),(tracker), 1, LOCK_FILE, LOCK_LINE )
#define rm_runlock(rm,tracker) \
_rm_runlock_debug((rm), (tracker), LOCK_FILE, LOCK_LINE )
#else
-#define rm_wlock(rm) _rm_wlock((rm))
-#define rm_wunlock(rm) _rm_wunlock((rm))
-#define rm_rlock(rm,tracker) _rm_rlock((rm),(tracker))
-#define rm_runlock(rm,tracker) _rm_runlock((rm), (tracker))
+#define rm_wlock(rm) _rm_wlock((rm))
+#define rm_wunlock(rm) _rm_wunlock((rm))
+#define rm_rlock(rm,tracker) ((void)_rm_rlock((rm),(tracker), 0))
+#define rm_try_rlock(rm,tracker) _rm_rlock((rm),(tracker), 1)
+#define rm_runlock(rm,tracker) _rm_runlock((rm), (tracker))
#endif
struct rm_args {
--Boundary-00=_J1KaMsyD3+FF4Et--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201008160515.21412.max>