Re: [Xen-devel] [Patch v5 2/5] x86/hpet: Use singe apic vector rather than irq_descs for HPET interrupts

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.11.13 at 17:23, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
> On 22/11/13 15:45, Jan Beulich wrote:
>>>>> On 14.11.13 at 17:01, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>>> +/* Wake up all cpus in the channel mask.  Lock should be held. */
>>> +static void hpet_wake_cpus(struct hpet_event_channel *ch)
>>>  {
>>> -    unsigned int cpu = smp_processor_id();
>>> +    cpuidle_wakeup_mwait(ch->cpumask);
>>>  
>>> -    if ( cpumask_test_and_clear_cpu(cpu, mask) )
>>> -        raise_softirq(TIMER_SOFTIRQ);
>>> -
>>> -    cpuidle_wakeup_mwait(mask);
>>> -
>>> -    if ( !cpumask_empty(mask) )
>>> -       cpumask_raise_softirq(mask, TIMER_SOFTIRQ);
>>> +    if ( !cpumask_empty(ch->cpumask) )
>>> +       cpumask_raise_softirq(ch->cpumask, TIMER_SOFTIRQ);
>> Looks like the cpumask_empty() check isn't really needed here?
> 
> cpuidle_wakeup_mwait(mask) will only wake cpus who have set their bit in
> cpuidle_mwait_flags.
> 
> There might be cpus who have registered for waking up who have not yet
> set their bit in cpuidle_mwait_flags.
> 
> Out of caution, I did by best to avoid playing with the guts of the
> timing code, as it seems quite fragile.

Some misunderstanding? The cpumask_empty() check doesn't
guard the call to cpuidle_wakeup_mwait(), but the one to
cpumask_raise_softirq() (which appears to be prepared to be
called with an empty mask).

> I would however prefer not to manually inline hpet_setup_msi() into
> hpet_broadcast_enter().  The compiler can do that for me, and it saves
> making hpet_broadcast_enter() even more complicated.

Sure, and I wasn't suggesting that.

>>> +        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 */
>> And you nevertheless want it committed this way?
> 
> Probably best the comment gets dropped.

But is _does_ seem very ugly, so the comment is true.

>>> +        s_time_t fallback_deadline = STIME_MAX;
>>> +        unsigned int i, fallback_idx = -1;
>>> +
>>> +        for ( i = 0; i < num_hpets_used; ++i )
>>> +        {
>>> +            ch = &hpet_events[i];
>>> +            spin_lock(&ch->lock);
>>> +
>>> +            if ( ch->cpu == -1 )
>>> +                goto continue_search;
>>> +
>>> +            /* This channel is going to expire far too early */
>>> +            if ( ch->next_event < deadline - MICROSECS(50) )
>>> +                goto continue_search;
>> So this is going to make us wake early. You remember that all this
>> code exists solely for power saving purposes? Iiuc the CPU will
>> then basically spin entering an idle state, until its actual wakeup
>> time is reached.
> 
> What would you suggest instead?  We dont really want to be waking up late.

In fact we're always kind of waking up late, due to the processing
time it takes to come back out of the C state. If someone heavily
favors responsiveness (i.e. low wakeup latency) over power
savings, (s)he should disallow the use of deep C states.

>>> +            if ( ch->cpu != -1 && ch->next_event == fallback_deadline )
>> Can't this be "<= deadline", being quite a bit more flexible?
> 
> This is a test for whether ch is the one I identified as the best inside
> the loop.  There should be sufficient flexibility inside the loop.

The thing is that with the lock dropped, ch->next_event may have
changed. But it would still suit us if <= deadline.

>>> +            else
>>> +                /* All else has failed.  Wander the idle loop again */
>>> +                this_cpu(timer_deadline) = NOW() - 1;
>>> +
>>> +            spin_unlock(&ch->lock);
>>> +        }
>>> +        else
>>> +            /* All HPETs were too soon.  Wander the idle loop again */
>>> +            this_cpu(timer_deadline) = NOW() - 1;
>> Here and above - what good will this do? Is this just in the
>> expectation that the next time round a free channel will likely be
>> found? If so, why can't you just go back to the start of the
>> function.
> 
> Or a different HPET programmed to a different time which is now
> compatible with our wakeup timeframe.

Are there cases where the wakeup time gets moved backwards
(i.e. less far in the future)? I can't seem to immediately think of
such a case.

> We cannot spin in this function, as we have interrupts disabled.

And I was suggesting this only if the failure condition would provide
a positive sign of there some suitable resource having become
available.

Jan

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