[PATCH 1/2] mm/memcontrol: return 1 from cgroup.memory __setup() handler

[PATCH 1/2] mm/memcontrol: return 1 from cgroup.memory __setup() handler

[Randy Dunlap]

__setup() handlers should return 1 if the command line option is handled
and 0 if not (or maybe never return 0; it just pollutes init's environment).

The only reason that this particular __setup handler does not pollute
init's environment is that the setup string contains a '.', as in
"cgroup.memory". This causes init/main.c::unknown_boottoption() to
consider it to be an "Unused module parameter" and ignore it. (This is
for parsing of loadable module parameters any time after kernel init.)
Otherwise the string "cgroup.memory=whatever" would be added to init's
environment strings.

Instead of relying on this '.' quirk, just return 1 to indicate that
the boot option has been handled.

Note that there is no warning message if someone enters:
	cgroup.memory=anything_invalid

Fixes: f7e1cb6ec51b0 ("mm: memcontrol: account socket memory in unified hierarchy memory controller")
Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
Reported-by: Igor Zhbanov <i.zhbanov@omprussia.ru>
Link: lore.kernel.org/r/64644a2f-4a20-bab3-1e15-3b2cdd0defe3@omprussia.ru
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
Cc: cgroups@vger.kernel.org
---
 mm/memcontrol.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- linux-next-20220217.orig/mm/memcontrol.c
+++ linux-next-20220217/mm/memcontrol.c
@@ -7044,7 +7044,7 @@ static int __init cgroup_memory(char *s)
 		if (!strcmp(token, "nokmem"))
 			cgroup_memory_nokmem = true;
 	}
-	return 0;
+	return 1;
 }
 __setup("cgroup.memory=", cgroup_memory);
 

[PATCH 1/2] mm/memcontrol: return 1 from cgroup.memory __setup() handler

[Randy Dunlap]

__setup() handlers should return 1 if the command line option is handled
and 0 if not (or maybe never return 0; it just pollutes init's environment).

The only reason that this particular __setup handler does not pollute
init's environment is that the setup string contains a '.', as in
"cgroup.memory". This causes init/main.c::unknown_boottoption() to
consider it to be an "Unused module parameter" and ignore it. (This is
for parsing of loadable module parameters any time after kernel init.)
Otherwise the string "cgroup.memory=whatever" would be added to init's
environment strings.

Instead of relying on this '.' quirk, just return 1 to indicate that
the boot option has been handled.

Note that there is no warning message if someone enters:
	cgroup.memory=anything_invalid

Fixes: f7e1cb6ec51b0 ("mm: memcontrol: account socket memory in unified hierarchy memory controller")
Signed-off-by: Randy Dunlap <rdunlap-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
Reported-by: Igor Zhbanov <i.zhbanov-4G3FwjhnV40EXkLx5dDeUA@public.gmane.org>
Link: lore.kernel.org/r/64644a2f-4a20-bab3-1e15-3b2cdd0defe3-4G3FwjhnV40EXkLx5dDeUA@public.gmane.org
Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
Cc: Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org>
Cc: Michal Hocko <mhocko-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Vladimir Davydov <vdavydov.dev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
---
 mm/memcontrol.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- linux-next-20220217.orig/mm/memcontrol.c
+++ linux-next-20220217/mm/memcontrol.c
@@ -7044,7 +7044,7 @@ static int __init cgroup_memory(char *s)
 		if (!strcmp(token, "nokmem"))
 			cgroup_memory_nokmem = true;
 	}
-	return 0;
+	return 1;
 }
 __setup("cgroup.memory=", cgroup_memory);
 

Re: [PATCH 1/2] mm/memcontrol: return 1 from cgroup.memory __setup() handler

[Michal Koutný]

