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

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

On 22/11/13 16:49, Jan Beulich wrote:
>>>> On 22.11.13 at 17:23, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> On 22/11/13 15:45, Jan Beulich wrote:
>>>>>> On 14.11.13 at 17:01, Andrew Cooper <andrew.cooper3@citrix.com> 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).

cpuidle_wakeup_mwait() clears bits out of mask.  The result is the set
of cpus who were not set in cpuidle_mwait_flags.

>
>> 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.

Very true - I missed that.

>
>>>> +            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.

The case I was expecting is an HPET about to fire and being relinquished
by its owner.  After an iteration of the idle loop it might be free to
nab, or a different cpu has nabbed it and programmed it for a reasonable
time into the future.

~Andrew

>
>> 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