Re: [PATCH] timer: fix NR_CPUS=1 build with gcc13

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.09.2023 11:44, George Dunlap wrote:
> On Wed, Sep 13, 2023 at 8:32 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>
>> Gcc13 apparently infers from "if ( old_cpu < new_cpu )" that "new_cpu"
>> is >= 1, and then (on x86) complains about "per_cpu(timers, new_cpu)"
>> exceeding __per_cpu_offset[]'s bounds (being an array of 1 in such a
>> configuration). Make the code conditional upon there being at least 2
>> CPUs configured (otherwise there simply is nothing to migrate [to]).
> 
> Hmm, without digging into it, migrate_timer() doesn't seem like very
> robust code: It doesn't check to make sure that new_cpu is valid, nor
> does it give the option of returning an error if anything fails.

Question is - what do you expect the callers to do upon getting back
failure?

> Making migrate_timer() just do nothing on 1-cpu systems seems will
> remove the warning, but not really make things safer.
> 
> Is this a super hot path?

I don't think it is.

>  Would it make more sense to add `||
> (new_cpu > CONFIG_NR_CPUS)` to the early-return  conditional at the
> top of the first `for (; ; )` loop?

But that would mean not doing what was requested without any indication
to the caller. An out-of-range CPU passed in is generally very likely
to result in a crash, I think.

> I guess if we don't expect it ever to be called, it might be better to
> get rid of the code entirely; but maybe in that case we should add
> something like the following?
> 
> ```
> #else
>     WARN_ONCE("migrate_timer: Request to move to %u on a single-core
> system!", new_cpu);
>     ASSERT_UNREACHABLE();
> #endif
> ```

With the old_cpu == new_cpu case explicitly permitted (and that being
the only legal case when NR_CPUS=1, which arguably is an aspect which
makes gcc's diagnostic questionable), perhaps only

#else
    old_cpu = ...;
    if ( old_cpu != TIMER_CPU_status_killed )
        WARN_ON(new_cpu != old_cpu);
#endif

(I'm afraid we have no WARN_ON_ONCE() yet, nor WARN_ONCE())?

Jan