Re: [RFC/PATCH] Multithread initcalls to auto-resolve orderingissues.

From: NeilBrown
Date: Sun Jan 08 2012 - 22:29:41 EST


On Mon, 09 Jan 2012 12:05:28 +1100 Ryan Mallon <rmallon@xxxxxxxxx> wrote:

> On 09/01/12 11:21, NeilBrown wrote:
>
> >
> > [ To: list stolen from "introduce External Connector Class (extcon)" thread
> > where this topic was briefly mentioned. ]
> >
> > Hi,
> > I welcome review/feedback on the following idea and code.
>
>
> Hi Neil,
>
> Interesting idea, some comments below.
>

Thanks for the quick review!

> > Proposed solution:
> >
> > The solution I am proposing is to:
> > 1/ allow the 'request' functions which find and provide a resource to block
> > until the resource is available
> > 2/ run the init calls in multiple threads so that when one blocks waiting
> > for a resource, another starts up to run subsequent initcalls until
> > the blocking call gets its resource and can complete.
>
> What happens if the resource isn't actually present (driver/hardware
> missing or resource probe fails for example)? Does the initcall thread
> get stuck forever, or is there a timeout?

With my code it will get stuck forever.
What is the alternative? Just fail?

Is this a realistic scenario? If it is we would probably need some way to
say "this resource is no longer expected" .. sounds messy.

>
> > Details and issues.
> >
> > 1/ Sometimes it is appropriate to fail rather than to block, and a resource
> > providers need to know which.
> > This is unlikely to be an issue with GPIO and IRQ as is the identifying
> > number is >= 0, then the resource must be expected at some stage.
> > However for regulators a name is not given to the driver but rather the
> > driver asks if a supply is available with a given name for the device.
> > If not, it makes do without.
> > So for regulators (and probably other resources) we need to know what
> > resources to expect so we know if a resource will never appear.
> >
> > In this sample code I have added a call
> > regulator_has_full_constraints_listed()
> > which not only assures that all constraints are known, but list
> > them (array of pointers to regulator_init_data) so when a supply is
> > requested the regulator code can see if it expected.
> >
> > 2/ We probably don't want lots of threads running at once as there is
> > no good way to decide how many (maybe num_cpus??).
> > This patch takes the approach that only one thread should be runnable
> > at once in most cases.
>
>
> num_cpus won't work for UP systems. In practice the number of threads
> needed is probably low, since most devices probably only have a short
> dependency chain.

My reference to "num_cpus" was for how many threads should be runnable
concurrently (rather than blocking on something) so a value of '1' would be
OK and is what I (mostly) impose with the current code.

The total number of threads created with be the number of devices that are
dependent on something later in the default initcall order. So it could be
longer than the longest chain if there are multiple long chains that all
block at once. It could likely be a few, quite possibly be dozens, very
unlikely to be hundreds unless you really had lots and lots of devices.


>
> >
> > We keep a count of the number of threads started and the number of
> > threads that are blocked, and only start a new thread when all threads
> > are blocked, and only start a new initcall when all other threads are
> > blocked.
> >
> > So when one initcall makes a resource available, another thread which
> > was waiting could wake up and both will run concurrently. However no
> > more initcalls will start until all threads have completed or blocked.
>
>
> With this approach having an unbounded number of threads should be okay
> right? The number of threads shouldn't exceed the length of a dependency
> chain.
>
> >
> > 3/ The parent locking that __driver_attach() does which is "Needed for USB"
> > was a little problem - I changed it to alert the initcall thread
> > management so it knew that thread was blocking. I think this wouldn't be
> > a problem is the parent lock was *only* taken for USB...
> >
> > 4/ It is possible that some existing code registers a resource before it is
> > really ready to be used. Currently that isn't a problem as no-one will
> > actually try to use it until the initcall has completed. However with
> > this patch the device that wants to use a resource can do so the moment
> > it is registered.
>
>
> Such code would be buggy and need fixing correct? This patch could
> possibly help identify such buggy code?

Probably. When drivers are built as modules the initcalls can already run in
parallel so getting the setup wrong could conceivably be an issue already -
but yes, I think this patch could make those bugs more obvious.

As an example of something that might be a bug (And I had to look hard to
find it), in gpio-pl061.c in the _probe function it calls:

set_irq_flags(i+chip->irq_base, IRQF_VALID);
irq_set_chip_data(i + chip->irq_base, chip);

Now I haven't set up any code for blocking when IRQs aren't available yet,
but I suspect that the point they become available is when set_irq_flags is
called to set IRQF_VALID. If some other driver immediately tired to use that
irq it would find that the ->chip_data is NULL. e.g. it could call
pl061_irq_enable() which immediately gets the chip_data and dereferences it.


> > @@ -1179,15 +1195,41 @@ int gpio_request(unsigned gpio, const char *label)
> > struct gpio_chip *chip;
> > int status = -EINVAL;
> > unsigned long flags;
> > + int can_wait = !in_atomic();
>
>
> I don't think you can reliably do this. Callers should always track
> whether they can sleep or not. See: http://lwn.net/Articles/274695/

Undoubtedly true. I'm not even sure that gpio_request can be called from
atomic context, but as spin_lock_irqsave was used (below) instead of
spin_lock_irq, I thought it safest to test.

If this gets past the initial review stage I'm sure I'll get help from
someone how knows this code to make it right.


Thanks again,

NeilBrown

>
> >
> > spin_lock_irqsave(&gpio_lock, flags);
> >
> > if (!gpio_is_valid(gpio))
> > goto done;
> > desc = &gpio_desc[gpio];
> > + if (desc->chip == NULL) {
> > + /* possibly need to wait for the chip to appear */
> > + struct gpio_waiter w;
> > + int status2 = 0;
> > + DEFINE_WAIT(wait);
> > + if (test_and_set_bit(FLAG_WAITING, &desc->flags))
> > + /* Only one waiter allowed */
> > + goto done;
> > + if (!can_wait)
> > + goto done;
> > +
> > + init_waitqueue_head(&w.queue);
> > + w.gpio = gpio;
> > + list_add(&w.list, &waiters);
> > + prepare_to_wait(&w.queue, &wait, TASK_UNINTERRUPTIBLE);
> > +
> > + while (desc->chip == NULL && status2 == 0) {
> > + spin_unlock_irqrestore(&gpio_lock, flags);
> > + status2 = initcall_schedule();
> > + spin_lock_irqsave(&gpio_lock, flags);
> > + prepare_to_wait(&w.queue, &wait, TASK_UNINTERRUPTIBLE);
> > + }
> > + finish_wait(&w.queue, &wait);
> > + list_del(&w.list);
> > + if (desc->chip == NULL)
> > + goto done;
> > + }
> > chip = desc->chip;
> > - if (chip == NULL)
> > - goto done;
> >
> > if (!try_module_get(chip->owner))
> > goto done;

Attachment: signature.asc
Description: PGP signature