lkml.org 
[lkml]   [2008]   [Aug]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH -V3 01/11] percpu_counters: make fbc->count read atomic on 32 bit architecture
On Wed, 27 Aug 2008 23:01:52 +0200
Peter Zijlstra <a.p.zijlstra@chello.nl> 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.


\
 
 \ /
  Last update: 2008-08-27 23:27    [W:0.106 / U:16.088 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site