Sasha, Sasha Levin <sas...@kernel.org> writes: > Thank you for taking the time to review this.
welcome and sorry for the explosion. > On Mon, May 18, 2020 at 08:20:08PM +0200, Thomas Gleixner wrote: >>Sasha Levin <sas...@kernel.org> writes: >>This conditional irqsave gunk is clearly NOT what was in the tip tree >>before it got reverted: >> >> a86b4625138d ("x86/fsgsbase/64: Enable FSGSBASE instructions in helper >> functions") > > It wasn't in the reverted series, it came in Intel's v9 series, with > these comments in the cover letter: > > Updates from v8 [10]: > [...] > * Simplified GS base helper functions (Tony L.) Ok. I never looked at that series because that requested confirmation that nothing will regress due to the ptrace changes was not there. After a bit of handwaving this dried out. So I completely missed that back then. And I did not look at any later variant which had 0day complaints. > And I did, Thomas. While I'm not intimately familiar with the code I > made sure that all the patches that came on top of the merged series > before it got reverted made it into this new series. However, more work > has happened here after the revert and I would expect that the code in > this new series will be different than the code you reverted last > year. It's obvious that it would be different from what was merged simply because the affected code has changed but not in substantial points like losing a kprobes protection by "simplifying" something which was carefully done in the first place. It's not your fault at all, you just happened to be the messanger. The people responsible for that mess owe you at least a beer. >> - In paranoid_exit() >> >>- TRACE_IRQS_IRETQ_DEBUG >>+ TRACE_IRQS_OFF_DEBUG > > (assuming we're looking at the same thing here, ) Changed in v8 of the > series. Sigh. > I'm a bit confused about the surprise here that v12 is different than > the reverted patches. There were multiple rounds of review which > resulted in the code being more than just a revert of the revert along > with a small fix. > >> > This looks unpleasant to review. I wonder if it would be better to >> > unrevert >> > the reversion, merge up to Linus' tree or -tip, and then base the changes >> > on >> > top of that. >> >> I don't think that's a good idea. The old code is broken in several ways >> and not bisectable. So we really better start from scratch. > > And this is what we have here, a series that has more than trivial > differences from the revert, and is more of a pain to review. Look at > what you did with your 25 minutes: you've reverted the revert and went > on to apply fixes on top of it, exactly the thing you've asked > not to do a few months prior. I did that to analyse whether that new series has everything what was fixed back then and did not introduce new bugs. Mission accomplished. > No need to worry about me sending a new series, as I can't - I just > don't know what you want to see at this point: on one hand you're saying > "we really better start from scratch" and on the other hand "this > conditional irqsave gunk is clearly NOT what was in the tip tree before > it got reverted", you're making suggestions to change comments only to > later complain that "a gazillion of comment changes make reading the > diff harder". Gah. That comment change thing was just an annoyance and I complained about it because I was already grumpy as hell. So what I meant is that the blind revert of the revert, i.e. just reapplying the previous stuff, is horrible. Simply because the reverted patches were already not bisectable. And then applying random changes on top does not make it any better. So yes, I would have done exactly where I started: 1) Extract the original patches from git 2) Apply them and fixup the rejects and on top of that: 3) Make them bisectable by folding back the fixes to the right place and reordering them which creates a result which is equivalent to 'start from scratch' but without losing context and introducing new bugs. Simply because it's trivial to diff against the state before the revert. 4) Do the 'improvements' on top, discuss them and fold them back. For what you tried to do I would have omitted #4 completely and then did: 5) Rebase the latest Intel variant 6) Diff the results ideally step by step 7) Analyze the deltas carefully and if unsure about the result ask. That way you really would have noticed that this helper patch is substantially different and you would have noticed that the kprobes protection is gone. Also that would have clearly shown you the IRQ flag wreckage. So to go forward can you please just do #1 - #3 first? Vs. the s/GSBASE/GS base/g comment changes: I don't mind them per se, but they are incomplete because they just change it in the new code while there are still the original comments using GSBASE. So either we change it wholesale or not at all. If so, then this wants to be a separate patch right at the beginning of the new series which changes the existing comments before introducing a different variant. That "simplified" handling is going nowhere. That conditional irq disable and the redundant conditionals and the out of line invocation in switch_to() are just not going to happen. So when comparing it to the latest Intel trainwreck ignore that part completely, I've uploaded my quick shot with a few cleanups on top (folded back) for reference: https://tglx.de/~tglx/patches-fsgs.tar Uncompiled and untested. I'm not claiming it's bug free either. If you find one, please keep it. Hope that helps. Thanks, tglx