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

[Andrew Morton <akpm@linux-foundation.org>]

On Wed, 2 Jan 2013 23:46:46 +0000
Ben Hutchings <bhutchings@solarflare.com> wrote:

> On Wed, 2013-01-02 at 15:12 -0800, Andrew Morton wrote:
> > On Wed,  2 Jan 2013 13:52:25 -0800
> > David Decotigny <decot@googlers.com> wrote:
> > 
> > > In some cases, free_irq_cpu_rmap() is called while holding a lock
> > > (eg. rtnl). This can lead to deadlocks, because it invokes
> > > flush_scheduled_work() which ends up waiting for whole system
> > > workqueue to flush, but some pending works might try to acquire the
> > > lock we are already holding.
> > > 
> > > This commit uses reference-counting to replace
> > > irq_run_affinity_notifiers(). It also removes
> > > irq_run_affinity_notifiers() altogether.
> > 
> > I can't say that I've ever noticed cpu_rmap.c before :( Is is too late
> > to review it?
> > 
> > - The naming is chaotic.  At least these:
> > 
> > 	EXPORT_SYMBOL(alloc_cpu_rmap);
> > 	EXPORT_SYMBOL(free_cpu_rmap);
> > 	EXPORT_SYMBOL(cpu_rmap_add);
> > 	EXPORT_SYMBOL(cpu_rmap_update);
> > 	EXPORT_SYMBOL(free_irq_cpu_rmap);
> > 	EXPORT_SYMBOL(irq_cpu_rmap_add);
> > 
> >   should be consistently named cpu_rmap_foo()
> 
> There is a common practice of defining alloc_foo() and free_foo()
> alongside foo_do_this() and foo_do_that().  I deliberately chose to
> follow that.  If this is deprecated then it should be documented
> somewhere.

I don't think anyone has thought about it to that extent.  I always
recommend that the exported identifiers be named as
subsysid_functionname() and cannot think of any reason for
special-casing alloc and free.

> > - What's the locking model?  It appears to be caller-provided, but
> >   it is undocumented.
> 
> I think caller-provided can be assumed as the default for library code.

Nope, a lot of library code does internal locking.  And boy, does that
cause problems!  Experience tells us that caller-provided locking is
better.  But to avoid nasty problems, the library should clearly
document its locking requirements!

Bear in mind that spinlocks and mutexes aren't the only form of locks. 
A caller may wish to use an rwsem or rwlock, either to get parallelism
in cpu_rmap_lookup_index() and cpu_rmap_lookup_obj(), or because they
were already using such a lock.  The cpu_rmap() locking documentation
should describe which interface calls are OK with read-side locking. 
Not only to instruct users, but also to act as a constraint upon future
developers of the cpu_rmap code.  It becomes a contract saying "if you
use read_lock() for this, we won't later break your stuff".

> And IRQ setup and teardown need to be properly serialised in the driver
> already.
> 
> >   drivers/net/ethernet/mellanox/mlx4/ appears to be using
> >   msix_ctl.pool_lock for exclusion, but I didn't check for coverage.
> >
> >   drivers/net/ethernet/sfc/efx.c seems to not need locking because
> >   all its cpu_rmap operations are at module_init() time.
> >
> >   The cpu_rmap code would be less of a hand grenade if each of its
> >   interface functions documented the caller's locking requirements.
> 
> This particular 'hand grenade' *was* documented.  So I don't think
> documentation is the problem.

Dunno what you're referring to here.  There is no cpu_rmap() locking
documentation.

> > As for this patch: there's no cc:stable here but it does appear that
> > the problem is sufficiently serious to justify a backport, agree?
> [...]
> 
> Not sure.  So far as I can see, nothing called free_irq_cpu_rmap() while
> holding the RTNL lock before v3.8-rc1.  If there can be work items on a
> global workqueue that lock a PCI device (perhaps EEH?) then stable
> versions may also be affected.

OK.  The patch is rather non-trivial so I guess we aim for 3.8-only
for now.