On Mon, Feb 21, 2022 at 04:58:11PM -0800, Randy Dunlap <rdunlap@infradead.org> wrote:
> __setup() handlers should return 1 if the command line option is handled
> and 0 if not (or maybe never return 0; it just pollutes init's environment).

Interesting.

> Instead of relying on this '.' quirk, just return 1 to indicate that
> the boot option has been handled.

But your patch would return 1 even when no accepted value was passed,
i.e. is the command line option considered handled in that case?

Did you want to return 1 only when the cgroup.memory= value is
recognized?

Thanks,
Michal

Re: [PATCH 1/2] mm/memcontrol: return 1 from cgroup.memory __setup() handler

[Michal Koutný]

On Mon, Feb 21, 2022 at 04:58:11PM -0800, Randy Dunlap <rdunlap-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> wrote:
> __setup() handlers should return 1 if the command line option is handled
> and 0 if not (or maybe never return 0; it just pollutes init's environment).

Interesting.

> Instead of relying on this '.' quirk, just return 1 to indicate that
> the boot option has been handled.

But your patch would return 1 even when no accepted value was passed,
i.e. is the command line option considered handled in that case?

Did you want to return 1 only when the cgroup.memory= value is
recognized?

Thanks,
Michal

Re: [PATCH 1/2] mm/memcontrol: return 1 from cgroup.memory __setup() handler

[Randy Dunlap]

On 3/2/22 10:53, Michal Koutný wrote:
> On Mon, Feb 21, 2022 at 04:58:11PM -0800, Randy Dunlap <rdunlap@infradead.org> wrote:
>> __setup() handlers should return 1 if the command line option is handled
>> and 0 if not (or maybe never return 0; it just pollutes init's environment).
> 
> Interesting.
> 
>> Instead of relying on this '.' quirk, just return 1 to indicate that
>> the boot option has been handled.
> 
> But your patch would return 1 even when no accepted value was passed,
> i.e. is the command line option considered handled in that case?

Yes, for some definition of "handled."  It was seen by the __setup handler.

> Did you want to return 1 only when the cgroup.memory= value is
> recognized?

Not really. I did consider that (for all of the similar patches that I am
posting).

I don't think those strings (even with invalid option values) should be
added to init's environment.

I'm willing to add a pr_warn() or pr_notice() for any unrecognized
option value, but it should still return 1 IMO.

-- 
~Randy

Re: [PATCH 1/2] mm/memcontrol: return 1 from cgroup.memory __setup() handler

[Randy Dunlap]

On 3/2/22 10:53, Michal Koutný wrote:
> On Mon, Feb 21, 2022 at 04:58:11PM -0800, Randy Dunlap <rdunlap-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> wrote:
>> __setup() handlers should return 1 if the command line option is handled
>> and 0 if not (or maybe never return 0; it just pollutes init's environment).
> 
> Interesting.
> 
>> Instead of relying on this '.' quirk, just return 1 to indicate that
>> the boot option has been handled.
> 
> But your patch would return 1 even when no accepted value was passed,
> i.e. is the command line option considered handled in that case?

Yes, for some definition of "handled."  It was seen by the __setup handler.

> Did you want to return 1 only when the cgroup.memory= value is
> recognized?

Not really. I did consider that (for all of the similar patches that I am
posting).

I don't think those strings (even with invalid option values) should be
added to init's environment.

I'm willing to add a pr_warn() or pr_notice() for any unrecognized
option value, but it should still return 1 IMO.

-- 
~Randy

Re: [PATCH 1/2] mm/memcontrol: return 1 from cgroup.memory __setup() handler

[Michal Koutný]

On Wed, Mar 02, 2022 at 04:53:19PM -0800, Randy Dunlap <rdunlap@infradead.org> wrote:
> I don't think those strings (even with invalid option values) should be
> added to init's environment.

Isn't mere presence of the handler sufficient to filter those out? [1]

(Counter-example would be 'foo=1 foo=2' where 1 is accepted value by the
handler, 2 is unrecognized and should be passed to init. Is this a real
use case?)

> I'm willing to add a pr_warn() or pr_notice() for any unrecognized
> option value, but it should still return 1 IMO.

Regardless of the handler existence check, I see returning 1 would be
consistent with the majority of other memcg handlers.

For the uniformity,
Reviewed-by: Michal Koutný <mkoutny@suse.com>

(Richer reporting or -EINVAL is by my understanding now a different
problem.)

Thanks,
Michal

Re: [PATCH 1/2] mm/memcontrol: return 1 from cgroup.memory __setup() handler

[Michal Koutný]

On Wed, Mar 02, 2022 at 04:53:19PM -0800, Randy Dunlap <rdunlap-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> wrote:
> I don't think those strings (even with invalid option values) should be
> added to init's environment.

Isn't mere presence of the handler sufficient to filter those out? [1]

(Counter-example would be 'foo=1 foo=2' where 1 is accepted value by the
handler, 2 is unrecognized and should be passed to init. Is this a real
use case?)

> I'm willing to add a pr_warn() or pr_notice() for any unrecognized
> option value, but it should still return 1 IMO.

Regardless of the handler existence check, I see returning 1 would be
consistent with the majority of other memcg handlers.

For the uniformity,
Reviewed-by: Michal Koutný <mkoutny-IBi9RG/b67k@public.gmane.org>

(Richer reporting or -EINVAL is by my understanding now a different
problem.)

Thanks,
Michal

Re: [PATCH 1/2] mm/memcontrol: return 1 from cgroup.memory __setup() handler

[Randy Dunlap]

Hi Michal,

On 3/3/22 02:14, Michal Koutný wrote:
> On Wed, Mar 02, 2022 at 04:53:19PM -0800, Randy Dunlap <rdunlap@infradead.org> wrote:
>> I don't think those strings (even with invalid option values) should be
>> added to init's environment.
> 
> Isn't mere presence of the handler sufficient to filter those out? [1]

What is [1] here?

> (Counter-example would be 'foo=1 foo=2' where 1 is accepted value by the
> handler, 2 is unrecognized and should be passed to init. Is this a real
> use case?)

I don't know of any case where "foo=2" should be passed to init if
there is a setup function for "foo=" defined.

>> I'm willing to add a pr_warn() or pr_notice() for any unrecognized
>> option value, but it should still return 1 IMO.
> 
> Regardless of the handler existence check, I see returning 1 would be
> consistent with the majority of other memcg handlers.
> 
> For the uniformity,
> Reviewed-by: Michal Koutný <mkoutny@suse.com>
> 
> (Richer reporting or -EINVAL is by my understanding now a different
> problem.)

-- 
~Randy

Re: [PATCH 1/2] mm/memcontrol: return 1 from cgroup.memory __setup() handler

[Randy Dunlap]

Hi Michal,

On 3/3/22 02:14, Michal Koutný wrote:
> On Wed, Mar 02, 2022 at 04:53:19PM -0800, Randy Dunlap <rdunlap-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> wrote:
>> I don't think those strings (even with invalid option values) should be
>> added to init's environment.
> 
> Isn't mere presence of the handler sufficient to filter those out? [1]

What is [1] here?

> (Counter-example would be 'foo=1 foo=2' where 1 is accepted value by the
> handler, 2 is unrecognized and should be passed to init. Is this a real
> use case?)

I don't know of any case where "foo=2" should be passed to init if
there is a setup function for "foo=" defined.

>> I'm willing to add a pr_warn() or pr_notice() for any unrecognized
>> option value, but it should still return 1 IMO.
> 
> Regardless of the handler existence check, I see returning 1 would be
> consistent with the majority of other memcg handlers.
> 
> For the uniformity,
> Reviewed-by: Michal Koutný <mkoutny-IBi9RG/b67k@public.gmane.org>
> 
> (Richer reporting or -EINVAL is by my understanding now a different
> problem.)

-- 
~Randy

Re: [PATCH 1/2] mm/memcontrol: return 1 from cgroup.memory __setup() handler

[Michal Koutný]

On Thu, Mar 03, 2022 at 01:53:03PM -0800, Randy Dunlap <rdunlap@infradead.org> wrote:
> > Isn't mere presence of the handler sufficient to filter those out? [1]
> 
> What is [1] here?

Please ignore, too much editing on my side.

> I don't know of any case where "foo=2" should be passed to init if
> there is a setup function for "foo=" defined.

Good. I was asking because of the following semantics:
- absent handler -- pass to init,
- returns 0 -- filter out,
- returns negative -- filter out, print message.

> > (Richer reporting or -EINVAL is by my understanding now a different
> > problem.)

Regards,
Michal

Re: [PATCH 1/2] mm/memcontrol: return 1 from cgroup.memory __setup() handler

[Michal Koutný]

On Thu, Mar 03, 2022 at 01:53:03PM -0800, Randy Dunlap <rdunlap-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> wrote:
> > Isn't mere presence of the handler sufficient to filter those out? [1]
> 
> What is [1] here?

Please ignore, too much editing on my side.

> I don't know of any case where "foo=2" should be passed to init if
> there is a setup function for "foo=" defined.

Good. I was asking because of the following semantics:
- absent handler -- pass to init,
- returns 0 -- filter out,
- returns negative -- filter out, print message.

> > (Richer reporting or -EINVAL is by my understanding now a different
> > problem.)

Regards,
Michal

Re: [PATCH 1/2] mm/memcontrol: return 1 from cgroup.memory __setup() handler

[Randy Dunlap]

On 3/3/22 14:32, Michal Koutný wrote:
> On Thu, Mar 03, 2022 at 01:53:03PM -0800, Randy Dunlap <rdunlap@infradead.org> wrote:
>>> Isn't mere presence of the handler sufficient to filter those out? [1]
>>
>> What is [1] here?
> 
> Please ignore, too much editing on my side.
> 
>> I don't know of any case where "foo=2" should be passed to init if
>> there is a setup function for "foo=" defined.
> 
> Good. I was asking because of the following semantics:
> - absent handler -- pass to init,

Ack: if the handler code is not built, it is an Unknown boot option
and is passed to init.

> - returns 0 -- filter out,
> - returns negative -- filter out, print message.

Currently setup functions should return 1 (or any non-zero value)
to indicate "handled" or should return 0 to indicate "not handled".

Andrew has a patch in mmotm: include/linux/init.h so that the comment
before __setup() says:

/*
 * NOTE: __setup functions return values:
 * @fn returns 1 (or non-zero) if the option argument is "handled"
 * and returns 0 if the option argument is "not handled".
 */

>>> (Richer reporting or -EINVAL is by my understanding now a different
>>> problem.)

-- 
~Randy

Re: [PATCH 1/2] mm/memcontrol: return 1 from cgroup.memory __setup() handler

[Randy Dunlap]

On 3/3/22 14:32, Michal Koutný wrote:
> On Thu, Mar 03, 2022 at 01:53:03PM -0800, Randy Dunlap <rdunlap-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> wrote:
>>> Isn't mere presence of the handler sufficient to filter those out? [1]
>>
>> What is [1] here?
> 
> Please ignore, too much editing on my side.
> 
>> I don't know of any case where "foo=2" should be passed to init if
>> there is a setup function for "foo=" defined.
> 
> Good. I was asking because of the following semantics:
> - absent handler -- pass to init,

Ack: if the handler code is not built, it is an Unknown boot option
and is passed to init.

> - returns 0 -- filter out,
> - returns negative -- filter out, print message.

Currently setup functions should return 1 (or any non-zero value)
to indicate "handled" or should return 0 to indicate "not handled".

Andrew has a patch in mmotm: include/linux/init.h so that the comment
before __setup() says:

/*
 * NOTE: __setup functions return values:
 * @fn returns 1 (or non-zero) if the option argument is "handled"
 * and returns 0 if the option argument is "not handled".
 */

>>> (Richer reporting or -EINVAL is by my understanding now a different
>>> problem.)

-- 
~Randy