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