Re: [PATCH v2 2/5] x86/hpet: Use singe apic vector rather than irq_descs for HPET interrupts

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 08/11/13 01:08, Tim Deegan wrote:
> Hi,
>
> At 15:28 +0000 on 07 Nov (1383834502), Andrew Cooper wrote:
>> This involves rewriting most of the MSI related HPET code, and as a result
>> this patch looks very complicated.  It is probably best viewed as an end
>> result, with the following notes explaining what is going on.
>>
>> The new logic is as follows:
>>  * A single high priority vector is allocated and uses on all cpus.
>>  * Reliance on the irq infrastructure is completely removed.
>>  * Tracking of free hpet channels has changed.  It is now an individual
>>    bitmap, and allocation is based on winning a test_and_clear_bit()
>>    operation.
>>  * There is a notion of strict ownership of hpet channels.
>>  ** A cpu which owns an HPET channel can program it for a desired deadline.
>>  ** A cpu which can't find a free HPET channel to own may register for being
>>     woken up by another in-use HPET which will fire at an appropriate time.
>>  * Some functions have been renamed to be more descriptive.  Some functions
>>    have parameters changed to be more consistent.
>>  * Any function with a __hpet prefix expectes the appropriate lock to be held.
> It certainly looks like the code should be easier to understand after
> this!  I'll try to read through the end result later in the week, but
> I have a few questions from the patch:
>
>> -static void handle_hpet_broadcast(struct hpet_event_channel *ch)
>> +static void __hpet_interrupt(struct hpet_event_channel *ch)
>>  {
> [...]
>> +    __hpet_program_time(ch, this_cpu(timer_deadline), NOW(), 1);
>> +    raise_softirq(TIMER_SOFTIRQ);
>>  }
> What's the __hpet_program_time() doing?  It looks like we're
> reprogramming the hpet for our next event, before we handle the event
> we woke up for (i.e. always setting to to fire immediately).  Or have
> I misunderstood?

Hmm yes - on further consideration this is silly.  When moving the logic
around I did try to err on the side of what the old logic did wrt the
internal timing.

However, as we will unconditionally will wander through
hpet_broadcast_exit() and around to hpet_broadcast_enter() again,
reprogramming the channel at this point is crazy.

>
>> @@ -619,9 +425,8 @@ void __init hpet_broadcast_init(void)
>>          hpet_events[i].shift = 32;
>>          hpet_events[i].next_event = STIME_MAX;
>>          spin_lock_init(&hpet_events[i].lock);
>> -        wmb();
>> -        hpet_events[i].event_handler = handle_hpet_broadcast;
>>  
>> +        hpet_events[1].msi.irq = -1;
> Really [1] or DYM [i]?

Oops. Good catch. The font in my terminal makes those even harder to
distinguish.

>
>>          hpet_events[i].msi.msi_attrib.maskbit = 1;
>>          hpet_events[i].msi.msi_attrib.pos = MSI_TYPE_HPET;
>>      }
>> @@ -661,9 +466,6 @@ void hpet_broadcast_resume(void)
>>  
>>      for ( i = 0; i < n; i++ )
>>      {
>> -        if ( hpet_events[i].msi.irq >= 0 )
>> -            __hpet_setup_msi_irq(irq_to_desc(hpet_events[i].msi.irq));
>> -
>>          /* set HPET Tn as oneshot */
>>          cfg = hpet_read32(HPET_Tn_CFG(hpet_events[i].idx));
>>          cfg &= ~(HPET_TN_LEVEL | HPET_TN_PERIODIC);
>> @@ -706,36 +508,76 @@ void hpet_disable_legacy_broadcast(void)
>>  void hpet_broadcast_enter(void)
>>  {
>>      unsigned int cpu = smp_processor_id();
>> -    struct hpet_event_channel *ch = per_cpu(cpu_bc_channel, cpu);
>> +    struct hpet_event_channel *ch = this_cpu(hpet_channel);
>>      s_time_t deadline = this_cpu(timer_deadline);
>>  
>>      ASSERT(!local_irq_is_enabled());
>> +    ASSERT(ch == NULL);
>>  
>>      if ( deadline == 0 )
>>          return;
>>  
>> -    if ( !ch )
>> -        ch = hpet_get_channel(cpu);
>> +    ch = hpet_get_free_channel();
>>  
>> +    if ( ch )
>> +    {
>> +        /* This really should be an MSI channel by this point */
>> +        ASSERT( !(ch->flags & HPET_EVT_LEGACY) );
>> +
>> +        spin_lock(&ch->lock);
>> +
>> +        this_cpu(hpet_channel) = ch;
>> +        ch->cpu = cpu;
>> +        cpumask_set_cpu(cpu, ch->cpumask);
>> +
>> +        __hpet_setup_msi(ch);
>> +        __hpet_program_time(ch, deadline, NOW(), 1);
>> +        __hpet_msi_unmask(ch);
>> +
>> +        spin_unlock(&ch->lock);
>> +
>> +    }
>> +    else
>> +    {
>> +        /* TODO - this seems very ugly */
>> +        unsigned i;
>> +
>> +        for ( i = 0; i < num_hpets_used; ++i )
>> +        {
>> +            ch = &hpet_events[i];
>> +            spin_lock(&ch->lock);
>> +
>> +            if ( ch->cpu == -1 )
>> +                goto continue_search;
>> +
>> +            if ( ch->next_event >= deadline - MICROSECS(50) &&
>> +                 ch->next_event <= deadline )
>> +                break;
>> +
>> +        continue_search:
>> +            spin_unlock(&ch->lock);
>> +            ch = NULL;
>> +        }
>> +
>> +        if ( ch )
>> +        {
>> +            cpumask_set_cpu(cpu, ch->cpumask);
>> +            this_cpu(hpet_channel) = ch;
>> +            spin_unlock(&ch->lock);
>> +        }
>> +        else
>> +            this_cpu(timer_deadline) = NOW();
> Hmm.  So if we can't find a channel that fits the deadline we want,  
> we give up?  I thought the plan was to cause some other channel to
> fire early.
>
> Tim.

I considered that, but (experimentally) the typcial period of time asked
for is forced upwards by MIN_DELTA_NS, meaning that another cpu which
cant find an HPET is almost certainly going to find a valid one given
50us slack.  I decided that, in the rare case where this might occur,
wandering around the idle loop and trying again was rather safer than
reprogramming the hpet to be sooner, which then needs to have -ETIME
logic and emulated wakeup in the case that the deadline was missed.

~Andrew

>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel