Re: [PATCH RT 10/30] hrtimer: Prevent using hrtimer_grab_expiry_lock() on migration_base

[Rasmus Villemoes]

On 28/04/2020 14.59, Tom Zanussi wrote:
> On Tue, 2020-04-28 at 09:03 +0200, Rasmus Villemoes wrote:

>> Hold on a second. This patch (hrtimer: Prevent using
>> hrtimer_grab_expiry_lock() on migration_base) indeed seems to
>> implement
>> the optimization implied by the above, namely avoid the lock/unlock
>> in
>> case base == migration_base:
>>
>>> -	if (timer->is_soft && base && base->cpu_base) {
>>> +	if (timer->is_soft && base != &migration_base) {
>>
>> But the followup patch (hrtimer: Add a missing bracket and hide
>> `migration_base on !SMP) to fix the build on !SMP [the missing
>> bracket
>> part seems to have been fixed when backporting the above to 4.19-rt]
>> replaces that logic by
>>
>> +static inline bool is_migration_base(struct hrtimer_clock_base
>> *base)
>> +{
>> +	return base == &migration_base;
>> +}
>> +
>> ...
>> -	if (timer->is_soft && base != &migration_base) {
>> +	if (timer->is_soft && is_migration_base(base)) {
>>
>> in the SMP case, i.e. the exact opposite condition. One of these
>> can't
>> be correct.
>>
>> Assuming the followup patch was wrong and the condition should have
>> read
>>
>>   timer->is_soft && !is_migration_base(base)
>>
>> while keeping is_migration_base() false on !SMP might explain the
>> problem I see. But I'd like someone who knows this code to chime in.
>>
> 
> I don't know this code, but I think you're correct - the followup patch
> reversed the condition by forgetting the !.
> 
> So, does your problem go away when you make that change?

Yes, it does. (I'll have to ask the customer to check in their setup
whether the boot hang also vanishes).

Essentially, adding that ! is equivalent to reverting the two patches on
!SMP (which I also tested): Before, the condition was

  timer->is_soft && base && base->cpu_base

and, assuming the NULL pointer checks are indeed redundant, that's the
same as "timer->is_soft". Appending " && !is_migration_base()" to that,
with is_migration_base() always false as on !SMP, doesn't change anything.

Thanks,
Rasmus