Re: [PATCH linux-2.6 04/04] brsem: convert cpucontrol to brsem

[Nathan Lynch <ntl@pobox.com>]

Nick Piggin wrote:
> Tejun Heo wrote:
> 
> >+/*
> >+ * cpucontrol is a brsem used to synchronize cpu hotplug events.
> >+ * Invoking lock_cpu_hotplug() read-locks cpucontrol and no
> >+ * hotplugging events will occur until it's released.
> >+ *
> >+ * Unfortunately, brsem itself makes use of lock_cpu_hotplug() and
> >+ * performing brsem write-lock operations on cpucontrol deadlocks.
> >+ * This is avoided by...
> >+ *
> >+ * a. guaranteeing that cpu hotplug events won't occur during the
> >+ *    write-lock operations, and
> >+ *
> >+ * b. skipping lock_cpu_hotplug() inside brsem.
> >+ *
> >+ * #a is achieved by acquiring and releasing cpucontrol_mutex outside
> >+ * cpucontrol write-lock.  #b is achieved by skipping
> >+ * lock_cpu_hotplug() inside brsem if the current task is
> >+ * cpucontrol_mutex holder (is_cpu_hotplug_holder() test).
> >+ *
> >+ * Also, note that cpucontrol is first initialized with
> >+ * BRSEM_BYPASS_INITIALIZER and then initialized again with
> >+ * __create_brsem() instead of simply using create_brsem().  This is
> >+ * necessary as cpucontrol brsem gets used way before brsem subsystem
> >+ * becomes up and running.
> >+ *
> >+ * Until brsem is properly initialized, all brsem ops succeed
> >+ * unconditionally.  cpucontrol becomes operational only after
> >+ * cpucontrol_init() is finished, which should be called after
> >+ * brsem_init_early().
> >+ */
> 
> Mmm, this is just insane IMO.
> 
> Note that I happen to also think the idea (brsems) have merit, and
> that cpucontrol may be one of the places where a sane implementation
> would actually be useful... but at least when you're introducing
> this kind of complexity anywhere, you *really* need to be able to
> back it up with numbers.

The only performance-related complaint with cpu hotplug of which I'm
aware -- that taking a cpu down on a large system can be painfully
slow -- resides in the "write side" of the code, which is not the case
that the brsem implementation optimizes.  I think this patch would
make that case even worse.  So I don't think it's appropriate to use a
brsem for cpu hotplug, especially without trying rwsem first.


Nathan