* Re: [PATCH 1/3] mm/slub: Only define kmalloc_large_node_hook() for NUMA systems
@ 2017-05-23 16:56 ` Matthias Kaehlcke
0 siblings, 0 replies; 28+ messages in thread
From: Matthias Kaehlcke @ 2017-05-23 16:56 UTC (permalink / raw)
To: David Rientjes
Cc: Andrew Morton, Christoph Lameter, Pekka Enberg, Joonsoo Kim,
linux-mm, linux-kernel, Douglas Anderson
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.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/3] mm/slub: Only define kmalloc_large_node_hook() for NUMA systems
2017-05-23 16:56 ` Matthias Kaehlcke
@ 2017-05-23 17:12 ` Doug Anderson
-1 siblings, 0 replies; 28+ messages in thread
From: Doug Anderson @ 2017-05-23 17:12 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: David Rientjes, Andrew Morton, Christoph Lameter, Pekka Enberg,
Joonsoo Kim, linux-mm, linux-kernel@vger.kernel.org
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
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/3] mm/slub: Only define kmalloc_large_node_hook() for NUMA systems
@ 2017-05-23 17:12 ` Doug Anderson
0 siblings, 0 replies; 28+ messages in thread
From: Doug Anderson @ 2017-05-23 17:12 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: David Rientjes, Andrew Morton, Christoph Lameter, Pekka Enberg,
Joonsoo Kim, linux-mm, linux-kernel@vger.kernel.org
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/3] mm/slub: Only define kmalloc_large_node_hook() for NUMA systems
2017-05-23 16:56 ` Matthias Kaehlcke
@ 2017-05-24 20:36 ` David Rientjes
-1 siblings, 0 replies; 28+ messages in thread
From: David Rientjes @ 2017-05-24 20:36 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: Andrew Morton, Christoph Lameter, Pekka Enberg, Joonsoo Kim,
linux-mm, linux-kernel, Douglas Anderson
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.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 1/3] mm/slub: Only define kmalloc_large_node_hook() for NUMA systems
@ 2017-05-24 20:36 ` David Rientjes
0 siblings, 0 replies; 28+ messages in thread
From: David Rientjes @ 2017-05-24 20:36 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: Andrew Morton, Christoph Lameter, Pekka Enberg, Joonsoo Kim,
linux-mm, linux-kernel, Douglas Anderson
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.
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 1/3] mm/slub: Only define kmalloc_large_node_hook() for NUMA systems
2017-05-24 20:36 ` David Rientjes
@ 2017-05-24 22:09 ` Matthias Kaehlcke
-1 siblings, 0 replies; 28+ messages in thread
From: Matthias Kaehlcke @ 2017-05-24 22:09 UTC (permalink / raw)
To: David Rientjes
Cc: Andrew Morton, Christoph Lameter, Pekka Enberg, Joonsoo Kim,
linux-mm, linux-kernel, Douglas Anderson
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.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 1/3] mm/slub: Only define kmalloc_large_node_hook() for NUMA systems
@ 2017-05-24 22:09 ` Matthias Kaehlcke
0 siblings, 0 replies; 28+ messages in thread
From: Matthias Kaehlcke @ 2017-05-24 22:09 UTC (permalink / raw)
To: David Rientjes
Cc: Andrew Morton, Christoph Lameter, Pekka Enberg, Joonsoo Kim,
linux-mm, linux-kernel, Douglas Anderson
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.
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 1/3] mm/slub: Only define kmalloc_large_node_hook() for NUMA systems
2017-05-24 22:09 ` Matthias Kaehlcke
@ 2017-05-26 17:05 ` Doug Anderson
-1 siblings, 0 replies; 28+ messages in thread
From: Doug Anderson @ 2017-05-26 17:05 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: David Rientjes, Andrew Morton, Christoph Lameter, Pekka Enberg,
Joonsoo Kim, linux-mm, linux-kernel@vger.kernel.org
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
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 1/3] mm/slub: Only define kmalloc_large_node_hook() for NUMA systems
@ 2017-05-26 17:05 ` Doug Anderson
0 siblings, 0 replies; 28+ messages in thread
From: Doug Anderson @ 2017-05-26 17:05 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: David Rientjes, Andrew Morton, Christoph Lameter, Pekka Enberg,
Joonsoo Kim, linux-mm, linux-kernel@vger.kernel.org
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
^ permalink raw reply [flat|nested] 28+ messages in thread