lkml.org 
[lkml]   [2017]   [Feb]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 20/89] sched/headers, signals: Separate out task_struct::signal and task_struct::sighand types and accessors into <linux/sched/signal.h>

    * Linus Torvalds <torvalds@linux-foundation.org> wrote:

    > On Mon, Feb 6, 2017 at 1:35 PM, Ingo Molnar <mingo@kernel.org> wrote:
    > >
    > >
    > > BTW., most of the real work was in identifying and generating that "noise" - but
    > > to reviewers it's obviously the least interesting bits.
    > >
    > > Also note that beyond the header splitup the "noise" is actually what improves
    > > kernel code the most all around
    >
    > Oh, absolutely agreed. The "noise" is obviously the most important
    > part and the most challenging to actually generate (although maybe
    > some automation tool could do a lot of this in a perfect world).

    BTW., I used half-automation: an (imperfect...) script to recursively follow
    header dependencies to identify the places which I then changed manually, and a
    lot of testing to identify the cases the script missed (or where I made a manual
    mistake).

    I started out with a testing based approach alone, but that did not scale very
    well and I had the feeling that it did not converge fast enough either to lead to
    a palatable end result.

    Didn't want to invest too much into tooling, as I'd expect this to be a one-off
    effort for one of the worst header dependency problems we have in the kernel
    today. Fortunately most of our other headers are in a much better shape!

    > But the noise is also the one where verification is pretty much purely
    > about "does it still compile", so from a human angle, once the noise
    > has been generated, it's not actually all that interesting.

    Yeah, and the repetitive diffs over hundreds of files are actively hindering
    review.

    > It's kind of like being NP complete: verification of the noise is
    > "trivial" and not all that interesting - once the solution has been
    > find.
    >
    > > Will post them in 4 groups of 40 patches each or so - would that work for you?
    >
    > I suspect that will be a lot easier.to look at (with at least one
    > series basically being "there's no point in a human even looking at
    > it, other than to verify that it's purely #include changes").
    >
    > It would be good to make sure it also ends up bisecting nicely,
    > because *if* some problem happens, it would be nice if the bisect then
    > clearly points to either "oh, some mistake must have happened during
    > code movement" vs "ooh, some really subtle issue with a missed include
    > causing some odd fallback code".

    Yeah, so I tested it intensively as I was developing it, so barring
    merge/backmerge/conflict-resolution artifacts that happen during rebase (and they
    did happen ...) there shouldn't be any conceptual bisectability hickups in the
    lineup. It wasn't a 'break it and make it work at the end of the series'
    development flow, it was a 'make it work at every commit' approach.

    I'll do some bisection validation on the final result to make sure bisectability
    did not bitrot during all the restructuring. Problem is, even on a beefy 60-core
    server with 256 GB RAM a full cross-build test runs for half an hour just for
    defconfigs - so a 150-patch series bisectability test would run for over 3 days
    ...

    What I can do is some more careful manual review plus the re-testing of a couple
    of random points in the middle of the series - and of course the testing of the
    final result. This should make it reasonably certain that the good bisectability
    that was validated during earlier versions of the patch-set has a good chance of
    being present in the final series as well.

    > Because we *do* end up having code in C files that does things like
    >
    > #ifndef ARCH_HAS_XYZ
    > static inline void my_generic_xyz_implementation(...)
    > ...
    > #endif
    >
    > so you can end up in the situation that if some specific header file
    > wasn't included correctly, the code will still work, but now it will
    > use the generic definition rather than the specific one it was
    > supposed to use.

    Yeah, indeed.

    > So these re-organizations do have the potential to cause odd "silent"
    > breakage. Most breakage by far should presumably be of the type "it
    > doesn't compile and it's very obvious that the header file movement
    > missed something", but we *could* have that kind of subtle "it
    > compiles, and _almost_ even works, but has a cornercase that is
    > broken" that could be fingered by a bisect.
    >
    > That's when a good split of patches would be really nice to have too,
    > where a patch does either code movement or does header file
    > organization movement, but not both.
    >
    > So it's not _just_ about actually looking at the patches and trying to
    > make sense of them.

    Ok, I'm sold on this! This bisectability argument makes me feel much better about
    the resulting 150+ patch-count.

    Thanks,

    Ingo

    \
     
     \ /
      Last update: 2017-02-06 23:38    [W:6.774 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site