Re: [pchecks v2 2/2] percpu: Add preemption checks to __this_cpu ops

[Ingo Molnar]

* Christoph Lameter <cl@xxxxxxxxx> wrote:

> On Thu, 3 Oct 2013, Ingo Molnar wrote:
> 
> > _You_ added the facility with broken (== non-existent) preemption 
> > debugging for __this_cpu ops, _you_ caused Peter Zijstra and others to 
> > waste time due to you ignoring those requests to add debugging. 
> > Everyone rightfully expected _you_ to fix the problem you introduced.
> 
> Oh what __this_cpu ops was doing was clearly documented at the time it 
> was merged.

In hindsight it was merged prematurely - such things happen. After it was 
merged multiple people ran into problems: Thomas Gleixner, Peter Zijlstra 
and myself. IIRC at the Kernel Summit Linus agreed as well that not having 
__this_cpu() debug ops was a mistake.

If your only argument left is "but it was merged in that inferior form", 
and you refuse to fix its shortcomings, then our answer is to learn from 
our mistake and not merge patches from you in the future, until the 
facility is fixed.

> > And now you blame the victims of your sloppiness, that they should 
> > have fixed the problem you introduced?
> 
> I fix problems that others introduce into my subsystems as well. If 
> there is a problem then usually someone shows up with patches to address 
> these.
> 
> > People wasting time and the kernel becoming less robust is not a minor 
> > issue at all.
> 
> Well then I would have expected someone to show up with patches 
> following through on what was discussed. I am no expert on preemption.

If you don't understand the impact of your changes and the fragility it 
introduces then that's one more reason to not merge more patches from you 
until you rectify your omissions.

> > As a starting point it would be fine if you tested it on your own 
> > systems with all relevant debugging enabled...
> 
> Ok done that.
> 
> > > These two patches will allow this testing to be done. And I do not see
> > > any mention of technical issues with the code. [...]
> >
> > Here's the list of open technical problems:
> >
> >  - Lack of testing - you have not stated it whether any warnings 
> >    trigger with those two patches applied and debugging enabled, on 
> >    your systems.
> 
> I have posted an earlier patchset which includes fixes for the warnings 
> that were triggered. It described the testing that was done.

That 'earlier' patch set was with a different version of the preemption 
checks, and it's not at all clear whether the same warnings still trigger 
with your latest series.

> >  - I pointed out in detail how your last submission was broken in 
> >    several places which show lack of time and care on the patch 
> >    series.
> 
> This is regarding the garbled subject line in your inbox? So you want me 
> to fix the quilt tool now? I can just make this a single patch if that 
> helps?

Other people are able to submit patch series with non-garbled subject 
lines using Quilt and other tools. You should not blame Quilt for your 
messed up submission.

> >  - Your statement in the discussion that warnings will trigger with
> >    the debug option enabled points to an obvious technical problem as 
> >    well - all warnings known to trigger by you should be fixed by you, 
> >    as part of the series.
> 
> The problem is that you raised more issues related to the fixes that I 
> posted. I dont think this can be handled in one patchset.

I disagree - but in any case the patch set is not acceptable for upstream 
in its current form and my NAK stands, until this is fixed.

Thanks,

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