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

[Paul Goyette <paul%whooppee.com@localhost>]

On Fri, 6 Aug 2010, Mindaugas Rasiukevicius wrote:

> Hello,
>
> Paul Goyette <paul%whooppee.com@localhost> 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  |
-------------------------------------------------------------------------