Re: Module and device configuration locking [was Re: Modules loading modules?]
On Thu, 5 Aug 2010, Paul Goyette wrote:
On Thu, 5 Aug 2010, Paul Goyette wrote:
OK, got it.
I'll have another set of patches in a few days.
Well, it was a slow day at the office today, so I found some time to work on
this! :)
Attached is the latest version of this change. For simplicity, I have broken
the patches up into five separate groups:
Part 1 defines a set of new kernel locking primitives in file
kern/kern_lock.c to implement the recursive kernconfig_mutex. (I'm not
really stuck on the name, so open to alternative suggestions!) All of the
locking in sys/kern_module.c has been updated to use this instead of the
internal module_lock mutex. Additionally, the module_autoload() routine now
does its own locking, rather than requiring its caller to do it. And the
description of locking in the module(9) man page is updated.
Part 2 removes all of the explicit module_{,un}lock() calls from the various
xxx_verbose modules that I'd previous worked on.
Part 3 removes all of the explicit module_{,un}lock() calls from other bits
and pieces of the kernel. In a couple of places, new calls to
kernconfig_{,un}lock() are inserted.
Part 4 updates rump to provide equivalent locking routines. (pooka, I would
appreciate you looking at this.)
Part 5 adds a new atf test-case to the existing module tests to verify that
recursion actually works!
Following up on all the various comments from jmcneil@, pooka@, and
rmind@, I've attached a revised set of diffs. The most significant
change between this and the previous revision is the separation of the
kernconfig_lock_*() stuff into a separate kern_cfg.c source file. This
allows inclusion of the kernel code directly into rump, rather than
having to re-implement it in rump/klock.c
-
| Paul Goyette | PGP Key fingerprint: | E-mail addresses: |
| Customer Service | FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com|
| Network Engineer | 0786 F758 55DE 53BA 7731 | pgoyette at juniper.net |
| Kernel Developer | | pgoyette at netbsd.org |
-
ML.diff.tgz
Description: Binary data
Re: Module and device configuration locking [was Re: Modules loading modules?]
On Fri, 6 Aug 2010, Mindaugas Rasiukevicius wrote:
Hello,
Paul Goyette wrote:
Well, it was a slow day at the office today, so I found some time to
work on this! :)
Attached is the latest version of this change. For simplicity, I have
broken the patches up into five separate groups:
<...>
Few comments on the patch:
-#define__NetBSD_Version__ 599003800 /* NetBSD 5.99.38 */
+#define__NetBSD_Version__ 599003900 /* NetBSD 5.99.39 */
Why bumping?
Seemed like the right thing to do, since the locking protocol is
changing, as well as the global module_lock mutex disappearing. But I
really can't hink of any ill effects, unless someone has a module that
suddenly fails link-and-load because it references module_lock. It's
not very likely.
I'll defer, or I'll wait to ride someone else's bump just to be safe.
+ * of of kernel functionality, such as device configuration and loading
One "of" too much.
Fixed.
+void
+kernconfig_lock(void)
+{
+ lwp_t *l;
+
+ /*
+* OK to check this unlocked, since it could only be set to
+* curlwp by the current thread itself, and not by an interrupt
+* or any other LWP.
+*/
Please add KASSERT(!cpu_intr_p());
Done. I was worried about this, and forgot to ask. Thanks for picking
it up.
+ l = curlwp;
+ if (kernconfig_lwp == l) {
+ kernconfig_recurse++;
+ KASSERT(kernconfig_recurse != 0);
Such (++ then != 0) assert is a bit useless. How about:
KASSERT(kernconfig_recurse > 1);
The intent here was to ensure that we didn't overflow the counter back
to zero. It might have made sense when the counter was a uint8_t, but
probably not with an int! It can probably be removed completely.
+ } else {
+ mutex_enter(&kernconfig_mutex);
+ kernconfig_lwp = l;
+ kernconfig_recurse = 1;
+ }
+}
Somebody will confuse l with 1. :)
This came straight from ad@'s suggestions, and there seems to be a lot
of precedent in the sys/kern/ code to use l. But I will select a better
variable name.
+ if (--kernconfig_recurse == 0) {
+ kernconfig_lwp = 0;
+ mutex_exit(&kernconfig_mutex);
kernconfig_lwp = NULL;
Ooops! :)
+bool
+kernconfig_is_held(void)
+{
+
+ return (mutex_owned(&kernconfig_mutex));
No need for brackets around function.
Got it.
+ KASSERT(kernconfig_is_held);
But missing brackets in few cases.
Yeah, you're the second one to point that out. I fixed them once, not
sure how they crept back in. But they've been fixed again.
Otherwise seems OK to me. Thanks.
Thank you for the detailed look.
-
| Paul Goyette | PGP Key fingerprint: | E-mail addresses: |
| Customer Service | FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com|
| Network Engineer | 0786 F758 55DE 53BA 7731 | pgoyette at juniper.net |
| Kernel Developer | | pgoyette at netbsd.org |
-
Re: Module and device configuration locking [was Re: Modules loading modules?]
Hello,
Paul Goyette wrote:
>
> Well, it was a slow day at the office today, so I found some time to
> work on this! :)
>
> Attached is the latest version of this change. For simplicity, I have
> broken the patches up into five separate groups:
>
> <...>
Few comments on the patch:
> -#define __NetBSD_Version__ 599003800 /* NetBSD 5.99.38 */
> +#define __NetBSD_Version__ 599003900 /* NetBSD 5.99.39 */
Why bumping?
> + * of of kernel functionality, such as device configuration and loading
One "of" too much.
> +void
> +kernconfig_lock(void)
> +{
> + lwp_t *l;
> +
> + /*
> + * OK to check this unlocked, since it could only be set to
> + * curlwp by the current thread itself, and not by an interrupt
> + * or any other LWP.
> + */
Please add KASSERT(!cpu_intr_p());
> + l = curlwp;
> + if (kernconfig_lwp == l) {
> + kernconfig_recurse++;
> + KASSERT(kernconfig_recurse != 0);
Such (++ then != 0) assert is a bit useless. How about:
KASSERT(kernconfig_recurse > 1);
> + } else {
> + mutex_enter(&kernconfig_mutex);
> + kernconfig_lwp = l;
> + kernconfig_recurse = 1;
> + }
> +}
Somebody will confuse l with 1. :)
> + if (--kernconfig_recurse == 0) {
> + kernconfig_lwp = 0;
> + mutex_exit(&kernconfig_mutex);
kernconfig_lwp = NULL;
> +bool
> +kernconfig_is_held(void)
> +{
> +
> + return (mutex_owned(&kernconfig_mutex));
No need for brackets around function.
> + KASSERT(kernconfig_is_held);
But missing brackets in few cases.
Otherwise seems OK to me. Thanks.
--
Mindaugas
Re: Module and device configuration locking [was Re: Modules loading modules?]
On Thu, 5 Aug 2010, Paul Goyette wrote:
OK, got it.
I'll have another set of patches in a few days.
Well, it was a slow day at the office today, so I found some time to
work on this! :)
Attached is the latest version of this change. For simplicity, I have
broken the patches up into five separate groups:
Part 1 defines a set of new kernel locking primitives in file
kern/kern_lock.c to implement the recursive kernconfig_mutex. (I'm not
really stuck on the name, so open to alternative suggestions!) All of
the locking in sys/kern_module.c has been updated to use this instead of
the internal module_lock mutex. Additionally, the module_autoload()
routine now does its own locking, rather than requiring its caller to do
it. And the description of locking in the module(9) man page is
updated.
Part 2 removes all of the explicit module_{,un}lock() calls from the
various xxx_verbose modules that I'd previous worked on.
Part 3 removes all of the explicit module_{,un}lock() calls from other
bits and pieces of the kernel. In a couple of places, new calls to
kernconfig_{,un}lock() are inserted.
Part 4 updates rump to provide equivalent locking routines. (pooka, I
would appreciate you looking at this.)
Part 5 adds a new atf test-case to the existing module tests to verify
that recursion actually works!
-
| Paul Goyette | PGP Key fingerprint: | E-mail addresses: |
| Customer Service | FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com|
| Network Engineer | 0786 F758 55DE 53BA 7731 | pgoyette at juniper.net |
| Kernel Developer | | pgoyette at netbsd.org |
-Index: sys/sys/param.h
===
RCS file: /cvsroot/src/sys/sys/param.h,v
retrieving revision 1.373
diff -u -p -r1.373 param.h
--- sys/sys/param.h 28 Jul 2010 11:03:47 - 1.373
+++ sys/sys/param.h 6 Aug 2010 00:46:05 -
@@ -63,7 +63,7 @@
* 2.99.9 (299000900)
*/
-#define__NetBSD_Version__ 599003800 /* NetBSD 5.99.38 */
+#define__NetBSD_Version__ 599003900 /* NetBSD 5.99.39 */
#define __NetBSD_Prereq__(M,m,p) (M) * 1) + \
(m) * 100) + (p) * 100) <= __NetBSD_Version__)
Index: sys/sys/systm.h
===
RCS file: /cvsroot/src/sys/sys/systm.h,v
retrieving revision 1.239
diff -u -p -r1.239 systm.h
--- sys/sys/systm.h 31 Jan 2010 02:04:43 - 1.239
+++ sys/sys/systm.h 6 Aug 2010 00:46:06 -
@@ -492,6 +492,11 @@ void kernel_lock_init(void);
void _kernel_lock(int);
void _kernel_unlock(int, int *);
+void kernconfig_lock_init(void);
+void kernconfig_lock(void);
+void kernconfig_unlock(void);
+bool kernconfig_is_held(void);
+
#if defined(MULTIPROCESSOR) || defined(_MODULE)
#defineKERNEL_LOCK(count, lwp) \
do { \
Index: sys/sys/module.h
===
RCS file: /cvsroot/src/sys/sys/module.h,v
retrieving revision 1.24
diff -u -p -r1.24 module.h
--- sys/sys/module.h26 Jun 2010 07:23:57 - 1.24
+++ sys/sys/module.h6 Aug 2010 00:46:07 -
@@ -115,7 +115,6 @@ __link_set_add_rodata(modules, name##_mo
TAILQ_HEAD(modlist, module);
extern struct vm_map *module_map;
-extern kmutex_tmodule_lock;
extern u_int module_count;
extern u_int module_builtinlist;
extern struct modlist module_list;
Index: sys/kern/kern_lock.c
===
RCS file: /cvsroot/src/sys/kern/kern_lock.c,v
retrieving revision 1.150
diff -u -p -r1.150 kern_lock.c
--- sys/kern/kern_lock.c20 Dec 2009 20:42:23 - 1.150
+++ sys/kern/kern_lock.c6 Aug 2010 00:46:07 -
@@ -39,6 +39,7 @@ __KERNEL_RCSID(0, "$NetBSD: kern_lock.c,
#include
#include
#include
+#include
#include
#include
#include
@@ -56,6 +57,10 @@ bool kernel_lock_dodebug;
__cpu_simple_lock_t kernel_lock[CACHE_LINE_SIZE / sizeof(__cpu_simple_lock_t)]
__aligned(CACHE_LINE_SIZE);
+static kmutex_t kernconfig_mutex;
+static lwp_t *kernconfig_lwp;
+static int kernconfig_recurse;
+
void
assert_sleepable(void)
{
@@ -306,3 +311,59 @@ _kernel_unlock(int nlocks, int *countp)
if (countp != NULL)
*countp = olocks;
}
+
+/*
+ * Functions for manipulating the kernel configuration lock. This
+ * recursive lock should be used to protect all additions and removals
+ * of of kernel functionality, such as device configuration and loading
+ * of modular kernel components.
+ */
+
+void
+kernconfig_lock_init(void)
+{
+
+ mutex_init(&kernconfig_mutex, MUTEX_DEFAULT, IPL_NONE);
+ kernconfig_lwp = NULL;
