Date:	Fri, 15 Aug 2008 14:58:18 +0200
From:	Ingo Molnar <mingo@...e.hu>
To:	Nick Piggin <nickpiggin@...oo.com.au>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	linux-kernel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	"David S. Miller" <davem@...emloft.net>
Subject: Re: [git pull] core fixes


* Nick Piggin <nickpiggin@...oo.com.au> wrote:

> On Tuesday 12 August 2008 18:05, Nick Piggin wrote:
> > On Tuesday 12 August 2008 16:13, Nick Piggin wrote:
> > > On Tuesday 12 August 2008 08:20, Ingo Molnar wrote:
> > > > Nick Piggin (1):
> > > >       generic-ipi: fix stack and rcu interaction bug in
> > > > smp_call_function_mask()
> > >
> > > I'm still not 100% sure that I have this patch right... I might have seen
> > > a lockup trace implicating the smp call function path... which may have
> > > been due to some other problem or a different bug in the new call
> > > function code, but if some more people can take a look at it before
> > > merging?
> >
> > OK indeed it did have a couple of bugs. Firstly, I wasn't freeing the
> > data properly in the alloc && wait case. Secondly, I wasn't resetting
> > CSD_FLAG_WAIT in the for each cpu loop (so only the first CPU would
> > wait).
> >
> > After those fixes, the patch boots and runs with the kmalloc commented
> > out (so it always executes the slowpath).
> 
> Hmm, thanks for merging this Ingo, but it didn't get upstream very 
> nicely (first was merged the one totally broken patch I didn't even 
> really test let alone propose for inclusion!). Then some window of 
> other patches, then the incremental patch to fix it.
> 
> I know it probably is easiest for your "vacuum the mailing list" mode 
> of workflow. And it's not that I don't appreciate it, but what I care 
> about most is the end result and that is suboptimal in this case.
> 
> The argument that we lose the "thought process" of coming up with a 
> correct patch I don't really buy into either. We want to document the 
> thought process of well thought out, well reviewed and tested patches 
> -- if they get merged and subsequently found to be broken, it is 
> really nice to be able to look back at how and why they went wrong[*].

generally i agree and replace patches - but in this case i went for the 
delta because -rc3 was imminent and the previous patch was already 
well-tested with practical workloads, even though broken in the 
slowpath. Also, in terms of judging risks, it was easier to look at the 
delta between the two commits and say "that obviously cannot make it 
worse than the current code". But it's all a special-case really.

> But merging bits and pieces of such raw patches IMO just adds too much 
> noise to the tree, and breaks bisection too easily. In the case of my 
> patch, the kernel will still build and mostly run, but that is 
> actually even a worse way to break the biesection if you are hunting 
> for some obscure and hard to reproduce bug.

Note that the two commits were kept together tightly so the chance of 
bisection going in the middle of it _and_ hitting the obscure slowpath 
is reasonably small. What is wrong is to keep them apart (say merge a 
full upstream release between them) and break bisection on a wide basis.

Btw., we could add "dont bisect into this other commit" markers to later 
commits. Bisection is a relatively slow operation anyway, so checking a 
graph of bisection markers would be within its reach without causing any 
practical slowdown of bisection. (I think.)

> [*] BTW. it would be nice to have some formal regression: and/or link: 
> tags in git patches which I think would help that kind of analysis and 
> also help distros to pick up fixes...

yeah. Perhaps also some tags to note their lkml thread identity.

	Ingo
--
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