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-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
