Subject: Re: sysmon vs mutexes
To: Juan RP <juan@xtrarom.org>
From: Andrew Doran <ad@netbsd.org>
List: tech-kern
Date: 02/19/2007 20:45:34
Hi,

On Mon, Feb 19, 2007 at 07:38:52PM +0100, Juan RP wrote:

> as you might know, two days ago I sent patches to Andrew Doran
> to convert simple_locks/lockmgr and ltsleep/wakeup pairs in parts of
> the sysmon code. I thought that was ok because I was using a kernel
> without LOCKDEBUG enabled... sigh.
> 
> After booting a kernel with LOCKDEBUG, there were uninitialized locks
> somewhere in the code... so we did start searching what was wrong,
> and we know what was the problem:
> 
> * Drivers using the envsys framework usually are using
>    sysmon_envsys_register().
> * dev/acpi/acpi_apm uses sysmonioctl_envsys() directly.
> * Any code in the kernel might call sysmon_envsys_find().
> 
> My original patches were initializing the mutexes in
> sysmon_envsys_register() (I didn't know that acpi_apm.c use
> sysmonioctl_envsys() directly), so every time that apmd was about to
> run, BOOM.
> 
> So how we did not find any easy solution and the changes were reverted.
> 
> This morning I have found a way to make this work, and now I'm
> running a kernel with sysmon_envsys.c converted, the patch is at:
> 
> http://www.xtrarom.org/patches/newlock2/sme.diff
> 
> But this way seems not the correct way to initialize the mutexes,
> because right now I'm using a function for initialization and I run
> it in the functions that were having errors.
> 
> I would like to know if there's a better way to make them work
> correctly... do you have any suggestion or comment?

Basically, the problem here is that we want to replace the static
initializers (SIMPLELOCK_INITIALIZER and friends) with dynamic
initialization, so calls to mutex_init().

mutex_init() needs to be done at run time so that when the kernel is built
with the LOCKDEBUG option, we can allocate a unique ID for every lock in the
system. The basic idea is that the size of a kmutex_t or krwlock_t will
never change, regardless of what options the kernel was compiled with.
That's useful for LKMs and for kernel grovellers like vmstat.

There is no one clear place in this code where the locks can be initialized
dynamically. There are a few ways around it that have been suggested:

o Provide static initializers. This is a problem where lock init needs to do
  something more complicated than just setting up the structure, as is the
  case now.

o Provide static initializers, but set in dummy values, and have the first
  call to mutex_enter() or similar set up the lock. I don't think this one
  is possible, because mutex_init() may need to allocate memory, and so may
  need to sleep or take other locks.

o Use linksets. I don't like linksets; I think that they are somewhat LKM
  unfriendly, and make providing a stable kernel API harder. I much prefer
  that we have defined entry points for each LKM / code module and e.g.
  device instance where we set things up at runtime.

o Use RUN_ONCE() (from sys/once.h), and call an init function on every
  entry into the sysmon driver. I'm not fond of this one; it seems
  like a lot of unnecessary overhead.

o Figure out a way to provide an initialization function for envsys that
  gets called at boot/attach time. It's a character device, so there should
  be a common attach point we can use where we rely on autoconf to provide
  the necessary serialization, until the device/module has it's own lock(s)
  set up. I'm of the opinion that this the best way to handle it. I'll have
  a look for a place where we can provide such a function.

What do people think about the general issues involved? Any other suggestions
about the problem at hand (envsys)?

Cheers,
Andrew