Hi Dan,
Thank you for the patch.
On Mon, Dec 11, 2023 at 6:04 AM Dan Schatzberg <schatzberg.dan@xxxxxxxxx> 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.
I am curious what prompted you to develop this patch. I understand
what this patch does, just want to know more of your background story
why this is needed.
>
> For example:
>
> echo "2M swappiness=0" > /sys/fs/cgroup/memory.reclaim
>
> will perform reclaim on the rootcg with a swappiness setting of 0 (no
> swap) regardless of the vm.swappiness sysctl setting.
>
> Signed-off-by: Dan Schatzberg <schatzberg.dan@xxxxxxxxx>
> ---
> Documentation/admin-guide/cgroup-v2.rst | 15 ++++++-
> include/linux/swap.h | 3 +-
> mm/memcontrol.c | 55 ++++++++++++++++++++-----
> mm/vmscan.c | 13 +++++-
> 4 files changed, 70 insertions(+), 16 deletions(-)
>
> diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
> index 3f85254f3cef..fc2b379dbd0f 100644
> --- a/Documentation/admin-guide/cgroup-v2.rst
> +++ b/Documentation/admin-guide/cgroup-v2.rst
> @@ -1282,8 +1282,8 @@ PAGE_SIZE multiple when read back.
> This is a simple interface to trigger memory reclaim in the
> target cgroup.
>
> - This file accepts a single key, the number of bytes to reclaim.
> - No nested keys are currently supported.
> + This file accepts a string which containers thhe number of bytes
Just as Yosry points out, there is some typo here.
"contains the"
> + to reclaim.
>
> Example::
>
> @@ -1304,6 +1304,17 @@ PAGE_SIZE multiple when read back.
> This means that the networking layer will not adapt based on
> reclaim induced by memory.reclaim.
>
> + This file also allows the user to specify the swappiness value
> + to be used for the reclaim. For example:
> +
> + echo "1G swappiness=60" > memory.reclaim
> +
> + The above instructs the kernel to perform the reclaim with
> + a swappiness value of 60. Note that this has the same semantics
> + as the vm.swappiness sysctl - it sets the 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.
> diff --git a/include/linux/swap.h b/include/linux/swap.h
> index f6dd6575b905..ebc20d094609 100644
> --- a/include/linux/swap.h
> +++ b/include/linux/swap.h
> @@ -410,7 +410,8 @@ extern unsigned long try_to_free_pages(struct zonelist *zonelist, int order,
> extern 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);
> extern unsigned long mem_cgroup_shrink_node(struct mem_cgroup *mem,
> gfp_t gfp_mask, bool noswap,
> pg_data_t *pgdat,
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 1c1061df9cd1..74598c17d3cc 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -63,6 +63,7 @@
> #include <linux/resume_user_mode.h>
> #include <linux/psi.h>
> #include <linux/seq_buf.h>
> +#include <linux/parser.h>
> #include <linux/sched/isolation.h>
> #include "internal.h"
> #include <net/sock.h>
> @@ -2449,7 +2450,7 @@ static unsigned long reclaim_high(struct mem_cgroup *memcg,
> psi_memstall_enter(&pflags);
> nr_reclaimed += try_to_free_mem_cgroup_pages(memcg, nr_pages,
> gfp_mask,
> - MEMCG_RECLAIM_MAY_SWAP);
> + MEMCG_RECLAIM_MAY_SWAP, -1);
Instead of passing -1, maybe we can use mem_cgroup_swappiness(memcg);
And maybe remove the -1 test from the get_scan_count().
""
static void get_scan_count(struct lruvec *lruvec, struct scan_control *sc,
unsigned long *nr)
{
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 != -1 ?
sc->swappiness : mem_cgroup_swappiness(memcg);
""
In other words, I feel it is cleaner if try_to_free_mem_cgroup_pages
accept the swappiness as it is. There is no second guessing the value
if it is -1, then the internal function gets the swappiness from
mem_cgroup_swappiness().
Maybe we can completely remove the -1 special default value?
> psi_memstall_leave(&pflags);
> } while ((memcg = parent_mem_cgroup(memcg)) &&
> !mem_cgroup_is_root(memcg));
> @@ -2740,7 +2741,7 @@ static int try_charge_memcg(struct mem_cgroup *memcg, gfp_t gfp_mask,
>
> psi_memstall_enter(&pflags);
> nr_reclaimed = try_to_free_mem_cgroup_pages(mem_over_limit, nr_pages,
> - gfp_mask, reclaim_options);
> + gfp_mask, reclaim_options, -1);
Same here.
> psi_memstall_leave(&pflags);
>
> if (mem_cgroup_margin(mem_over_limit) >= nr_pages)
> @@ -3660,7 +3661,7 @@ static int mem_cgroup_resize_max(struct mem_cgroup *memcg,
> }
>
> if (!try_to_free_mem_cgroup_pages(memcg, 1, GFP_KERNEL,
> - memsw ? 0 : MEMCG_RECLAIM_MAY_SWAP)) {
> + memsw ? 0 : MEMCG_RECLAIM_MAY_SWAP, -1)) {
Same here.
> ret = -EBUSY;
> break;
> }
> @@ -3774,7 +3775,7 @@ static int mem_cgroup_force_empty(struct mem_cgroup *memcg)
> return -EINTR;
>
> if (!try_to_free_mem_cgroup_pages(memcg, 1, GFP_KERNEL,
> - MEMCG_RECLAIM_MAY_SWAP))
> + MEMCG_RECLAIM_MAY_SWAP, -1))
Here too.
> nr_retries--;
> }
>
> @@ -6720,7 +6721,7 @@ static ssize_t memory_high_write(struct kernfs_open_file *of,
> }
>
> reclaimed = try_to_free_mem_cgroup_pages(memcg, nr_pages - high,
> - GFP_KERNEL, MEMCG_RECLAIM_MAY_SWAP);
> + GFP_KERNEL, MEMCG_RECLAIM_MAY_SWAP, -1);
Here too.
>
> if (!reclaimed && !nr_retries--)
> break;
> @@ -6769,7 +6770,7 @@ static ssize_t memory_max_write(struct kernfs_open_file *of,
>
> if (nr_reclaims) {
> if (!try_to_free_mem_cgroup_pages(memcg, nr_pages - max,
> - GFP_KERNEL, MEMCG_RECLAIM_MAY_SWAP))
> + GFP_KERNEL, MEMCG_RECLAIM_MAY_SWAP, -1))
Here too.
> nr_reclaims--;
> continue;
> }
> @@ -6895,6 +6896,16 @@ static ssize_t memory_oom_group_write(struct kernfs_open_file *of,
> return nbytes;
> }
>
> +enum {
> + MEMORY_RECLAIM_SWAPPINESS = 0,
> + MEMORY_RECLAIM_NULL,
> +};
> +
> +static const match_table_t if_tokens = {
What this is called "if_tokens"? I am trying to figure out what "if" refers to.
> + { MEMORY_RECLAIM_SWAPPINESS, "swappiness=%d"},
> + { MEMORY_RECLAIM_NULL, NULL },
> +};
> +
Do we foresee a lot of tunable for the try to free page? I see. You
want to use match_token() to do the keyword parsing.
> static ssize_t memory_reclaim(struct kernfs_open_file *of, char *buf,
> size_t nbytes, loff_t off)
> {
> @@ -6902,12 +6913,33 @@ static ssize_t memory_reclaim(struct kernfs_open_file *of, char *buf,
> unsigned int nr_retries = MAX_RECLAIM_RETRIES;
> unsigned long nr_to_reclaim, nr_reclaimed = 0;
> unsigned int reclaim_options;
> - int err;
> + char *old_buf, *start;
> + substring_t args[MAX_OPT_ARGS];
> + int swappiness = -1;
>
> buf = strstrip(buf);
> - err = page_counter_memparse(buf, "", &nr_to_reclaim);
> - if (err)
> - return err;
> +
> + old_buf = buf;
> + nr_to_reclaim = memparse(buf, &buf) / PAGE_SIZE;
> + if (buf == old_buf)
> + return -EINVAL;
> +
> + buf = strstrip(buf);
> +
> + while ((start = strsep(&buf, " ")) != NULL) {
> + if (!strlen(start))
> + continue;
> + switch (match_token(start, if_tokens, args)) {
> + case MEMORY_RECLAIM_SWAPPINESS:
> + if (match_int(&args[0], &swappiness))
> + return -EINVAL;
> + if (swappiness < 0 || swappiness > 200)
Agree with Yosry on the 200 magic value.
I am also wondering if there is an easier way to just parse one
keyword. Will using strcmp("swappiness=") be a bad idea? I haven't
tried it myself though.
> + return -EINVAL;
> + break;
> + default:
> + return -EINVAL;
> + }
> + }
>
> reclaim_options = MEMCG_RECLAIM_MAY_SWAP | MEMCG_RECLAIM_PROACTIVE;
> while (nr_reclaimed < nr_to_reclaim) {
> @@ -6926,7 +6958,8 @@ static ssize_t memory_reclaim(struct kernfs_open_file *of, char *buf,
>
> reclaimed = try_to_free_mem_cgroup_pages(memcg,
> min(nr_to_reclaim - nr_reclaimed, SWAP_CLUSTER_MAX),
> - GFP_KERNEL, reclaim_options);
> + GFP_KERNEL, reclaim_options,
> + swappiness);
>
> if (!reclaimed && !nr_retries--)
> return -EAGAIN;
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 506f8220c5fe..a20965b20177 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -136,6 +136,9 @@ struct scan_control {
> /* Always discard instead of demoting to lower tier memory */
> unsigned int no_demotion:1;
>
> + /* Swappiness value for reclaim, if -1 use memcg/global value */
> + int swappiness;
> +
It would be great if we can get rid of the -1 case.
> /* Allocation order */
> s8 order;
>
> @@ -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 != -1 ?
> + sc->swappiness : mem_cgroup_swappiness(memcg);
Let's see if we can get rid of the -1. Now I see the -1 is actually
introduced by this patch, should be doable.
> 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 != -1)
> + return sc->swappiness;
> +
Get rid of that too.
Thank you.
Chris
> 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
>
>
