Hi Dan,
On Tue, Dec 12, 2023 at 1:43 PM Dan Schatzberg <schatzberg.dan@xxxxxxxxx> wrote:
>
> > 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.
Ah, found it. I was not CC on the cover letter but CC on the 1/1
patch. That is why I did not pick up the cover letter.
Yes, the cover letter explanation was great. Exactly what I am looking for.
>
> > 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.
Thanks
>
> > > 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.
Thanks. I will take a look at that change.
> > > +
> > > + 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.
Yes, that is totally your call. I am fine as it is. Just the micro
optimization of me trying to see if there is a slimmer way to do it.
Chris
