Date:	Wed, 11 Nov 2009 04:25:46 +0900
From:	Tejun Heo <tj@...nel.org>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
CC:	Linux Kernel <linux-kernel@...r.kernel.org>,
	Yinghai Lu <yhlu.kernel@...il.com>, Ingo Molnar <mingo@...e.hu>
Subject: Re: [GIT PULL] percpu fixes for 2.6.32-rc6

Hello,

Linus Torvalds wrote:
> On Wed, 11 Nov 2009, Tejun Heo wrote:
>> If I'm missing something, I'm sure you'll hammer it into me.
> 
> Here's from the comments on that function:
> 
>  * RETURNS:
>  * 0 if noop, 1 if successfully extended, -errno on failure.
> 
> and here's from one of the main callers:
> 
>                 list_for_each_entry(chunk, &pcpu_slot[slot], list) {
> 		...
>                         switch (pcpu_extend_area_map(chunk, &flags)) {
>                         case 0:
>                                 break;
>                         case 1:
>                                 goto restart;   /* pcpu_lock dropped, restart */
> 
> where that '&pcpu_slot[slot]' list is protected by the pcpu_lock, and 
> nothing else. At least according to all the _other_ comments in that file. 
> Including the one that very much tries to _explain_ the locking at the 
> top, quote:

Oh, yeah, right.  I was too fixated on the part modified by the patch.

>   "The latter is a spinlock and protects the index data structures - chunk 
>    slots, chunks and area maps in chunks."
> 
> So as far as I can tell, either the comments are all crap, the whole 
> restart code is pointless and in fact the whole spin-lock is seemingly 
> almost entirely pointless to begin with (since pcpu_alloc_mutex is the 
> only thing that matters), or the code is buggy.

The return value is wrong but it wouldn't lead to oops.  There's a
very slight chance that it might end up creating extra chunk when not
necessary - probably why it went unnoticed all this time.  The
spin-lock is only to allow free_percpu() to be called from atomic
context, so its usefulness would only be visible if you look at
free_percpu() too.

> Also, quite frankly, even if the code _isn't_ buggy, it's still wrong to 
> release a lock that somebody else took. It's a sure-fire way to write 
> unmaintainable code with bugs almost guaranteed in the future. Yes, it 
> happens, and sometimes it's the only sane way to do it, but in this case 
> that really isn't true as far as I can tell.
> 
> From my (admittedly fairly quick) look, my suggested split-up really would 
> make the code _more_ readable (no need for that subtle "negative, zero or 
> positive all mean different things" logic), and hopefully avoid the whole 
> "drop the lock that somebody else took", because we could drop it in the 
> caller where it was taken.
> 
> So it all boils down to: the code is unquestionably ugly and almost 
> certainly broken. And if it isn't broken, then _all_ the comments are 
> total crap. 

Yeap, the return value definitely is broken and the rather ugly
calling convention is remanant from the days when there was only
single mutex protecting the whole thing.

I think this type of function is a bit special in locking requirement
tho.  The initial step - checking whether the operation is necessary -
requires lock and the final step - copying over to the new thing and
installing it - also requires the lock, so unless there's one
unnecessary unlock/lock pair, the second function would be called
without lock but return with lock, which probably is safer than
releasing and regrabbing lock in the middle but still not quite
pretty.

In this case, as the second function needs to release to free the old
map, the extra unlock/lock pair is actually necessary.  Splitting into
two functions is fine but I think it would be better to fix it first
and then split them in following patches so that it can be bisected if
I screw up while splitting, right?

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists