Re: [Xen-devel] [PATCH v8 for-xen-4.5 2/2] dpci: Replace tasklet with an softirq (v8)

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.10.14 at 18:36, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 27/10/14 17:01, Konrad Rzeszutek Wilk wrote:
>> On Mon, Oct 27, 2014 at 11:24:31AM +0000, Jan Beulich wrote:
>>>>>> On 27.10.14 at 12:09, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>> Can it ever be the case that we are waiting for a remote pcpu to run its
>>>> softirq handler?  If so, the time spent looping here could be up to 1
>>>> scheduling timeslice in the worst case, and 30ms is a very long time to
>>>> wait.
>>> Good point - I think this can be the case. But there seems to be a
>>> simple counter measure: The first time we get to this point, send an
>>> event check IPI to the CPU in question (or in the worst case
>>> broadcast one if the CPU can't be determined in a race free way).
>> I can either do this using the wrapper:
>>
>>      if ( pt_pirq_softirq_active(pirq_dpci) )
>>      {
>>          spin_unlock(&d->event_lock);
>>       if ( pirq_dpci->cpu >= 0 )
>>       {
>>              cpu_raise_softirq(pirq_dpci->cpu, HVM_DPCI_SOFTIRQ);
>>              pirq_dpci->cpu = -1;
>>       }
>>          cpu_relax();
>>          goto restart;
>>
>> Ought to do it (cpu_raise_softirq will exit out if
>> the 'pirq_dpci->cpu == smp_processor_id()'). It also has some batching checks
>> so that we won't do the IPI if we are in the middle of IPI-ing already
>> an CPU.
>>
>> Or just write it out (and bypass some of the checks 'cpu_raise_softirq'
>> has):
>>
>>      if ( pt_pirq_softirq_active(pirq_dpci) )
>>      {
>>          spin_unlock(&d->event_lock);
>>       if ( pirq_dpci->cpu >= 0 && pirq_dpci->cpu != smp_processor_id() )
>>       {
>>              smp_send_event_check_cpu(pirq_dpci->cpu);
>>              pirq_dpci->cpu = -1;
>>       }
>>          cpu_relax();
>>          goto restart;
>>
>>
>> Note:
>>
>> The 'cpu' is stashed whenever 'raise_softirq_for' has been called.
>>
> 
> You need to send at most 1 IPI, or you will be pointlessly spamming the
> target pcpu.  Therefore, a blind goto restart seems ill-advised.

With ->cpu being set to -1, I don't see how more than one IPI would
get sent here.

> The second version doesn't necessarily set HVM_DPCI_SOFTIRQ pending,

Right.

> while the first version suffers a risk of the softirq being caught in a
> batch.

Not without anyone up the call stack having called
cpu_raise_softirq_batch_begin().

> Furthermore, with mwait support, the IPI is elided completely, which is
> completely wrong in this situation.

As already said on IRC, this isn't the case: An IPI gets avoided only
when we _know_ the remote CPU is MWAITing (or resuming from
MWAIT).

> Therefore, I think you need to manually set the HVM_DPCI_SOFTIRQ bit,
> then forcibly send the IPI.

Open coding things is almost always wrong.

Jan


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