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

[Dan Schatzberg <schatzberg.dan@gmail.com>]

On Mon, Dec 11, 2023 at 05:06:54PM -0800, Chris Li wrote:
> Hi Dan,
> 
> Thank you for the patch.
> 
> On Mon, Dec 11, 2023 at 6:04 AM Dan Schatzberg <schatzberg.dan@gmail.com> 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.

I wrote about this in some detail in the cover letter (0/1). Take a
look and let me know if the rationale is still unclear.

> Instead of passing -1, maybe we can use mem_cgroup_swappiness(memcg);
>

Yeah this makes sense, I'll go ahead and make that change and
eliminate the -1.

> >                                 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.

I used the same logic as in "mm: Add nodes= arg to memory.reclaim". I
can just call it tokens.

> 
> > +       { 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.

See below

> 
> >  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.

As above, "mm: Add nodes= arg to memory.reclaim" was previously in the
mm tree doing it this way, so I duplicated it. I think given that
there have been lots of discussions about extending this interface,
this match table has some potential future value and I don't see a
major downside to using it in favor of strcmp.