Re: [PATCH -mm -v4 05/21] mm, THP, swap: Support PMD swap mapping in free_swap_and_cache()/swap_free()

Dave Hansen
2018-07-09T17:19:35Z

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.

Can I ask that you take another round through this set and:

1. Consolidate code refactoring into separate patches
2. Add comments to code, and avoid doing it solely in changelogs
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.

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.