Re: [PATCH v4] lib: cpu_rmap: avoid flushing all workqueues

[Ben Hutchings <bhutchings@solarflare.com>]

On Thu, 2013-01-03 at 13:47 -0800, Andrew Morton wrote:
> On Wed, 2 Jan 2013 18:35:00 -0800
> David Decotigny <decot@googlers.com> wrote:
> >
> 
> (please don't top-post)
> 
> > Thanks. It is not too late to review this code. But I'd prefer not to
> > address refactoring issues with this patch, which is supposed to fix a
> > deadlock with some drivers. So I'd rather not to add function
> > renaming, suppressions, etc. that have an effect outside cpu_rmap's
> > code. Instead, I'd like to propose another patch later, which will be
> > a little more intrusive in that respect, if that's ok with you.
> > 
> > I believe Ben answered your other concerns, I consider him as the
> > expert as to whether there should be finer-grained locking implemented
> > in this subsystem. Let me just add that I second him in saying that
> > the deadlock risk was clearly identified and mentioned in the doc.
> > Unfortunately, initial implementation makes this risk hard to
> > work-around for some drivers, which is what this patch proposes to
> > address.
> 
> ^^ all this pertains to the existing code.
> 
> > So, for now, I'd like to keep v4 as the current version. And some
> > refactoring will be done in a later patch.
> 
> ^^ this pertains to the current patch.  And no reason for ignoring my
> review comments was provided.
> 
> So I did it myself.  It's very simple, as free_cpu_rmap() has no callers.
> 
> Also, free_irq_cpu_rmap() is now distinctly fishy - it runs all the
> notifiers every time it is called.

It removes the notifiers.

> Surely it should only do that when the refcount falls to zero?
[...]

No, absolutely not.  An IRQ cpu_rmap may have multiple references to it
now, but it only has one owner: the driver that allocates it and the
associated IRQs.

As soon as the driver frees those IRQs, they may be allocated by some
other driver, so we require that notifiers are removed from them first
(see the WARN_ON in free_irq()).  Also, if free_irq_cpu_rmap() did not
remove the notifiers, more notifications could be scheduled and keep the
cpu_rmap alive indefinitely.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.