> On Mar 6, 2020, at 12:13 PM, Nick Desaulniers <[email protected]> wrote: > > On Fri, Mar 6, 2020 at 7:42 AM Qian Cai <[email protected]> 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 <[email protected]> > > 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 <[email protected]> Peter, can you take a look at this patch when you have a chance? Having Clang to complain this every time is a bit annoying. > >> --- >> 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
