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

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

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@citrix.com> 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.

The second version doesn't necessarily set HVM_DPCI_SOFTIRQ pending,
while the first version suffers a risk of the softirq being caught in a
batch.

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

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

~Andrew