On Thu, Dec 14, 2023 at 10:22 AM Dan Schatzberg <schatzberg.dan@xxxxxxxxx> wrote: > > On Thu, Dec 14, 2023 at 09:38:55AM +0100, Michal Hocko wrote: > > On Tue 12-12-23 17:38:03, Dan Schatzberg wrote: > > > Allow proactive reclaimers to submit an additional swappiness=<val> > > > argument to memory.reclaim. This overrides the global or per-memcg > > > swappiness setting for that reclaim attempt. > > > > You are providing the usecase in the cover letter and Andrew usually > > appends that to the first patch in the series. I think it would be > > better to have the usecase described here. > > > > [...] > > > @@ -1304,6 +1297,18 @@ PAGE_SIZE multiple when read back. > > > This means that the networking layer will not adapt based on > > > reclaim induced by memory.reclaim. > > > > > > +The following nested keys are defined. > > > + > > > + ========== ================================ > > > + swappiness Swappiness value to reclaim with > > > + ========== ================================ > > > + > > > + Specifying a swappiness value instructs the kernel to perform > > > + the reclaim with that swappiness value. Note that this has the > > > + same semantics as the vm.swappiness sysctl - it sets the > > > > same semantics as vm.swappiness applied to memcg reclaim with all the > > existing limitations and potential future extensions. > > Thanks, will make this change. > > > > > > + relative IO cost of reclaiming anon vs file memory but does > > > + not allow for reclaiming specific amounts of anon or file memory. > > > + > > > memory.peak > > > A read-only single value file which exists on non-root > > > cgroups. > > > > The biggest problem with the implementation I can see, and others have > > pointed out the same, is how fragile this is. You really have to check > > the code and _every_ place which defines scan_control to learn that > > mem_cgroup_shrink_node, reclaim_clean_pages_from_list, > > reclaim_folio_list, lru_gen_seq_write, try_to_free_pages, balance_pgdat, > > shrink_all_memory and __node_reclaim. I have only checked couple of > > them, like direct reclaim and kswapd and none of them really sets up > > swappiness to the default memcg nor global value. So you effectively end > > up with swappiness == 0. > > > > While the review can point those out it is quite easy to break and you > > will only learn about that very indirectly. I think it would be easier > > to review and maintain if you go with a pointer that would fallback to > > mem_cgroup_swappiness() if NULL which will be the case for every > > existing reclaimer except memory.reclaim with swappiness value. > > I agree. My initial implementation used a pointer for this > reason. I'll switch this back. Just to be clear - I still need to > initialize scan_control.swappiness in all these other places right? It > appears to mostly be stack-initialized which means it has > indeterminate value, not necessarily zero. My understanding is that in a partially initialized struct, uninitialized members default to 0, but I am not a C expert :)
