Re: [PATCH] [RFC] kernel/cpu: Use lockref for online CPU reference counting

From: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>,
	Intel graphics driver community testing & development
	<intel-gfx@lists.freedesktop.org>,
	Linux kernel development <linux-kernel@vger.kernel.org>,
	David Hildenbrand <dahi@linux.vnet.ibm.com>,
	"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>,
	Ingo Molnar <mingo@kernel.org>
Subject: Re: [PATCH] [RFC] kernel/cpu: Use lockref for online CPU reference counting
Date: Tue, 16 Feb 2016 12:51:03 +0200	[thread overview]
Message-ID: <1455619863.4977.29.camel@linux.intel.com> (raw)
In-Reply-To: <20160216091440.GT6357@twins.programming.kicks-ass.net>

On ti, 2016-02-16 at 10:14 +0100, Peter Zijlstra wrote:
> On Tue, Feb 16, 2016 at 10:49:36AM +0200, Joonas Lahtinen wrote:
> > I originally thought of implementing this more similar to what you
> > specify, but then I came across a discussion in the mailing list where
> > it was NAKed adding more members to task_struct;
> > 
> > http://comments.gmane.org/gmane.linux.kernel/970273
> > 
> > Adding proper recursion (the way my initial implementation was going)
> > got ugly without modifying task_struct because get_online_cpus() is a
> > speed critical code path.
> 
> Yeah, just don't let Linus hear you say that. get_online_cpus() is _not_
> considered performance critical.

Oh well, at least changes to it added quite noticeably to the bootup
time of a system.

> 
> > So I'm all for fixing the current code in a different way if that will
> > then be merged.
> 
> So I'm not sure why you're poking at this horror show to begin with.
> ISTR you mentioning a lockdep splat for SKL, but failed to provide
> detail.
> 

Quoting my original patch;

"See the Bugzilla link for more details.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93294"

The improvement my patch implements is to use lockref for locked
reference counting (hotplug code previously rolled its own mutex +
atomic combo), which gets rid of the deadlock scenario described and
linked in the initial patch. Trace for the scenario;

https://bugs.freedesktop.org/attachment.cgi?id=121490

I think using lockref makes it substantially less special, lockref code
being a lot more battle-tested in the FS code than the previous
cpu_hotplug.lock mess.

> Making the hotplug lock _more_ special to fix that is just wrong. Fix
> the retarded locking that lead to it.
> 

I do agree that it's still not pretty, but now it does correctly what
the previous code was trying to do with custom mutex + atomic.

I'm all for fixing the code further, but prior to proceeding there
needs to be some sort of an agreement on either making
get_online_cpus() slower (which does not seem like a good idea) or
adding more members to task_struct.

Regards, Joonas

> 
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx