Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <1583509304-28508-1-git-send-email-cai@lca.pw>
In-Reply-To: <1583509304-28508-1-git-send-email-cai@lca.pw>
From:   Nick Desaulniers <ndesaulniers@google.com>
Date:   Fri, 6 Mar 2020 09:13:52 -0800
Message-ID: <CAKwvOd=V44ksbiffN5UYw-oVfTK_wdeP59ipWANkOUS_zavxew@mail.gmail.com>
Subject: Re: [PATCH] sched/cputime: silence a -Wunused-function warning
To:     Qian Cai <cai@lca.pw>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>, juri.lelli@redhat.com,
        Vincent Guittot <vincent.guittot@linaro.org>,
        dietmar.eggemann@arm.com, Steven Rostedt <rostedt@goodmis.org>,
        bsegall@google.com, mgorman@suse.de,
        LKML <linux-kernel@vger.kernel.org>,
        clang-built-linux <clang-built-linux@googlegroups.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Mar 6, 2020 at 7:42 AM Qian Cai <cai@lca.pw> wrote:
>
> account_other_time() is only used when CONFIG_IRQ_TIME_ACCOUNTING=y (in
> irqtime_account_process_tick()) or CONFIG_VIRT_CPU_ACCOUNTING_GEN=y (in
> get_vtime_delta()). When both are off, it will generate a compilation
> warning from Clang,
>
> kernel/sched/cputime.c:255:19: warning: unused function
> 'account_other_time' [-Wunused-function]
> static inline u64 account_other_time(u64 max)
>
> Rather than wrapping around this function with a macro expression,
>
>  if defined(CONFIG_IRQ_TIME_ACCOUNTING) || \
>     defined(CONFIG_VIRT_CPU_ACCOUNTING_GEN)
>
> just use __maybe_unused for this small function which seems like a good
> trade-off.
>
> Signed-off-by: Qian Cai <cai@lca.pw>

Hi Qian, thanks for the patch!

Generally, I'm not a fan of __maybe_unused.  It is a tool in the
toolbox for solving this issue, but it's my least favorite.  Should
the call sites be eliminated, this may mask an unused and entirely
dead function from being removed.  Preprocessor guards based on config
give more context into *why* a particular function may be unused.

So let's take a look at the call sites of account_other_time().  Looks
like irqtime_account_process_tick() (guarded by
CONFIG_IRQ_TIME_ACCOUNTING) and get_vtime_delta() (guarded by
CONFIG_VIRT_CPU_ACCOUNTING_GEN).  If it were one config guard, then I
would prefer to move the definition to be within the same guard, just
before the function definition that calls it, but we have a more
complicated case here.

The next thing I'd check to see is if there's a dependency between
configs.  In this case, both CONFIG_IRQ_TIME_ACCOUNTING and
CONFIG_VIRT_CPU_ACCOUNTING_GEN are defined in init/Kconfig.  In this
case there's also no dependency between configs, so specifying one
doesn't imply the other; so guarding on one of the two configs is also
not an option.

So, as much as I'm not a fan of __maybe_unused, it is indeed the
simplest option.  Though, I'd still prefer the ifdef you describe
instead, maybe the maintainers can provide guidance/preference?
Otherwise,

Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

> ---
>  kernel/sched/cputime.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
> index cff3e656566d..85da4d6dee24 100644
> --- a/kernel/sched/cputime.c
> +++ b/kernel/sched/cputime.c
> @@ -252,7 +252,7 @@ static __always_inline u64 steal_account_process_time(u64 maxtime)
>  /*
>   * Account how much elapsed time was spent in steal, irq, or softirq time.
>   */
> -static inline u64 account_other_time(u64 max)
> +static inline __maybe_unused u64 account_other_time(u64 max)
>  {
>         u64 accounted;
>
> --

-- 
Thanks,
~Nick Desaulniers