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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Mon, Oct 27, 2014 at 05:36:03PM +0000, Andrew Cooper 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@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.

Right. That is what it does (it sets pirq_dpci->cpu to a negative value
so that we don't try to spam the target).
> 
> The second version doesn't necessarily set HVM_DPCI_SOFTIRQ pending,

It does not have to as the target has already done so. That is because
the ->cpu value is set in raise_softirq_for which also sets the
HVM_DPIC_SOFTIRQ pending.

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

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

Wait, where did that come from? If we use mwaits IPIs are ignored?
Oh, you mean with the 'batching' support.
>O 
> Therefore, I think you need to manually set the HVM_DPCI_SOFTIRQ bit,
> then forcibly send the IPI.

OK, so the second (smp_send_event_check_cpu). And the bit is already
set - but I will add a comment explaining that.
> 
> ~Andrew
>