Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1645948 > unrolled thread

[PATCH 1/3] mm/slub: Only define kmalloc_large_node_hook() for NUMA systems

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/3] mm/slub: Only define kmalloc_large_node_hook() for NUMA systems Matthias Kaehlcke <mka@chromium.org> - 2017-05-19 23:10 +0200
    Re: [PATCH 1/3] mm/slub: Only define kmalloc_large_node_hook() for  NUMA systems David Rientjes <rientjes@google.com> - 2017-05-22 22:40 +0200
      Re: [PATCH 1/3] mm/slub: Only define kmalloc_large_node_hook() for  NUMA systems Matthias Kaehlcke <mka@chromium.org> - 2017-05-22 23:00 +0200
        Re: [PATCH 1/3] mm/slub: Only define kmalloc_large_node_hook() for  NUMA systems Andrew Morton <akpm@linux-foundation.org> - 2017-05-22 23:50 +0200
          Re: [PATCH 1/3] mm/slub: Only define kmalloc_large_node_hook() for  NUMA systems David Rientjes <rientjes@google.com> - 2017-05-23 03:40 +0200
            Re: [PATCH 1/3] mm/slub: Only define kmalloc_large_node_hook() for  NUMA systems Matthias Kaehlcke <mka@chromium.org> - 2017-05-23 19:00 +0200
              Re: [PATCH 1/3] mm/slub: Only define kmalloc_large_node_hook() for  NUMA systems Doug Anderson <dianders@chromium.org> - 2017-05-23 19:20 +0200
              Re: [PATCH 1/3] mm/slub: Only define kmalloc_large_node_hook() for  NUMA systems David Rientjes <rientjes@google.com> - 2017-05-24 22:40 +0200
                Re: [PATCH 1/3] mm/slub: Only define kmalloc_large_node_hook() for  NUMA systems Matthias Kaehlcke <mka@chromium.org> - 2017-05-25 00:20 +0200
                  Re: [PATCH 1/3] mm/slub: Only define kmalloc_large_node_hook() for  NUMA systems Doug Anderson <dianders@chromium.org> - 2017-05-26 19:10 +0200

#1645948 — [PATCH 1/3] mm/slub: Only define kmalloc_large_node_hook() for NUMA systems

The function is only used when CONFIG_NUMA=y. Placing it in an #ifdef
block fixes the following warning when building with clang:

mm/slub.c:1246:20: error: unused function 'kmalloc_large_node_hook'
    [-Werror,-Wunused-function]

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
 mm/slub.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/mm/slub.c b/mm/slub.c
