[PATCH] mm: memcontrol.c: move mem_cgroup_id_get_many under CONFIG_MMU

[PATCH] mm: memcontrol.c: move mem_cgroup_id_get_many under CONFIG_MMU

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

mem_cgroup_id_get_many() is used under CONFIG_MMU.
This patch moves it to under CONFIG_MMU.
We will get below warning without this patch
if .config doesn't have CONFIG_MMU.

	LINUX/mm/memcontrol.c:4814:13: warning: 'mem_cgroup_id_get_many'\
		defined but not used [-Wunused-function]
	static void mem_cgroup_id_get_many(struct mem_cgroup *memcg, unsigned int n)
	^~~~~~~~~~~~~~~~~~~~~~

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 mm/memcontrol.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index c5b5f74..8a157ef 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -4811,11 +4811,6 @@ static void mem_cgroup_id_remove(struct mem_cgroup *memcg)
 	}
 }
 
-static void mem_cgroup_id_get_many(struct mem_cgroup *memcg, unsigned int n)
-{
-	refcount_add(n, &memcg->id.ref);
-}
-
 static void mem_cgroup_id_put_many(struct mem_cgroup *memcg, unsigned int n)
 {
 	if (refcount_sub_and_test(n, &memcg->id.ref)) {
@@ -5153,6 +5148,11 @@ static void mem_cgroup_css_reset(struct cgroup_subsys_state *css)
 }
 
 #ifdef CONFIG_MMU
+static void mem_cgroup_id_get_many(struct mem_cgroup *memcg, unsigned int n)
+{
+	refcount_add(n, &memcg->id.ref);
+}
+
 /* Handlers for move charge at task migration. */
 static int mem_cgroup_do_precharge(unsigned long count)
 {
-- 
2.7.4

Re: [PATCH] mm: memcontrol.c: move mem_cgroup_id_get_many under CONFIG_MMU

[Michal Hocko]

On Tue 17-12-19 15:47:40, Kuninori Morimoto wrote:
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> mem_cgroup_id_get_many() is used under CONFIG_MMU.

Not really. It is used when SWAP is enabled currently. But it is not
really bound to the swap functionality by any means. It just happens
that we do not have other users currently. We might put it under
CONFIG_SWAP but I do not really think it is a big improvement.

> This patch moves it to under CONFIG_MMU.
> We will get below warning without this patch
> if .config doesn't have CONFIG_MMU.
> 
> 	LINUX/mm/memcontrol.c:4814:13: warning: 'mem_cgroup_id_get_many'\
> 		defined but not used [-Wunused-function]
> 	static void mem_cgroup_id_get_many(struct mem_cgroup *memcg, unsigned int n)
> 	^~~~~~~~~~~~~~~~~~~~~~

Is this warning really a big deal? The function is not used, alright,
and the compiler will likely just drop it.

> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>  mm/memcontrol.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index c5b5f74..8a157ef 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -4811,11 +4811,6 @@ static void mem_cgroup_id_remove(struct mem_cgroup *memcg)
>  	}
>  }
>  
> -static void mem_cgroup_id_get_many(struct mem_cgroup *memcg, unsigned int n)
> -{
> -	refcount_add(n, &memcg->id.ref);
> -}
> -
>  static void mem_cgroup_id_put_many(struct mem_cgroup *memcg, unsigned int n)
>  {
>  	if (refcount_sub_and_test(n, &memcg->id.ref)) {
> @@ -5153,6 +5148,11 @@ static void mem_cgroup_css_reset(struct cgroup_subsys_state *css)
>  }
>  
>  #ifdef CONFIG_MMU
> +static void mem_cgroup_id_get_many(struct mem_cgroup *memcg, unsigned int n)
> +{
> +	refcount_add(n, &memcg->id.ref);
> +}
> +
>  /* Handlers for move charge at task migration. */
>  static int mem_cgroup_do_precharge(unsigned long count)
>  {
> -- 
> 2.7.4

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: memcontrol.c: move mem_cgroup_id_get_many under CONFIG_MMU

[Chris Down]

Hi Kuninori,

Michal Hocko writes:
>On Tue 17-12-19 15:47:40, Kuninori Morimoto wrote:
>> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>>
>> mem_cgroup_id_get_many() is used under CONFIG_MMU.
>
>Not really. It is used when SWAP is enabled currently. But it is not
>really bound to the swap functionality by any means. It just happens
>that we do not have other users currently. We might put it under
>CONFIG_SWAP but I do not really think it is a big improvement.

Agreed, I think we shouldn't wrap this in preprocessor conditionals it since 
it's entirely possible it will end up used elsewhere and we'll end up with a 
mess of #ifdefs.

>> This patch moves it to under CONFIG_MMU.
>> We will get below warning without this patch
>> if .config doesn't have CONFIG_MMU.
>>
>> 	LINUX/mm/memcontrol.c:4814:13: warning: 'mem_cgroup_id_get_many'\
>> 		defined but not used [-Wunused-function]
>> 	static void mem_cgroup_id_get_many(struct mem_cgroup *memcg, unsigned int n)
>> 	^~~~~~~~~~~~~~~~~~~~~~
>
>Is this warning really a big deal? The function is not used, alright,
>and the compiler will likely just drop it.

Let's just add __maybe_unused, since it seems like what we want in this 
scenario -- it avoids new users having to enter preprocessor madness, while 
also not polluting the build output.

Once you've done that, I'll send over my ack. :-)

Thanks.

Re: [PATCH] mm: memcontrol.c: move mem_cgroup_id_get_many under CONFIG_MMU

[Qian Cai]

> On Dec 17, 2019, at 8:54 AM, Chris Down <chris@chrisdown.name> wrote:
> 
> Let's just add __maybe_unused, since it seems like what we want in this scenario -- it avoids new users having to enter preprocessor madness, while also not polluting the build output.

__maybe_unused should only be used in the last resort as it mark the compiler to catch the real issues in the future. In this case, it might be better just ignore it as only non-realistic compiling test would use !CONFIG_MMU in this case.

Re: [PATCH] mm: memcontrol.c: move mem_cgroup_id_get_many under CONFIG_MMU

[Chris Down]

Qian Cai writes:
>__maybe_unused should only be used in the last resort as it mark the compiler 
>to catch the real issues in the future. In this case, it might be better just 
>ignore it as only non-realistic compiling test would use !CONFIG_MMU in this 
>case.

While that's true, I'd rather not end up with getting more patches based on 
tests like these. On balance the risk of adding __maybe_unused here with a note 
to remove it later seems better than having to reply to every patch removing 
warnings :-)

