On Tue, 23 Oct 2012, Ming Lei wrote:
> On Mon, Oct 22, 2012 at 10:33 PM, Alan Stern <st...@rowland.harvard.edu>
> wrote:
> >
> > Tail recursion should be implemented as a loop, not as an explicit
> > recursion. That is, the function should be:
> >
> > void pm_runtime_set_memalloc_noio(struct device *dev, bool enable)
> > {
> > do {
> > dev->power.memalloc_noio_resume = enable;
> >
> > if (!enable) {
> > /*
> > * Don't clear the parent's flag if any of the
> > * parent's children have their flag set.
> > */
> > if (device_for_each_child(dev->parent, NULL,
> > dev_memalloc_noio))
> > return;
> > }
> > dev = dev->parent;
> > } while (dev);
> > }
>
> OK, will take the non-recursion implementation for saving kernel
> stack space.
>
> >
> > except that you need to add locking, for two reasons:
> >
> > There's a race. What happens if another child sets the flag
> > between the time device_for_each_child() runs and the next loop
> > iteration?
>
> Yes, I know the race, and not adding a lock because the function
> is mostly called in .probe() or .remove() callback and its parent's device
> lock is held to avoid this race.
>
> Considered that it may be called in async probe() (scsi disk), one lock
> is needed, the simplest way is to add a global lock. Any suggestion?
No. Because of where you put the new flag, it must be protected by
dev->power.lock. And this means the iterative implementation shown
above can't be used as is. It will have to be more like this:
void pm_runtime_set_memalloc_noio(struct device *dev, bool enable)
{
spin_lock_irq(&dev->power.lock);
dev->power.memalloc_noio_resume = enable;
while (dev->parent) {
spin_unlock_irq(&dev->power.lock);
dev = dev->parent;
spin_lock_irq(&dev->power.lock);
/*
* Don't clear the parent's flag if any of the
* parent's children have their flag set.
*/
if (!enable && device_for_each_child(dev->parent, NULL,
dev_memalloc_noio))
break;
dev->power.memalloc_noio_resume = enable;
}
spin_unlock_irq(&dev->power.lock);
}
> > Even without a race, access to bitfields is not SMP-safe
> > without locking.
>
> You mean one ancestor device might not be in active when
> one of its descendants is being probed or removed?
No. Consider this example:
struct foo {
int a:1;
int b:1;
} x;
Consider what happens if CPU 0 does "x.a = 1" at the same time as
another CPU 1 does "x.b = 1". The compiler might produce object code
looking like this for CPU 0:
move x, reg1
or 0x1, reg1
move reg1, x
and this for CPU 1:
move x, reg2
or 0x2, reg2
move reg2, x
With no locking, the two "or" instructions could execute
simultaneously. What will the final value of x be?
The two CPUs will interfere, even though they are touching different
bitfields.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