index 57e5156f02be..66e1046435b7 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1313,11 +1313,14 @@ static inline void dec_slabs_node(struct kmem_cache *s, int node,
  * Hooks for other subsystems that check memory allocations. In a typical
  * production configuration these hooks all should produce no code at all.
  */
+
+#ifdef CONFIG_NUMA
 static inline void kmalloc_large_node_hook(void *ptr, size_t size, gfp_t flags)
 {
 	kmemleak_alloc(ptr, size, 1, flags);
 	kasan_kmalloc_large(ptr, size, flags);
 }
+#endif
 
 static inline void kfree_hook(const void *x)
 {
-- 
2.13.0.303.g4ebf302169-goog

[toc] | [next] | [standalone]

#1647352 — Re: [PATCH 1/3] mm/slub: Only define kmalloc_large_node_hook() for NUMA systems

On Fri, 19 May 2017, Matthias Kaehlcke wrote:

> The function is only used when CONFIG_NUMA=y. Placing it in an #ifdef
> block fixes the following warning when building with clang:
> 
> mm/slub.c:1246:20: error: unused function 'kmalloc_large_node_hook'
>     [-Werror,-Wunused-function]
> 

Is clang not inlining kmalloc_large_node_hook() for some reason?  I don't 
think this should ever warn on gcc.

> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>

Acked-by: David Rientjes <rientjes@google.com>

> ---
>  mm/slub.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index 57e5156f02be..66e1046435b7 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1313,11 +1313,14 @@ static inline void dec_slabs_node(struct kmem_cache *s, int node,
>   * Hooks for other subsystems that check memory allocations. In a typical
>   * production configuration these hooks all should produce no code at all.
>   */
> +
> +#ifdef CONFIG_NUMA
>  static inline void kmalloc_large_node_hook(void *ptr, size_t size, gfp_t flags)
>  {
>  	kmemleak_alloc(ptr, size, 1, flags);
>  	kasan_kmalloc_large(ptr, size, flags);
>  }
> +#endif
>  
>  static inline void kfree_hook(const void *x)
>  {

[toc] | [prev] | [next] | [standalone]

#1647378 — Re: [PATCH 1/3] mm/slub: Only define kmalloc_large_node_hook() for NUMA systems

El Mon, May 22, 2017 at 01:39:26PM -0700 David Rientjes ha dit:

> On Fri, 19 May 2017, Matthias Kaehlcke wrote:
> 
> > The function is only used when CONFIG_NUMA=y. Placing it in an #ifdef
> > block fixes the following warning when building with clang:
> > 
> > mm/slub.c:1246:20: error: unused function 'kmalloc_large_node_hook'
> >     [-Werror,-Wunused-function]
> > 
> 
> Is clang not inlining kmalloc_large_node_hook() for some reason?  I don't 
> think this should ever warn on gcc.

clang warns about unused static inline functions outside of header
files, in difference to gcc.

> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> 
> Acked-by: David Rientjes <rientjes@google.com>
> 
> > ---
> >  mm/slub.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/mm/slub.c b/mm/slub.c
> > index 57e5156f02be..66e1046435b7 100644
> > --- a/mm/slub.c
> > +++ b/mm/slub.c
> > @@ -1313,11 +1313,14 @@ static inline void dec_slabs_node(struct kmem_cache *s, int node,
> >   * Hooks for other subsystems that check memory allocations. In a typical
> >   * production configuration these hooks all should produce no code at all.
> >   */
> > +
> > +#ifdef CONFIG_NUMA
> >  static inline void kmalloc_large_node_hook(void *ptr, size_t size, gfp_t flags)
> >  {
> >  	kmemleak_alloc(ptr, size, 1, flags);
> >  	kasan_kmalloc_large(ptr, size, flags);
> >  }
> > +#endif
> >  
> >  static inline void kfree_hook(const void *x)
> >  {

[toc] | [prev] | [next] | [standalone]

#1647421 — Re: [PATCH 1/3] mm/slub: Only define kmalloc_large_node_hook() for NUMA systems

On Mon, 22 May 2017 13:56:21 -0700 Matthias Kaehlcke <mka@chromium.org> wrote:

> El Mon, May 22, 2017 at 01:39:26PM -0700 David Rientjes ha dit:
> 
> > On Fri, 19 May 2017, Matthias Kaehlcke wrote:
> > 
> > > The function is only used when CONFIG_NUMA=y. Placing it in an #ifdef
> > > block fixes the following warning when building with clang:
> > > 
> > > mm/slub.c:1246:20: error: unused function 'kmalloc_large_node_hook'
> > >     [-Werror,-Wunused-function]
> > > 
> > 
> > Is clang not inlining kmalloc_large_node_hook() for some reason?  I don't 
> > think this should ever warn on gcc.
> 
> clang warns about unused static inline functions outside of header
> files, in difference to gcc.

I wish it wouldn't.  These patches just add clutter.

[toc] | [prev] | [next] | [standalone]

#1647564 — Re: [PATCH 1/3] mm/slub: Only define kmalloc_large_node_hook() for NUMA systems

On Mon, 22 May 2017, Andrew Morton wrote:

> > > Is clang not inlining kmalloc_large_node_hook() for some reason?  I don't 
> > > think this should ever warn on gcc.
> > 
> > clang warns about unused static inline functions outside of header
> > files, in difference to gcc.
> 
> I wish it wouldn't.  These patches just add clutter.
> 

Matthias, what breaks if you do this?

diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
index de179993e039..e1895ce6fa1b 100644
--- a/include/linux/compiler-clang.h
+++ b/include/linux/compiler-clang.h
@@ -15,3 +15,8 @@
  * with any version that can compile the kernel
  */
 #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
+
+#ifdef inline
+#undef inline
+#define inline __attribute__((unused))
+#endif

[toc] | [prev] | [next] | [standalone]

#1648271 — Re: [PATCH 1/3] mm/slub: Only define kmalloc_large_node_hook() for NUMA systems

Hi David,

El Mon, May 22, 2017 at 06:35:23PM -0700 David Rientjes ha dit:

> On Mon, 22 May 2017, Andrew Morton wrote:
> 
> > > > Is clang not inlining kmalloc_large_node_hook() for some reason?  I don't 
> > > > think this should ever warn on gcc.
> > > 
> > > clang warns about unused static inline functions outside of header
> > > files, in difference to gcc.
> > 
> > I wish it wouldn't.  These patches just add clutter.
> > 
> 
> Matthias, what breaks if you do this?
> 
> diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
> index de179993e039..e1895ce6fa1b 100644
> --- a/include/linux/compiler-clang.h
> +++ b/include/linux/compiler-clang.h
> @@ -15,3 +15,8 @@
>   * with any version that can compile the kernel
>   */
>  #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
> +
> +#ifdef inline
> +#undef inline
> +#define inline __attribute__((unused))
> +#endif

Thanks for the suggestion!

Nothing breaks and the warnings are silenced. It seems we could use
this if there is a stong opposition against having warnings on unused
static inline functions in .c files.

Still I am not convinced that gcc's behavior is preferable in this
case. True, it saves us from adding a bunch of __maybe_unused or
#ifdefs, on the other hand the warning is a useful tool to spot truly
unused code. So far about 50% of the warnings I looked into fall into
this category.

[toc] | [prev] | [next] | [standalone]

#1648288 — Re: [PATCH 1/3] mm/slub: Only define kmalloc_large_node_hook() for NUMA systems

Hi,

On Tue, May 23, 2017 at 9:56 AM, Matthias Kaehlcke <mka@chromium.org> wrote:
> Hi David,
>
> El Mon, May 22, 2017 at 06:35:23PM -0700 David Rientjes ha dit:
>
>> On Mon, 22 May 2017, Andrew Morton wrote:
>>
>> > > > Is clang not inlining kmalloc_large_node_hook() for some reason?  I don't
>> > > > think this should ever warn on gcc.
>> > >
>> > > clang warns about unused static inline functions outside of header
>> > > files, in difference to gcc.
>> >
>> > I wish it wouldn't.  These patches just add clutter.
>> >
>>
>> Matthias, what breaks if you do this?
>>
>> diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
>> index de179993e039..e1895ce6fa1b 100644
>> --- a/include/linux/compiler-clang.h
>> +++ b/include/linux/compiler-clang.h
>> @@ -15,3 +15,8 @@
>>   * with any version that can compile the kernel
>>   */
>>  #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
>> +
>> +#ifdef inline
>> +#undef inline
>> +#define inline __attribute__((unused))
>> +#endif
>
> Thanks for the suggestion!

Wow, I totally didn't think of this either when talking with Matthias
about this.  I guess it makes the assumption that nobody else is
hacking "inline" like we are, but "what are the chances of someone
doing that (TM)"  ;-)


> Nothing breaks and the warnings are silenced. It seems we could use
> this if there is a stong opposition against having warnings on unused
> static inline functions in .c files.
>
> Still I am not convinced that gcc's behavior is preferable in this
> case. True, it saves us from adding a bunch of __maybe_unused or
> #ifdefs, on the other hand the warning is a useful tool to spot truly
> unused code. So far about 50% of the warnings I looked into fall into
> this category.

Personally I actually prefer clang's behavior to gcc's here and I
think Matthias's patches are actually doing a service to the
community.  As he points out, many of the patches he's posted have
removed totally dead code from the kernel.  This code has often
existed for many years but never been noticed.  While the code wasn't
hurting anyone (it was dead and the compiler wasn't generating any
code for it), it would certainly add confusion to anyone reading the
source code.

Obviously my opinion isn't as important as the opinion of upstream
maintainers, but hopefully you'll consider it anyway.  :)  If you
still want to just make clang behave like gcc then I think David's
suggestion is a great one.


-Doug

[toc] | [prev] | [next] | [standalone]

#1649932 — Re: [PATCH 1/3] mm/slub: Only define kmalloc_large_node_hook() for NUMA systems

On Tue, 23 May 2017, Matthias Kaehlcke wrote:

> > diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
> > index de179993e039..e1895ce6fa1b 100644
> > --- a/include/linux/compiler-clang.h
> > +++ b/include/linux/compiler-clang.h
> > @@ -15,3 +15,8 @@
> >   * with any version that can compile the kernel
> >   */
> >  #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
> > +
> > +#ifdef inline
> > +#undef inline
> > +#define inline __attribute__((unused))
> > +#endif
> 
> Thanks for the suggestion!
> 
> Nothing breaks and the warnings are silenced. It seems we could use
> this if there is a stong opposition against having warnings on unused
> static inline functions in .c files.
> 

It would be slightly different, it would be:

#define inline inline __attribute__((unused))

to still inline the functions, I was just seeing if there was anything 
else that clang was warning about that was unrelated to a function's 
inlining.

> Still I am not convinced that gcc's behavior is preferable in this
> case. True, it saves us from adding a bunch of __maybe_unused or
> #ifdefs, on the other hand the warning is a useful tool to spot truly
> unused code. So far about 50% of the warnings I looked into fall into
> this category.
> 

I think gcc's behavior is a result of how it does preprocessing and is a 
clearly defined and long-standing semantic given in the gcc manual 
regarding -Wunused-function.

#define IS_PAGE_ALIGNED(__size)	(!(__size & ((size_t)PAGE_SIZE - 1)))
static inline int is_page_aligned(size_t size)
{
	return !(size & ((size_t)PAGE_SIZE - 1));
}

Gcc will not warn about either of these being unused, regardless of -Wall, 
-Wunused-function, or -pedantic.  Clang, correct me if I'm wrong, will 
only warn about is_page_aligned().

So the argument could be made that one of the additional benefits of 
static inline functions is that a subset of compilers, heavily in the 
minority, will detect whether it's unused and we'll get patches that 
remove them.  Functionally, it would only result in LOC reduction.  But, 
isn't adding #ifdef's to silence the warning just adding more LOC?

I have no preference either way, I think it would be up to the person who 
is maintaining the code and has to deal with the patches.

[toc] | [prev] | [next] | [standalone]

#1649999 — Re: [PATCH 1/3] mm/slub: Only define kmalloc_large_node_hook() for NUMA systems

Hi David,

El Wed, May 24, 2017 at 01:36:21PM -0700 David Rientjes ha dit:

> On Tue, 23 May 2017, Matthias Kaehlcke wrote:
> 
> > > diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
> > > index de179993e039..e1895ce6fa1b 100644
> > > --- a/include/linux/compiler-clang.h
> > > +++ b/include/linux/compiler-clang.h
> > > @@ -15,3 +15,8 @@
> > >   * with any version that can compile the kernel
> > >   */
> > >  #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
> > > +
> > > +#ifdef inline
> > > +#undef inline
> > > +#define inline __attribute__((unused))
> > > +#endif
> > 
> > Thanks for the suggestion!
> > 
> > Nothing breaks and the warnings are silenced. It seems we could use
> > this if there is a stong opposition against having warnings on unused
> > static inline functions in .c files.
> > 
> 
> It would be slightly different, it would be:
> 
> #define inline inline __attribute__((unused))
> 
> to still inline the functions, I was just seeing if there was anything 
> else that clang was warning about that was unrelated to a function's 
> inlining.
> 
> > Still I am not convinced that gcc's behavior is preferable in this
> > case. True, it saves us from adding a bunch of __maybe_unused or
> > #ifdefs, on the other hand the warning is a useful tool to spot truly
> > unused code. So far about 50% of the warnings I looked into fall into
> > this category.
> > 
> 
> I think gcc's behavior is a result of how it does preprocessing and is a 
> clearly defined and long-standing semantic given in the gcc manual 
> regarding -Wunused-function.
> 
> #define IS_PAGE_ALIGNED(__size)	(!(__size & ((size_t)PAGE_SIZE - 1)))
> static inline int is_page_aligned(size_t size)
> {
> 	return !(size & ((size_t)PAGE_SIZE - 1));
> }
> 
> Gcc will not warn about either of these being unused, regardless of -Wall, 
> -Wunused-function, or -pedantic.  Clang, correct me if I'm wrong, will 
> only warn about is_page_aligned().

Indeed, clang does not warn about unused defines.

> So the argument could be made that one of the additional benefits of 
> static inline functions is that a subset of compilers, heavily in the 
> minority, will detect whether it's unused and we'll get patches that 
> remove them.  Functionally, it would only result in LOC reduction.  But, 
> isn't adding #ifdef's to silence the warning just adding more LOC?

The LOC reduction comes from the removal of the actual dead code that
is spotted because the warning was enabled and pointed it out :)

Using #ifdef is one option, in most cases the function can be marked as
__maybe_unused, which technically doesn't (necessarily) increase
LOC. However some maintainers prefer the use of #ifdef over
__maybe_unused in certain cases.

> I have no preference either way, I think it would be up to the person who 
> is maintaining the code and has to deal with the patches.

I think it would be good to have a general policy/agreement, to either
disable the warning completely (not my preference) or 'allow' the use
of one of the available mechanism to suppress the warning for
functions that are not used in some configurations or only kept around
for reference/debugging/symmetry.

[toc] | [prev] | [next] | [standalone]

#1651529 — Re: [PATCH 1/3] mm/slub: Only define kmalloc_large_node_hook() for NUMA systems

Hi,

On Wed, May 24, 2017 at 3:09 PM, Matthias Kaehlcke <mka@chromium.org> wrote:
> Hi David,
>
> El Wed, May 24, 2017 at 01:36:21PM -0700 David Rientjes ha dit:
>
>> On Tue, 23 May 2017, Matthias Kaehlcke wrote:
>>
>> > > diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
>> > > index de179993e039..e1895ce6fa1b 100644
>> > > --- a/include/linux/compiler-clang.h
>> > > +++ b/include/linux/compiler-clang.h
>> > > @@ -15,3 +15,8 @@
>> > >   * with any version that can compile the kernel
>> > >   */
>> > >  #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
>> > > +
>> > > +#ifdef inline
>> > > +#undef inline
>> > > +#define inline __attribute__((unused))
>> > > +#endif
>> >
>> > Thanks for the suggestion!
>> >
>> > Nothing breaks and the warnings are silenced. It seems we could use
>> > this if there is a stong opposition against having warnings on unused
>> > static inline functions in .c files.
>> >
>>
>> It would be slightly different, it would be:
>>
>> #define inline inline __attribute__((unused))
>>
>> to still inline the functions, I was just seeing if there was anything
>> else that clang was warning about that was unrelated to a function's
>> inlining.
>>
>> > Still I am not convinced that gcc's behavior is preferable in this
>> > case. True, it saves us from adding a bunch of __maybe_unused or
>> > #ifdefs, on the other hand the warning is a useful tool to spot truly
>> > unused code. So far about 50% of the warnings I looked into fall into
>> > this category.
>> >
>>
>> I think gcc's behavior is a result of how it does preprocessing and is a
>> clearly defined and long-standing semantic given in the gcc manual
>> regarding -Wunused-function.
>>
>> #define IS_PAGE_ALIGNED(__size)       (!(__size & ((size_t)PAGE_SIZE - 1)))
>> static inline int is_page_aligned(size_t size)
>> {
>>       return !(size & ((size_t)PAGE_SIZE - 1));
>> }
>>
>> Gcc will not warn about either of these being unused, regardless of -Wall,
>> -Wunused-function, or -pedantic.  Clang, correct me if I'm wrong, will
>> only warn about is_page_aligned().
>
> Indeed, clang does not warn about unused defines.
>
>> So the argument could be made that one of the additional benefits of
>> static inline functions is that a subset of compilers, heavily in the
>> minority, will detect whether it's unused and we'll get patches that
>> remove them.  Functionally, it would only result in LOC reduction.  But,
>> isn't adding #ifdef's to silence the warning just adding more LOC?
>
> The LOC reduction comes from the removal of the actual dead code that
> is spotted because the warning was enabled and pointed it out :)
>
> Using #ifdef is one option, in most cases the function can be marked as
> __maybe_unused, which technically doesn't (necessarily) increase
> LOC. However some maintainers prefer the use of #ifdef over
> __maybe_unused in certain cases.
>
>> I have no preference either way, I think it would be up to the person who
>> is maintaining the code and has to deal with the patches.
>
> I think it would be good to have a general policy/agreement, to either
> disable the warning completely (not my preference) or 'allow' the use
> of one of the available mechanism to suppress the warning for
> functions that are not used in some configurations or only kept around
> for reference/debugging/symmetry.

I would tend to agree with Matthias that we need some type of general
policy / agreement with enough maintainers.  It's nice to consider all
warnings as important and if there are a few outliers it makes it
easier to ignore all warnings.

BTW: just as extra motivation showing the usefulness of this work, see
a patch I just posted that deletes a bunch of unneeded code in an ASoC
driver:

https://patchwork.kernel.org/patch/9750813/

I don't know anything about this driver but the clang warning made it
obvious that something was wrong.  Either we were doing a bit of
useless saving or (perhaps) the restore was actually supposed to be
called somewhere and we had a bug.

-Doug

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web