lkml.org 
[lkml]   [2018]   [Jul]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH -mm -v4 05/21] mm, THP, swap: Support PMD swap mapping in free_swap_and_cache()/swap_free()
    Date
    Dave Hansen <dave.hansen@linux.intel.com> writes:

    > I'm seeing a pattern here.
    >
    > old code:
    >
    > foo()
    > {
    > do_swap_something()
    > }
    >
    > new code:
    >
    > foo(bool cluster)
    > {
    > if (cluster)
    > do_swap_cluster_something();
    > else
    > do_swap_something();
    > }
    >
    > That make me fear that we have:
    > 1. Created a new, wholly untested code path
    > 2. Created two places to patch bugs
    > 3. Are not reusing code when possible
    >
    > The code non-resuse was, and continues to be, IMNHO, one of the largest
    > sources of bugs with the original THP implementation. It might be
    > infeasible to do here, but let's at least give it as much of a go as we can.

    I totally agree that we should unify the code path for huge and normal
    page/swap if possible. One concern is code size for !CONFIG_THP_SWAP.
    The original method is good for that. The new method may introduce some
    huge swap related code that is hard to be eliminated for
    !CONFIG_THP_SWAP. Andrew Morton pointed this out for the patchset of
    the first step of the THP swap optimization.

    This may be mitigated at least partly via,

    `
    #ifdef CONFIG_THP_SWAP
    #define nr_swap_entries(nr) (nr)
    #else
    #define nr_swap_entries(nr) 1
    #endif

    void do_something(swp_entry_t entry, int __nr_entries)
    {
    int i, nr_entries = nr_swap_entries(__nr_entries);

    if (nr_entries = SWAPFILE_CLUSTER)
    ; /* huge swap specific */
    else
    ; /* normal swap specific */

    for (i = 0; i < nr_entries; i++) {
    ; /* do something for each entry */
    }

    /* ... */
    }
    `

    and rely on compiler to do the dirty work for us if possible.

    Hi, Andrew,

    What do you think about this?

    > Can I ask that you take another round through this set and:
    >
    > 1. Consolidate code refactoring into separate patches

    Sure.

    > 2. Add comments to code, and avoid doing it solely in changelogs

    Sure.

    > 3. Make an effort to share more code between the old code and new
    > code. Where code can not be shared, call that out in the changelog.

    Will do that if we resolve the code size concern.

    > This is a *really* hard-to-review set at the moment. Doing those things
    > will make it much easier to review and hopefully give us more
    > maintainable code going forward.
    >
    > My apologies for not having done this review sooner.

    Thanks a lot for your comments!

    Best Regards,
    Huang, Ying

    \
     
     \ /
      Last update: 2018-07-10 09:14    [W:4.335 / U:0.028 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site