mm/memcontrol.c: fix another unused function warning
diff mbox series

Message ID 20191001142227.1227176-1-arnd@arndb.de
State New
Headers show
Series
  • mm/memcontrol.c: fix another unused function warning
Related show

Commit Message

Arnd Bergmann Oct. 1, 2019, 2:22 p.m. UTC
Removing the mem_cgroup_id_get() stub function introduced a new warning
of the same kind when CONFIG_MMU is disabled:

mm/memcontrol.c:4929:13: error: unused function 'mem_cgroup_id_get_many' [-Werror,-Wunused-function]

Address this using a __maybe_unused annotation.

Note: alternatively, this could be moved into an #ifdef block.  Marking it
'static inline' would not work here as that would still produce the
warning on clang, which only ignores unused inline functions declared
in header files instead of .c files.

Fixes: 4d0e3230a56a ("mm/memcontrol.c: fix a -Wunused-function warning")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 mm/memcontrol.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Qian Cai Oct. 1, 2019, 2:40 p.m. UTC | #1
On Tue, 2019-10-01 at 16:22 +0200, Arnd Bergmann wrote:
> Removing the mem_cgroup_id_get() stub function introduced a new warning
> of the same kind when CONFIG_MMU is disabled:

Shouldn't CONFIG_MEMCG depends on CONFIG_MMU instead?

> 
> mm/memcontrol.c:4929:13: error: unused function 'mem_cgroup_id_get_many' [-Werror,-Wunused-function]
> 
> Address this using a __maybe_unused annotation.
> 
> Note: alternatively, this could be moved into an #ifdef block.  Marking it
> 'static inline' would not work here as that would still produce the
> warning on clang, which only ignores unused inline functions declared
> in header files instead of .c files.
> 
> Fixes: 4d0e3230a56a ("mm/memcontrol.c: fix a -Wunused-function warning")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  mm/memcontrol.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index c313c49074ca..5f9f90e3cef8 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -4921,7 +4921,8 @@ static void mem_cgroup_id_remove(struct mem_cgroup *memcg)
>  	}
>  }
>  
> -static void mem_cgroup_id_get_many(struct mem_cgroup *memcg, unsigned int n)
> +static void __maybe_unused
> +mem_cgroup_id_get_many(struct mem_cgroup *memcg, unsigned int n)
>  {
>  	refcount_add(n, &memcg->id.ref);
>  }
Arnd Bergmann Oct. 1, 2019, 4 p.m. UTC | #2
On Tue, Oct 1, 2019 at 4:40 PM Qian Cai <cai@lca.pw> wrote:
>
> On Tue, 2019-10-01 at 16:22 +0200, Arnd Bergmann wrote:
> > Removing the mem_cgroup_id_get() stub function introduced a new warning
> > of the same kind when CONFIG_MMU is disabled:
>
> Shouldn't CONFIG_MEMCG depends on CONFIG_MMU instead?

Maybe. Generally we allow building a lot of stuff without CONFIG_MMU that
may not make sense, so I just followed the same idea here.

      Arnd
Nick Desaulniers Oct. 1, 2019, 4:36 p.m. UTC | #3
On Tue, Oct 1, 2019 at 7:22 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> Removing the mem_cgroup_id_get() stub function introduced a new warning
> of the same kind when CONFIG_MMU is disabled:
>
> mm/memcontrol.c:4929:13: error: unused function 'mem_cgroup_id_get_many' [-Werror,-Wunused-function]
>
> Address this using a __maybe_unused annotation.
>
> Note: alternatively, this could be moved into an #ifdef block.  Marking it

Hi Arnd,
Thank you for the patch!  I would prefer to move the definition to the
correct set of #ifdef guards rather than __maybe_unused.  Maybe move
the definition of mem_cgroup_id_get_many() to just before
__mem_cgroup_clear_mc()?  I find __maybe_unused to be a code smell.

> 'static inline' would not work here as that would still produce the
> warning on clang, which only ignores unused inline functions declared
> in header files instead of .c files.
>
> Fixes: 4d0e3230a56a ("mm/memcontrol.c: fix a -Wunused-function warning")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  mm/memcontrol.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index c313c49074ca..5f9f90e3cef8 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -4921,7 +4921,8 @@ static void mem_cgroup_id_remove(struct mem_cgroup *memcg)
>         }
>  }
>
> -static void mem_cgroup_id_get_many(struct mem_cgroup *memcg, unsigned int n)
> +static void __maybe_unused
> +mem_cgroup_id_get_many(struct mem_cgroup *memcg, unsigned int n)
>  {
>         refcount_add(n, &memcg->id.ref);
>  }
> --
Qian Cai Oct. 1, 2019, 4:44 p.m. UTC | #4
On Tue, 2019-10-01 at 18:00 +0200, Arnd Bergmann wrote:
> On Tue, Oct 1, 2019 at 4:40 PM Qian Cai <cai@lca.pw> wrote:
> > 
> > On Tue, 2019-10-01 at 16:22 +0200, Arnd Bergmann wrote:
> > > Removing the mem_cgroup_id_get() stub function introduced a new warning
> > > of the same kind when CONFIG_MMU is disabled:
> > 
> > Shouldn't CONFIG_MEMCG depends on CONFIG_MMU instead?
> 
> Maybe. Generally we allow building a lot of stuff without CONFIG_MMU that
> may not make sense, so I just followed the same idea here.

Those blindly mark __maybe_unused might just mask important warnings off in the
future, and they are ugly. Let's fix it properly.
Michal Hocko Oct. 2, 2019, 7:53 a.m. UTC | #5
On Tue 01-10-19 10:40:05, Qian Cai wrote:
> On Tue, 2019-10-01 at 16:22 +0200, Arnd Bergmann wrote:
> > Removing the mem_cgroup_id_get() stub function introduced a new warning
> > of the same kind when CONFIG_MMU is disabled:
> 
> Shouldn't CONFIG_MEMCG depends on CONFIG_MMU instead?

I wanted to do that in the past but there was a clear disagreement from
Johannes.
Michal Hocko Oct. 2, 2019, 7:54 a.m. UTC | #6
On Tue 01-10-19 09:36:24, Nick Desaulniers wrote:
> On Tue, Oct 1, 2019 at 7:22 AM Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > Removing the mem_cgroup_id_get() stub function introduced a new warning
> > of the same kind when CONFIG_MMU is disabled:
> >
> > mm/memcontrol.c:4929:13: error: unused function 'mem_cgroup_id_get_many' [-Werror,-Wunused-function]
> >
> > Address this using a __maybe_unused annotation.
> >
> > Note: alternatively, this could be moved into an #ifdef block.  Marking it
> 
> Hi Arnd,
> Thank you for the patch!  I would prefer to move the definition to the
> correct set of #ifdef guards rather than __maybe_unused.  Maybe move
> the definition of mem_cgroup_id_get_many() to just before
> __mem_cgroup_clear_mc()?  I find __maybe_unused to be a code smell.

Agreed!

Patch
diff mbox series

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index c313c49074ca..5f9f90e3cef8 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -4921,7 +4921,8 @@  static void mem_cgroup_id_remove(struct mem_cgroup *memcg)
 	}
 }
 
-static void mem_cgroup_id_get_many(struct mem_cgroup *memcg, unsigned int n)
+static void __maybe_unused
+mem_cgroup_id_get_many(struct mem_cgroup *memcg, unsigned int n)
 {
 	refcount_add(n, &memcg->id.ref);
 }