Re: [PATCH 1/1] mm: add swapiness= arg to memory.reclaim

From: Huan Yang
Date: Thu Nov 30 2023 - 21:27:45 EST



在 2023/12/1 10:17, Yosry Ahmed 写道:
On Thu, Nov 30, 2023 at 6:14 PM Huan Yang <11133793@xxxxxxxx> wrote:

在 2023/12/1 10:05, Yosry Ahmed 写道:
@@ -2327,7 +2330,8 @@ 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 = mem_cgroup_swappiness(memcg);
+ int swappiness = sc->swappiness ?
+ *sc->swappiness : mem_cgroup_swappiness(memcg);

Should we use "unlikely" here to indicate that sc->swappiness is an unexpected behavior?
Due to current use case only apply in proactive reclaim.
On a system that is not under memory pressure, the rate of proactive
reclaim could be higher than reactive reclaim. We should only use
likely/unlikely when it's obvious a scenario will happen most of the
time. I don't believe that's the case here.
Not all vendors will use proactive interfaces, and reactive reclaim are
a normal
system behavior. In this regard, I think it is appropriate to add
"unlikely".
The general guidance is not to use likely/unlikely when it's not
certain, which I believe is the case here. I think the CPU will make
OK, I will remember this part.
better decisions on its own than if we give it hints that's wrong in
some situations. Others please correct me if I am wrong.
No, you're right. CPU is good to do this.

u64 fraction[ANON_AND_FILE];
u64 denominator = 0; /* gcc */
enum scan_balance scan_balance;
@@ -2608,6 +2612,9 @@ 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;

Also there.

+
return mem_cgroup_swappiness(memcg);
}

@@ -6433,7 +6440,8 @@ unsigned long mem_cgroup_shrink_node(struct mem_cgroup *memcg,
unsigned long try_to_free_mem_cgroup_pages(struct mem_cgroup *memcg,
unsigned long nr_pages,
gfp_t gfp_mask,
- unsigned int reclaim_options)
+ unsigned int reclaim_options,
+ int *swappiness)
{
unsigned long nr_reclaimed;
unsigned int noreclaim_flag;
@@ -6448,6 +6456,7 @@ unsigned long try_to_free_mem_cgroup_pages(struct mem_cgroup *memcg,
.may_unmap = 1,
.may_swap = !!(reclaim_options & MEMCG_RECLAIM_MAY_SWAP),
.proactive = !!(reclaim_options & MEMCG_RECLAIM_PROACTIVE),
+ .swappiness = swappiness,
};
/*
* Traverse the ZONELIST_FALLBACK zonelist of the current node to put
--
2.34.1

My previous patch attempted to ensure fully deterministic semantics under extreme swappiness.
For example, when swappiness is set to 200, only anonymous pages will be reclaimed.
Due to code in MGLRU isolate_folios will try scan anon if no scanned, will try other type.(We do not want
it to attempt this behavior.)
How do you think about extreme swappiness scenarios?
I think having different semantics between swappiness passed to
proactive reclaim and global swappiness can be confusing. If it's
needed to have a swappiness value that says "anon only no matter
what", perhaps we should introduce such a new value and make it
supported by both global and proactive reclaim swappiness? We could
support writing "max" or something similar instead of a special value
to mean that.
Yes, use other hint more suitable for this scenario.

However, from this patch, it seems that this feature is not supported.
Do you have a demand for this scenario?
We do anonymous-only proactive reclaim in some setups, so it would be
nice to have. I am not sure if it's absolutely needed vs. just using
swappiness=200 and living with the possibility of reclaiming some file
pages.
Right now, the scenario where swappiness=200 is sufficient for us, but having the
tendency to only reclaim anonymous pages has a clear semantics that is
suitable for upper-level strategy scenarios, rather than relying solely on
the functionality of swappiness.