Message-Id: <20220411191559.a844c7140faeba2e68d842b7@linux-foundation.org>
Date:   Mon, 11 Apr 2022 19:15:59 -0700
From:   Andrew Morton <akpm@...ux-foundation.org>
To:     Yu Zhao <yuzhao@...gle.com>
Cc:     Stephen Rothwell <sfr@...hwell.id.au>, linux-mm@...ck.org,
        Andi Kleen <ak@...ux.intel.com>,
        Aneesh Kumar <aneesh.kumar@...ux.ibm.com>,
        Barry Song <21cnbao@...il.com>,
        Catalin Marinas <catalin.marinas@....com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Hillf Danton <hdanton@...a.com>, Jens Axboe <axboe@...nel.dk>,
        Jesse Barnes <jsbarnes@...gle.com>,
        Johannes Weiner <hannes@...xchg.org>,
        Jonathan Corbet <corbet@....net>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Matthew Wilcox <willy@...radead.org>,
        Mel Gorman <mgorman@...e.de>,
        Michael Larabel <Michael@...haellarabel.com>,
        Michal Hocko <mhocko@...nel.org>,
        Mike Rapoport <rppt@...nel.org>,
        Rik van Riel <riel@...riel.com>,
        Vlastimil Babka <vbabka@...e.cz>,
        Will Deacon <will@...nel.org>,
        Ying Huang <ying.huang@...el.com>,
        linux-arm-kernel@...ts.infradead.org, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org, page-reclaim@...gle.com,
        x86@...nel.org
Subject: Re: [PATCH v10 00/14] Multi-Gen LRU Framework


It's pretty clear from the results and from the test coverage to date
that Linux wants this.

I do think additional review is needed.  For the usual code quality
reasons, but also to help others get up to speed with the proposed
changes and to ensure that we have something which is well maintainable
going forward.

The code at present is unnecessarily difficult to review and that
review will be less effective than it might be, because of its lack of
commentary.

I'll merge the v10 series into -mm and -next for additional runtime
exposure, but I'll be asking for a broad update.

The data structures appear to be adequately documented (this is rare)
but the code itself is lacking.  Please go through each and every new
function and ask "is this function so self-evident that it can be
presented to a newcomer with no explanatory comments at all".

If "yes" then check again.  If still "yes" then fine.  If "no" then
let's craft comments which tell the reader things which are not evident
from the code itself.  Things which are helpful to that reader.  Design
concepts, side-effects, preconditions, unobvious traps, etc.

Please ensure that the current group of mglru developers review these
comment additions as closely as they review code changes.

Alas, it's late in the process to be adding these things.  But it can
be made better.

I'll make some high-level comments on the patches themselves now, but I
see little point in attempting detailed review when a better-explained
version is hopefully forthcoming.


A few gratuitous notebook entries:

- lru_gen_struct.timestamps works in jiffies?  Why?  jiffies can vary
  based on platform and config - why add inter-platform and
  inter-Kconfig variability like this?  Time is measured in seconds!

  And the amount of work which can be performed in one second varies
  by, guess, 100x on machines which we care about.  This also has
  potential to introduce tremendous inter-platform variability in the
  behaviour of this feature.

- did lru_gen_use_mm() really need to test nodes_full()?  Is that
  likely enough to be a net benefit?

- seq_is_valid() sounds like it belongs to the seq_file() subsystem

- does get_nr_evictable() need to return and use signed types (long)?
  It sums the contents of lru_gen_struct.nr_pages[][][], which is
  ulong, so I think "no".

Powered by blists - more mailing lists