Re: [PATCH -V3 01/11] percpu_counters: make fbc->count read atomicon 32 bit architecture

[Andrew Morton]

On Wed, 27 Aug 2008 23:01:52 +0200
Peter Zijlstra <a.p.zijlstra@xxxxxxxxx> wrote:

> > 
> > > +static inline s64 percpu_counter_read(struct percpu_counter *fbc)
> > > +{
> > > +	return fbc_count(fbc);
> > > +}
> > 
> > This change means that a percpu_counter_read() from interrupt context
> > on a 32-bit machine is now deadlockable, whereas it previously was not
> > deadlockable on either 32-bit or 64-bit.
> > 
> > This flows on to the lib/proportions.c, which uses
> > percpu_counter_read() and also does spin_lock_irqsave() internally,
> > indicating that it is (or was) designed to be used in IRQ contexts.
> 
> percpu_counter() never was irq safe, which is why the proportion stuff
> does all the irq disabling bits by hand.

percpu_counter_read() was irq-safe.  That changes here.  Needs careful
review, changelogging and, preferably, runtime checks.  But perhaps
they should be inside some CONFIG_thing which won't normally be done in
production.

otoh, percpu_counter_read() is in fact a rare operation, so a bit of
overhead probably won't matter.

(write-often, read-rarely is the whole point.  This patch's changelog's
assertion that "Since fbc->count is read more frequently and updated
rarely" is probably wrong.  Most percpu_counters will have their
fbc->count modified far more frequently than having it read from).

> > It means that bdi_stat() can no longer be used from interrupt context.
> 
> Actually, as long as the write side of the seqlock usage is done with
> IRQs disabled, the read side should be good.

yup.

> If the read loop gets preempted by a write action, the seq count will
> not match up and we'll just try again.
>
> The only lethal combination is trying to do the read loop while inside
> the write side.

yup
 
> If you look at backing-dev.h, you'll see that all modifying operations
> disable IRQs.

OK.

> > So a whole lot of thought and review and checking is needed here.  It
> > should all be spelled out in the changelog.  This will be a horridly
> > rare deadlock, so suitable WARN_ON()s should be added to detect when
> > callers are vulnerable to it.
> > 
> > Or we make the whole thing irq-safe.
> 
> That'd rather substantially penalize those cases where we don't need it.
> >From what I understood this whole pushf/popf stuff is insanely expensive
> on a few archs.

Sure.  I _expect_ that this interface change won't actually break
anything.  But it adds a restriction which we should think about, and
document.



btw, what the heck is percpu_counter_init_irq()?  Some mysterious
lockdep-specific thing?

<does git-fiddle.  Oh.  crappy changelog.>

I let that one leak through uncommented.  Must be getting old. 
Probably it will need an EXPORT_SYMBOL() sometime.


I expect that if we're going to go ahead and make percpu_counter_read()
no longer usable from interrupt context then we'll eventually end up
needing the full suite of _irq() and _irqsave() interface functions. 
percpu_counter_add_irqsave(), etc.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/