On Thu, Dec 21, 2023 at 10:29:59AM +0100, Michal Hocko wrote: > On Wed 20-12-23 07:26:51, Dan Schatzberg wrote: > > ... > > LGTM > Acked-by: Michal Hocko <mhocko@xxxxxxxx. > > Just one minor thing. It would be really great to prevent from potential > incorrect use of mem_cgroup_swappiness. This should be internal function > to memcg. Now, having scan_control internal to vmscan.c makes that > harder and moving it out to swap.h or internal.h sounds overreaching. > > We could do this at least to reduce those mistakes. I can make it a > proper patch if this seems reasonable or you can fold it into your patch > directly. > --- > > diff --git a/mm/vmscan.c b/mm/vmscan.c > index f98dff23b758..5f3a182e9515 100644 > --- a/mm/vmscan.c > +++ b/mm/vmscan.c > @@ -92,8 +92,10 @@ struct scan_control { > unsigned long anon_cost; > unsigned long file_cost; > > - /* Swappiness value for reclaim. NULL will fall back to per-memcg/global value */ > +#ifdef CONFIG_MEMCG > + /* Swappiness value for reclaim. Always use sc_swappiness()! */ > int *swappiness; > +#endif > > /* Can active folios be deactivated as part of reclaim? */ > #define DEACTIVATE_ANON 1 > @@ -230,6 +232,13 @@ static bool writeback_throttling_sane(struct scan_control *sc) > #endif > return false; > } > + > +static int sc_swappiness(struct scan_control *sc, struct mem_cgroup *memcg) > +{ > + if (sc->swappiness) > + return *sc->swappiness; > + return mem_cgroup_swappiness(memcg); > +} > #else > static bool cgroup_reclaim(struct scan_control *sc) > { > @@ -245,6 +254,10 @@ static bool writeback_throttling_sane(struct scan_control *sc) > { > return true; > } > +static int sc_swappiness(struct scan_control *sc, struct mem_cgroup *memcg) > +{ > + return READ_ONCE(vm_swappiness); > +} > #endif > > static void set_task_reclaim_state(struct task_struct *task, > @@ -2330,8 +2343,7 @@ static void get_scan_count(struct lruvec *lruvec, struct scan_control *sc, > struct pglist_data *pgdat = lruvec_pgdat(lruvec); > struct mem_cgroup *memcg = lruvec_memcg(lruvec); > unsigned long anon_cost, file_cost, total_cost; > - int swappiness = sc->swappiness ? > - *sc->swappiness : mem_cgroup_swappiness(memcg); > + int swappiness = sc_swappiness(sc, memcg); > u64 fraction[ANON_AND_FILE]; > u64 denominator = 0; /* gcc */ > enum scan_balance scan_balance; > @@ -2612,10 +2624,7 @@ static int get_swappiness(struct lruvec *lruvec, struct scan_control *sc) > mem_cgroup_get_nr_swap_pages(memcg) < MIN_LRU_BATCH) > return 0; > > - if (sc->swappiness) > - return *sc->swappiness; > - > - return mem_cgroup_swappiness(memcg); > + return sc_swappiness(sc, memcg); > } > > static int get_nr_gens(struct lruvec *lruvec, int type) > -- > Michal Hocko > SUSE Labs Thanks for the review Michal and sorry for the delayed response. Your patch looks reasonable to me but I'm a bit unclear about the need for #ifdef - mem_cgroup_swappiness already works correctly regardless of CONFIG_MEMCG or not - why not make sc->swappiness and sc_swappiness() unconditional? Happy to roll that into the next version of my patch.