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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <49178f48-6635-353c-678d-3db436d3f9c3@linux.intel.com>
Date:   Mon, 9 Jul 2018 10:19:25 -0700
From:   Dave Hansen <dave.hansen@...ux.intel.com>
To:     "Huang, Ying" <ying.huang@...el.com>,
        Andrew Morton <akpm@...ux-foundation.org>
Cc:     linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Michal Hocko <mhocko@...e.com>,
        Johannes Weiner <hannes@...xchg.org>,
        Shaohua Li <shli@...nel.org>, Hugh Dickins <hughd@...gle.com>,
        Minchan Kim <minchan@...nel.org>,
        Rik van Riel <riel@...hat.com>,
        Naoya Horiguchi <n-horiguchi@...jp.nec.com>,
        Zi Yan <zi.yan@...rutgers.edu>,
        Daniel Jordan <daniel.m.jordan@...cle.com>
Subject: Re: [PATCH -mm -v4 05/21] mm, THP, swap: Support PMD swap mapping in
 free_swap_and_cache()/swap_free()

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.