I struggle to imagine a real issue this would catch that wouldn't already be 
caught by other means. If it's just the risks of dead code, that seems equally 
risky as taking time away from reviewers.

We should probably also review the coding style doc again, since this looks 
suspect:

>If you have a function or variable which may potentially go unused in a
>particular configuration, and the compiler would warn about its definition
>going unused, mark the definition as __maybe_unused rather than wrapping it in
>a preprocessor conditional.  (However, if a function or variable *always* goes
>unused, delete it.)

Re: [PATCH] mm: memcontrol.c: move mem_cgroup_id_get_many under CONFIG_MMU

[Michal Hocko]

On Tue 17-12-19 09:16:36, Qian Cai wrote:
> 
> 
> > On Dec 17, 2019, at 8:54 AM, Chris Down <chris@chrisdown.name> wrote:
> > 
> > Let's just add __maybe_unused, since it seems like what we want in this scenario -- it avoids new users having to enter preprocessor madness, while also not polluting the build output.
> 
> __maybe_unused should only be used in the last resort as it mark the compiler to catch the real issues in the future. In this case, it might be better just ignore it as only non-realistic compiling test would use !CONFIG_MMU in this case.

yes, I would just ignore this warning. Btw. it seems that this is
enabled by default for -Wall. Is this useful for kernel builds at
all? Does it realistically help discovering real issues? If not then
can we simply blacklist it?
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: memcontrol.c: move mem_cgroup_id_get_many under CONFIG_MMU

[Qian Cai]

