Hi Dan,
On Tue, Dec 12, 2023 at 1:27 PM Dan Schatzberg <schatzberg.dan@xxxxxxxxx> wrote:
>
> > > + 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)
> >
> > I am not a fan of extending the hardcoded 0 and 200 values, and now
> > the new -1 value. Maybe it's time to create constants for the min and
> > max swappiness values instead of hardcoding them everywhere? This can
> > be a separate preparatory patch. Then, -1 (or any invalid value) can
> > also be added as a constant with a useful name, instead of passing -1
> > to all other callers.
> >
> > This should make the code a little bit more readable and easier to extend.
>
> I'm not sure I understand the concern. This check just validates that
> the swappiness value inputted is between 0 and 200 (inclusive)
> otherwise the interface returns -EINVAL. Are you just concerned that
> these constants are not named explicitly so they can be reused
> elsewhere in the code?
>
I think the concern is why 200? Why not 400 or 600?
The user might write bigger values and expect the reclaim to work with
those values.
If there is some hard coded limit enforced somewhere else so writing
more than 200 does not make sense. It would be nice to have those
other places reference this limit as well. Thus give 200 a name and
use it in other places of the code as well.
Chris
