Re: [PATCH for-4.21 01/10] x86/HPET: limit channel changes

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.10.2025 12:24, Roger Pau Monné wrote:
> On Thu, Oct 16, 2025 at 09:31:21AM +0200, Jan Beulich wrote:
>> Despite 1db7829e5657 ("x86/hpet: do local APIC EOI after interrupt
>> processing") we can still observe nested invocations of
>> hpet_interrupt_handler(). This is, afaict, a result of previously used
>> channels retaining their IRQ affinity until some other CPU re-uses them.
> 
> But the underlying problem here is not so much the affinity itself,
> but the fact that the channel is not stopped after firing?

(when being detached, that is) That's the main problem here, yes. A minor
benefit is to avoid the MMIO write in hpet_msi_set_affinity(). See also
below.

Further, even when mask while detaching, the issue would re-surface after
unmasking; it's just that the window then is smaller.

>> @@ -454,9 +456,21 @@ static struct hpet_event_channel *hpet_g
>>      if ( num_hpets_used >= nr_cpu_ids )
>>          return &hpet_events[cpu];
>>  
>> +    /*
>> +     * Try the least recently used channel first.  It may still have its 
>> IRQ's
>> +     * affinity set to the desired CPU.  This way we also limit having 
>> multiple
>> +     * of our IRQs raised on the same CPU, in possibly a nested manner.
>> +     */
>> +    ch = per_cpu(lru_channel, cpu);
>> +    if ( ch && !test_and_set_bit(HPET_EVT_USED_BIT, &ch->flags) )
>> +    {
>> +        ch->cpu = cpu;
>> +        return ch;
>> +    }
>> +
>> +    /* Then look for an unused channel. */
>>      next = arch_fetch_and_add(&next_channel, 1) % num_hpets_used;
>>  
>> -    /* try unused channel first */
>>      for ( i = next; i < next + num_hpets_used; i++ )
>>      {
>>          ch = &hpet_events[i % num_hpets_used];
>> @@ -479,6 +493,8 @@ static void set_channel_irq_affinity(str
>>  {
>>      struct irq_desc *desc = irq_to_desc(ch->msi.irq);
>>  
>> +    per_cpu(lru_channel, ch->cpu) = ch;
>> +
>>      ASSERT(!local_irq_is_enabled());
>>      spin_lock(&desc->lock);
>>      hpet_msi_mask(desc);
> 
> Maybe I'm missing the point here, but you are resetting the MSI
> affinity anyway here, so there isn't much point in attempting to
> re-use the same channel when Xen still unconditionally goes through the
> process of setting the affinity anyway?

While still using normal IRQs, there's still a benefit: We can re-use the
same vector (as staying on the same CPU), and hence we save an IRQ
migration (being the main source of nested IRQs according to my
observations).

We could actually do even better, by avoiding the mask/unmask pair there,
which would avoid triggering the "immediate" IRQ that I (for now) see as
the only explanation of the large amount of "early" IRQs that I observe
on (at least) Intel hardware. That would require doing the msg.dest32
check earlier, but otherwise looks feasible. (Actually, the unmask would
still be necessary, in case we're called with the channel already masked.)

Jan