> On Dec 17, 2019, at 9:46 AM, Michal Hocko <mhocko@kernel.org> wrote:
> 
> yes, I would just ignore this warning. Btw. it seems that this is
> enabled by default for -Wall. Is this useful for kernel builds at
> all? Does it realistically help discovering real issues? If not then
> can we simply blacklist it?

-Wunused-function is useful in-general as it caught many dead code that some commits left unintentionally with real-world configs.

Re: [PATCH] mm: memcontrol.c: move mem_cgroup_id_get_many under CONFIG_MMU

[Chris Down]

Michal Hocko writes:
>yes, I would just ignore this warning. Btw. it seems that this is
>enabled by default for -Wall. Is this useful for kernel builds at
>all? Does it realistically help discovering real issues? If not then
>can we simply blacklist it?

There's no way we're the first people to encounter these problems, so what did 
we do in the past when situations like this (adding a generic API which is not 
yet used by non-configurable code) came up, and in retrospect did they work 
well?

As far as I know -Wunused-function also guards against other errors, like when 
a function is prototyped but not actually defined, which might be more useful 
to know about.

(Side note: I'm moderately baffled that a tightly scoped __maybe_unused is 
considered sinister but somehow disabling -Wunused-function is on the table 
:-))

Re: [PATCH] mm: memcontrol.c: move mem_cgroup_id_get_many under CONFIG_MMU

[Qian Cai]

> On Dec 17, 2019, at 9:37 AM, Chris Down <chris@chrisdown.name> wrote:
> 
> I struggle to imagine a real issue this would catch that wouldn't already be caught by other means. If it's just the risks of dead code, that seems equally risky as taking time away from reviewers.

Hate to say this but ignore unrealistic configs compiling test should not take that much time.

Re: [PATCH] mm: memcontrol.c: move mem_cgroup_id_get_many under CONFIG_MMU

[Michal Hocko]

On Tue 17-12-19 10:04:36, Qian Cai wrote:
> 
> 
> > On Dec 17, 2019, at 9:46 AM, Michal Hocko <mhocko@kernel.org> wrote:
> > 
> > yes, I would just ignore this warning. Btw. it seems that this is
> > enabled by default for -Wall. Is this useful for kernel builds at
> > all? Does it realistically help discovering real issues? If not then
> > can we simply blacklist it?
> 
> -Wunused-function is useful in-general as it caught many dead code
> that some commits left unintentionally with real-world configs.

I do understand the general purpose of the warning. I am simply not sure
the kernel tree is a good candidate with a huge number of different
config combinations that might easily result in warnings which would
tend to result in even more ifdeferry than we have.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: memcontrol.c: move mem_cgroup_id_get_many under CONFIG_MMU

[Qian Cai]

> On Dec 17, 2019, at 10:13 AM, Michal Hocko <mhocko@kernel.org> wrote:
> 
> I do understand the general purpose of the warning. I am simply not sure
> the kernel tree is a good candidate with a huge number of different
> config combinations that might easily result in warnings which would
> tend to result in even more ifdeferry than we have.

Yes, compiling test without real-world use case is evil. Once we ignore the evil, it becomes much more manageable.

Re: [PATCH] mm: memcontrol.c: move mem_cgroup_id_get_many under CONFIG_MMU

[Michal Hocko]

On Tue 17-12-19 15:09:21, Chris Down wrote:
[...]
> (Side note: I'm moderately baffled that a tightly scoped __maybe_unused is
> considered sinister but somehow disabling -Wunused-function is on the table
> :-))

Well, I usually do not like to see __maybe_unused because that is prone
to bit-rot and loses its usefulness. Looking into the recent git logs
most -Wunused-function led to the code removal (which is really good
but the compiler is likely to do that already so the overall impact is
not that large) or more ifdefery. I do not really see many instance of
__maybe_unused.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: memcontrol.c: move mem_cgroup_id_get_many under CONFIG_MMU

[Chris Down]

