| Message ID | 20220222005811.10672-1-rdunlap@infradead.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] mm/memcontrol: return 1 from cgroup.memory __setup() handler | expand |
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
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.
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
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.)
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
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.)
--- 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);
__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(-)