Re: [PATCH -next] mm/vmscan: fix an undefined behavior for zone id

[Michal Hocko]

On Tue 12-11-19 13:20:17, Johannes Weiner wrote:
> On Tue, Nov 12, 2019 at 05:31:56PM +0100, Michal Hocko wrote:
> > On Tue 12-11-19 11:16:58, Johannes Weiner wrote:
> > > On Tue, Nov 12, 2019 at 04:27:50PM +0100, Michal Hocko wrote:
> > > > lruvec_lru_size is explicitly documented to use MAX_NR_ZONES for all
> > > > LRUs and git grep says there are more instances outside of
> > > > get_scan_count. So all of them have to be fixed.
> > > 
> > > Which ones?
> > > 
> > > [hannes@computer linux]$ git grep lruvec_lru_size
> > > include/linux/mmzone.h:extern unsigned long lruvec_lru_size(struct lruvec *lruvec, enum lru_list lru, int zone_idx);
> > > mm/vmscan.c: * lruvec_lru_size -  Returns the number of pages on the given LRU list.
> > > mm/vmscan.c:unsigned long lruvec_lru_size(struct lruvec *lruvec, enum lru_list lru, int zone_idx)
> > > mm/vmscan.c:    anon  = lruvec_lru_size(lruvec, LRU_ACTIVE_ANON, MAX_NR_ZONES - 1) +
> > > mm/vmscan.c:            lruvec_lru_size(lruvec, LRU_INACTIVE_ANON, MAX_NR_ZONES - 1);
> > > mm/vmscan.c:    file  = lruvec_lru_size(lruvec, LRU_ACTIVE_FILE, MAX_NR_ZONES - 1) +
> > > mm/vmscan.c:            lruvec_lru_size(lruvec, LRU_INACTIVE_FILE, MAX_NR_ZONES - 1);
> > > mm/vmscan.c:            lruvec_size = lruvec_lru_size(lruvec, lru, sc->reclaim_idx);
> > > [hannes@computer linux]$
> > 
> > I have checked the Linus tree but now double checked with the current
> > next
> > $ git describe next/master
> > next-20191112
> > $ git grep "lruvec_lru_size.*MAX_NR_ZONES" next/master
> > next/master:mm/vmscan.c:                        lruvec_lru_size(lruvec, inactive_lru, MAX_NR_ZONES), inactive,
> > next/master:mm/vmscan.c:                        lruvec_lru_size(lruvec, active_lru, MAX_NR_ZONES), active,
> > next/master:mm/vmscan.c:        anon  = lruvec_lru_size(lruvec, LRU_ACTIVE_ANON, MAX_NR_ZONES) +
> > next/master:mm/vmscan.c:                lruvec_lru_size(lruvec, LRU_INACTIVE_ANON, MAX_NR_ZONES);
> > next/master:mm/vmscan.c:        file  = lruvec_lru_size(lruvec, LRU_ACTIVE_FILE, MAX_NR_ZONES) +
> > next/master:mm/vmscan.c:                lruvec_lru_size(lruvec, LRU_INACTIVE_FILE, MAX_NR_ZONES);
> > next/master:mm/workingset.c:    active_file = lruvec_lru_size(lruvec, LRU_ACTIVE_FILE, MAX_NR_ZONES);
> > 
> > are there any changes which didn't make it to linux next yet?
> 
> Aaahh, that makes sense. I was looking at the latest mmots which
> has
> 
> - mm: vmscan: detect file thrashing at the reclaim root
> - mm: vmscan: enforce inactive:active ratio at the reclaim root
> 
> Those replace the inactive_is_low and the workingset callsites with
> the recursive lruvec_page_state(). It looks like that isn't in next -
> and while I hope it'll make it into 5.5, it might not. So we need a
> fix that considers the other callsites as well.
> 
> Qian's patches that Andrew already has will be good then, as it
> reduces the churn to those other callsites that are in flux.
> 
> We can clean things up when the dust settles.

Yeah, while I am not really super happy about the code that is more
complex than necessary the clean up can be done on top and we can also
think about how to do it properly (I still haven't given up ;))

-- 
Michal Hocko
SUSE Labs