Michal Hocko writes:
>On Tue 17-12-19 15:09:21, Chris Down wrote:
>[...]
>> (Side note: I'm moderately baffled that a tightly scoped __maybe_unused is
>> considered sinister but somehow disabling -Wunused-function is on the table
>> :-))
>
>Well, I usually do not like to see __maybe_unused because that is prone
>to bit-rot and loses its usefulness. Looking into the recent git logs
>most -Wunused-function led to the code removal (which is really good
>but the compiler is likely to do that already so the overall impact is
>not that large) or more ifdefery. I do not really see many instance of
>__maybe_unused.

Hmm, but __maybe_unused is easy to find and document the reasons behind nearby, 
and then reevaluate at some later time. On the other hand, it's much *harder* 
to reevaluate which functions actually are unused in the long term if we remove 
-Wunused-function, because enabling it to find candidates will result in an 
incredibly amount of noise from those who have missed unused functions 
previously due to the lack of the warning.

Maybe Qian is right and we should just ignore such patches, but I think that 
comes with its own risks that we will alienate perfectly well intentioned new 
contributors to mm without them having any idea why we did that.

Re: [PATCH] mm: memcontrol.c: move mem_cgroup_id_get_many under CONFIG_MMU

[Chris Down]

Chris Down writes:
>Maybe Qian is right and we should just ignore such patches, but I 
>think that comes with its own risks that we will alienate perfectly 
>well intentioned new contributors to mm without them having any idea 
>why we did that.

s/Qian/Cai/ :-)

Re: [PATCH] mm: memcontrol.c: move mem_cgroup_id_get_many under CONFIG_MMU

[Qian Cai]

> On Dec 17, 2019, at 10:28 AM, Chris Down <chris@chrisdown.name> wrote:
> 
> Maybe Qian is right and we should just ignore such patches, but I think that comes with its own risks that we will alienate perfectly well intentioned new contributors to mm without them having any idea why we did that.

Yes, that is a good point, but in reality is that there are many subsystems have already done the same. We even have some famous introduction document for kernel development put in the way that if “the maintainers (or Linus) ignored your patches after the resend, they probably don’t like it.”

Re: [PATCH] mm: memcontrol.c: move mem_cgroup_id_get_many under CONFIG_MMU

[Michal Hocko]

On Tue 17-12-19 15:28:14, Chris Down wrote:
> Michal Hocko writes:
> > On Tue 17-12-19 15:09:21, Chris Down wrote:
> > [...]
> > > (Side note: I'm moderately baffled that a tightly scoped __maybe_unused is
> > > considered sinister but somehow disabling -Wunused-function is on the table
> > > :-))
> > 
> > Well, I usually do not like to see __maybe_unused because that is prone
> > to bit-rot and loses its usefulness. Looking into the recent git logs
> > most -Wunused-function led to the code removal (which is really good
> > but the compiler is likely to do that already so the overall impact is
> > not that large) or more ifdefery. I do not really see many instance of
> > __maybe_unused.
> 
> Hmm, but __maybe_unused is easy to find and document the reasons behind
> nearby, and then reevaluate at some later time. On the other hand, it's much
> *harder* to reevaluate which functions actually are unused in the long term
> if we remove -Wunused-function, because enabling it to find candidates will
> result in an incredibly amount of noise from those who have missed unused
> functions previously due to the lack of the warning.

I usually git grep for the function and that covers many cases. But
realistically, I am more than skeptical people are going to do a regular
cleanup like that. And that is the biggest deal with this annotation.
Once it gets marked it will just stay that way and potentially get
really unused eventually. So the overall benefit is close the zero in
that case.

Maybe dropping -Wunused-function is an overreaction. Git log shows there
has been some code removed which is probably the most viable reaction to
those reports. Maybe we just want to add those for W=1 or something like
that.

> Maybe Qian is right and we should just ignore such patches, but I think that
> comes with its own risks that we will alienate perfectly well intentioned
> new contributors to mm without them having any idea why we did that.

I believe that both possitive and negative reaction to _any_ patch has
to be properly justified - same applies to the patch itself. A warning
report/fix is not an expcetion. In this particular case it has been pointed
out that the reported function is a general purpose one which just
happens to be used only for CONFIG_SWAP (rather than CONFIG_MMU) and
using additional ifdeferry is likely not going to help long term.
-- 
Michal Hocko
SUSE Labs