[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <alpine.LFD.2.00.1006090857530.4506@i5.linux-foundation.org>
Date: Wed, 9 Jun 2010 09:06:37 -0700 (PDT)
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: tytso@....edu
cc: Ingo Molnar <mingo@...e.hu>, Salman <sqazi@...gle.com>,
Andrew Morton <akpm@...ux-foundation.org>,
peterz@...radead.org, akpm@...x-foundation.org,
linux-kernel@...r.kernel.org, tytso@...gle.com,
Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH] Fix a race in pid generation that causes pids to be
reused immediately.
On Wed, 9 Jun 2010, tytso@....edu wrote:
>
> Salman had created a very nice ASCII art diagram of the race in the
> mail thread with the internal bug reporter who noticed the problem.
> We could include that, if you don't mind the commit description
> growing by 30-40 lines. :-)
I don't horribly mind, but I also don't think it's necessarily all that
useful to go that far. For the commit message, there are two real reasons:
- explaining the patch itself upstream to make people like me understand
_why_ it needs to be applied.
But then a denser explanation than a 30-40 line ASCII art diagram would
probably suffice.
- explaning the code after-the-fact to people who end up seeing it in
log/blame output
And then we don't care so much about the old bug per se, as about how
the code is supposed to work - so a lot of verbiage about what used to
happen is much less important than describing just what's going on with
the cmpxchg (where the "loop" is all the way from the original value
load, especially in this case where we don't have to loop all the way
back).
In fact, conceptually the cmpxchg loop is pretty much the whole function,
it's just that if we know that any racing elements will be idempotent wrt
anything but the final assignment of 'last_pid'. So we can end up just
looping over the final part. I think _that_ is the clever part, and I
think that is the part that needs explanation.
(And that is also why the diff itself doesn't include the "early part of
the loop", and why I couldn't understand it purely as a patch - because it
in this case the patch is "artificially small" because of how we don't
need to loop all the way up)
> We do need to deal with pid wrap possibility just to be completely
> correct, although the chance of hitting _that_ are pretty remote.
That seems to be purely an artifact of the cleverness of avoiding re-doing
the whole loop, no? If we _had_ re-done the whole loop (the "normal"
cmpxchg model), we would have re-started the whole pid search and handled
the overflow in the existing overflow handling code.
Linus
--
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