tech-kern archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: Module and device configuration locking [was Re: Modules loading modules?]

On Fri, 6 Aug 2010, Mindaugas Rasiukevicius wrote:


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.


+       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!  :)

+       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    |
| Network Engineer | 0786 F758 55DE 53BA 7731 | pgoyette at |
| Kernel Developer |                          | pgoyette at  |

Home | Main Index | Thread Index | Old Index