[PATCH 0/3] cfq-iosched: Fix warnings about unused functions

[PATCH 0/3] cfq-iosched: Fix warnings about unused functions

[Matthias Kaehlcke]

This patch series fixes a bunch of 'unused-function' warnings raised by
clang.

Matthias Kaehlcke (3):
  cfq-iosched: Mark cfq_clear_cfqq_*() as __maybe_unused
  cfq-iosched: Fix warning about unused dummy functions
  cfq-iosched: Delete unused function min_vdisktime()

 block/cfq-iosched.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

-- 
2.13.0.219.gdb65acc882-goog

[PATCH 1/3] cfq-iosched: Mark cfq_clear_cfqq_*() as __maybe_unused

[Matthias Kaehlcke]

This fixes the following warning when building with clang:

    block/cfq-iosched.c:449:1: error: unused function 'cfq_clear_cfqq_sync'
        [-Werror,-Wunused-function]

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
 block/cfq-iosched.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index da69b079725f..36ab3645effc 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -431,7 +431,8 @@ static inline void cfq_mark_cfqq_##name(struct cfq_queue *cfqq)		\
 {									\
 	(cfqq)->flags |= (1 << CFQ_CFQQ_FLAG_##name);			\
 }									\
-static inline void cfq_clear_cfqq_##name(struct cfq_queue *cfqq)	\
+static inline void __maybe_unused					\
+cfq_clear_cfqq_##name(struct cfq_queue *cfqq)				\
 {									\
 	(cfqq)->flags &= ~(1 << CFQ_CFQQ_FLAG_##name);			\
 }									\
-- 
2.13.0.219.gdb65acc882-goog

[PATCH 2/3] cfq-iosched: Fix warning about unused dummy functions

[Matthias Kaehlcke]

Only define dummies for cfqg_stats_set_start_group_wait_time()
and cfqg_stats_end_empty_time() when CONFIG_CFQ_GROUP_IOSCHED=y. This
fixes the following warnings when building with clang:

    block/cfq-iosched.c:589:20: error: unused function
        'cfqg_stats_set_start_group_wait_time' [-Werror,-Wunused-function]

    block/cfq-iosched.c:590:20: error: unused function
        'cfqg_stats_end_empty_time' [-Werror,-Wunused-function]

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
 block/cfq-iosched.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index 36ab3645effc..b23c73db4cc0 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -588,8 +588,10 @@ static void cfqg_stats_update_avg_queue_size(struct cfq_group *cfqg)
 
 #else	/* CONFIG_CFQ_GROUP_IOSCHED && CONFIG_DEBUG_BLK_CGROUP */
 
+#ifdef CONFIG_CFQ_GROUP_IOSCHED
 static inline void cfqg_stats_set_start_group_wait_time(struct cfq_group *cfqg, struct cfq_group *curr_cfqg) { }
 static inline void cfqg_stats_end_empty_time(struct cfqg_stats *stats) { }
+#endif
 static inline void cfqg_stats_update_dequeue(struct cfq_group *cfqg) { }
 static inline void cfqg_stats_set_start_empty_time(struct cfq_group *cfqg) { }
 static inline void cfqg_stats_update_idle_time(struct cfq_group *cfqg) { }
-- 
2.13.0.219.gdb65acc882-goog

[PATCH 3/3] cfq-iosched: Delete unused function min_vdisktime()

[Matthias Kaehlcke]

This fixes the following warning when building with clang:

    block/cfq-iosched.c:970:19: error: unused function 'min_vdisktime'
        [-Werror,-Wunused-function]

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
 block/cfq-iosched.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index b23c73db4cc0..61cb45553314 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -981,15 +981,6 @@ static inline u64 max_vdisktime(u64 min_vdisktime, u64 vdisktime)
 	return min_vdisktime;
 }
 
-static inline u64 min_vdisktime(u64 min_vdisktime, u64 vdisktime)
-{
-	s64 delta = (s64)(vdisktime - min_vdisktime);
-	if (delta < 0)
-		min_vdisktime = vdisktime;
-
-	return min_vdisktime;
-}
-
 static void update_min_vdisktime(struct cfq_rb_root *st)
 {
 	struct cfq_group *cfqg;
-- 
2.13.0.219.gdb65acc882-goog

Re: [PATCH 1/3] cfq-iosched: Mark cfq_clear_cfqq_*() as __maybe_unused

[Christoph Hellwig]

On Fri, May 26, 2017 at 02:22:35PM -0700, Matthias Kaehlcke wrote:
> This fixes the following warning when building with clang:
> 
>     block/cfq-iosched.c:449:1: error: unused function 'cfq_clear_cfqq_sync'
>         [-Werror,-Wunused-function]
> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>

Matthias, can you please stop sending these patches?  Gcc semantics
correctly are that static inlines can be unused and it's perfectly
fine.  It's your job to make clang fit that instead of spreading garbage
all over the kernel.

Re: [PATCH 3/3] cfq-iosched: Delete unused function min_vdisktime()

[Jens Axboe]

On 05/26/2017 03:22 PM, Matthias Kaehlcke wrote:
> This fixes the following warning when building with clang:
> 
>     block/cfq-iosched.c:970:19: error: unused function 'min_vdisktime'
>         [-Werror,-Wunused-function]

I have applied this one for 4.13, as that is just dead code. I don't
think the others are worth bothering with.

-- 
Jens Axboe

Re: [PATCH 1/3] cfq-iosched: Mark cfq_clear_cfqq_*() as __maybe_unused

[Doug Anderson]

Christoph,

On Sun, May 28, 2017 at 12:49 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Fri, May 26, 2017 at 02:22:35PM -0700, Matthias Kaehlcke wrote:
>> This fixes the following warning when building with clang:
>>
>>     block/cfq-iosched.c:449:1: error: unused function 'cfq_clear_cfqq_sync'
>>         [-Werror,-Wunused-function]
>>
>> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
>
> Matthias, can you please stop sending these patches?  Gcc semantics
> correctly are that static inlines can be unused and it's perfectly
> fine.  It's your job to make clang fit that instead of spreading garbage
> all over the kernel.

I think we've been having discussions about this in many different
scattered threads.  A quick summary here is:

* clang only warns about unused static inline functions if those
functions are in ".c" files.  That basically means there aren't nearly
as many false positives of the check as you would think.

* Matthias has found several instances of dead code with his work.
It's nice to get rid of those.  In addition, in at least one example
his work has actually identified code that was not dead (AKA actual
instructions were generated) but the code was clearly not correct.
That's because there was a "static inline" save and restore function.
The save was called but not the restore.  This points to either a bug
(should have called the restore) or code that should be eliminated.
https://patchwork.kernel.org/patch/9750813/

* Most compiler warnings generate a bit of "noise".  It's always a
judgement call about whether the signal to noise ratio makes the
warning useful.  This is a tough call, but IMHO the signal to noise
ratio for the clang behavior makes it worth it.  The number of "maybe
unused" patches is not that great and IMHO adding the attribute is
also documenting the function, which is useful too.

* There exists a patch to make clang behave like gcc.
https://patchwork.kernel.org/patch/9746913/.  If folks truly believe
that the noise is not worth it, we can apply that.


I just talked to Matthias and he is going to try to start a thread to
get hopefully get a general policy agreed upon before continuing to
post patches.


-Doug