linux-kernel - Re: [PATCH 00/13] mm: clean up some lru related pieces

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 for Android: free password hash cracker in your pocket

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

Date:   Fri, 18 Sep 2020 03:36:31 -0600
From:   Yu Zhao <yuzhao@...gle.com>
To:     Michal Hocko <mhocko@...e.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Alex Shi <alex.shi@...ux.alibaba.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ingo Molnar <mingo@...hat.com>,
        Johannes Weiner <hannes@...xchg.org>,
        Vladimir Davydov <vdavydov.dev@...il.com>,
        Roman Gushchin <guro@...com>,
        Shakeel Butt <shakeelb@...gle.com>,
        Chris Down <chris@...isdown.name>,
        Yafang Shao <laoar.shao@...il.com>,
        Vlastimil Babka <vbabka@...e.cz>,
        Huang Ying <ying.huang@...el.com>,
        Pankaj Gupta <pankaj.gupta.linux@...il.com>,
        Matthew Wilcox <willy@...radead.org>,
        Konstantin Khlebnikov <koct9i@...il.com>,
        Minchan Kim <minchan@...nel.org>,
        Jaewon Kim <jaewon31.kim@...sung.com>, cgroups@...r.kernel.org,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 00/13] mm: clean up some lru related pieces

On Fri, Sep 18, 2020 at 09:45:49AM +0200, Michal Hocko wrote:
> On Thu 17-09-20 21:00:38, Yu Zhao wrote:
> > Hi Andrew,
> > 
> > I see you have taken this:
> >   mm: use add_page_to_lru_list()/page_lru()/page_off_lru()
> > Do you mind dropping it?
> > 
> > Michal asked to do a bit of additional work. So I thought I probably
> > should create a series to do more cleanups I've been meaning to.
> > 
> > This series contains the change in the patch above and goes a few
> > more steps farther. It's intended to improve readability and should
> > not have any performance impacts. There are minor behavior changes in
> > terms of debugging and error reporting, which I have all highlighted
> > in the individual patches. All patches were properly tested on 5.8
> > running Chrome OS, with various debug options turned on.
> > 
> > Michal,
> > 
> > Do you mind taking a looking at the entire series?
> 
> I have stopped at patch 3 as all patches until then are really missing
> any justification. What is the point for all this to be done? The code
> is far from trivial and just shifting around sounds like a risk. You are

I appreciate your caution, and if you let me know what exactly your
concerns are, we could probably work them out together.

> removing ~50 LOC which is always nice but I am not sure the resulting
> code is better maintainble or easier to read and understand. Just
> consider __ClearPageLRU moving to page_off_lru patch. What is the
> additional value of having the flag moved and burry it into a function
> to have even more side effects? I found the way how __ClearPageLRU is

Mind elaborating the side effects?

> nicely close to removing it from LRU easier to follow. This is likely
> subjective and other might think differently but as it is not clear what
> is your actual goal here it is hard to judge pros and cons.

I like this specific example from patch 3. Here is what it does: we
have three places using the same boilerplate, i.e., page_off_lru() +
__ClearPageLRU(), the patch moves __ClearPageLRU() into page_off_lru(),
which already does __ClearPageActive() and __ClearPageUnevictable().
Later on, we rename page_off_lru() to __clear_page_lru_flags() (patch
8).

Its point seems quite clear to me. Why would *anybody* want to use
two helper functions *repeatedly* when the job can be done with just
one? Nobody is paid by the number of lines they add, right? :) And
for that matter, why would anybody want any boilerplate to be open
coded from the same group of helper functions arranged in various
ways? I don't think the answer is subjective, but I don't expect
everybody to agree with me.

Now back to your general question: what's the point of this series?
Readability -- less error prone and easier to maintain. This series
consolidate open-coded boilerplate like the following in many places.
Take lru_lazyfree_fn() as an example:

-		bool active = PageActive(page);
 		int nr_pages = thp_nr_pages(page);

-		del_page_from_lru_list(page, lruvec,
-				       LRU_INACTIVE_ANON + active);
+		del_page_from_lru_list(page, lruvec);
 		ClearPageActive(page);
 		ClearPageReferenced(page);
		<snipped>
 		ClearPageSwapBacked(page);
-		add_page_to_lru_list(page, lruvec, LRU_INACTIVE_FILE);
+		add_page_to_lru_list(page, lruvec);

I hope this helps, but if it doesn't, I'd be more than happy to have
more discussions on the details. And not that I don't appreciate your
review, but please be more specific than 'sounds like a risk' or 'have
even more side effects' so I can address your concerns effectively.