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

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Tue, Oct 28, 2014 at 10:43:32AM +0000, Jan Beulich wrote:
> >>> On 27.10.14 at 22:13, <konrad.wilk@xxxxxxxxxx> wrote:
> > +    /*
> > +     * A crude 'while' loop with us dropping the spinlock and giving
> > +     * the softirq_dpci a chance to run.
> > +     * We MUST check for this condition as the softirq could be scheduled
> > +     * and hasn't run yet. Note that this code replaced tasklet_kill which
> > +     * would have spun forever and would do the same thing (wait to flush 
> > out
> > +     * outstanding hvm_dirq_assist calls.
> > +     */
> > +    if ( pt_pirq_softirq_active(pirq_dpci) )
> > +    {
> > +        spin_unlock(&d->event_lock);
> > +        if ( pirq_dpci->cpu >= 0 && pirq_dpci->cpu != smp_processor_id() )
> > +        {
> > +            /*
> > +             * The 'raise_softirq_for' sets the CPU and raises the softirq 
> > bit
> > +             * so we do not need to set the target CPU's HVM_DPCI_SOFTIRQ.
> > +             */
> > +            smp_send_event_check_cpu(pirq_dpci->cpu);
> > +            pirq_dpci->cpu = -1;
> > +        }
> > +        cpu_relax();
> > +        goto restart;
> > +    }
> 
> As said in an earlier reply to Andrew, I think this open coding goes
> too far. And with the softirq known to have got sent, I also don't
> really see why it needs to be resent _at all_ (and the comments
> don't explain this either).

In the other emails you and Andrew said:

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

Which is true. That is what this is trying to address.

But if we use 'cpu_raise_softirq' which you advocate it would inhibit the IPI
if the  HVM_DPCI_SOFTIRQ is set on the remote CPU:

void cpu_raise_softirq(unsigned int cpu, unsigned int nr) 
{
    unsigned int this_cpu = smp_processor_id();

    if ( test_and_set_bit(nr, &softirq_pending(cpu))            <=== that will 
be true
         || (cpu == this_cpu)
         || arch_skip_send_event_check(cpu) )
        return;

    if ( !per_cpu(batching, this_cpu) || in_irq() )
        smp_send_event_check_cpu(cpu);
    else
        set_bit(nr, &per_cpu(batch_mask, this_cpu));
}

In which case we still won't be sending the IPI. The open-usage of
'smp_send_event_check_cpu' would bypass that check (which would in this
scenario cause to inhibit the IPI).

Perhaps you are suggesting something like this (on top of this patch) - also
attached is the new patch with this change folded in.

diff --git a/xen/common/softirq.c b/xen/common/softirq.c
index 22e417a..2b90316 100644
--- a/xen/common/softirq.c
+++ b/xen/common/softirq.c
@@ -94,6 +94,14 @@ void cpumask_raise_softirq(const cpumask_t *mask, unsigned 
int nr)
         smp_send_event_check_mask(raise_mask);
 }
 
+void cpu_raise_softirq_ipi(unsigned int this_cpu, unsigned int cpu,
+                           unsigned int nr)
+{
+    if ( !per_cpu(batching, this_cpu) || in_irq() )
+        smp_send_event_check_cpu(cpu);
+    else
+        set_bit(nr, &per_cpu(batch_mask, this_cpu));
+}
 void cpu_raise_softirq(unsigned int cpu, unsigned int nr)
 {
     unsigned int this_cpu = smp_processor_id();
@@ -103,10 +111,7 @@ void cpu_raise_softirq(unsigned int cpu, unsigned int nr)
          || arch_skip_send_event_check(cpu) )
         return;
 
-    if ( !per_cpu(batching, this_cpu) || in_irq() )
-        smp_send_event_check_cpu(cpu);
-    else
-        set_bit(nr, &per_cpu(batch_mask, this_cpu));
+    cpu_raise_softirq_ipi(this_cpu, cpu, nr);
 }
 
 void cpu_raise_softirq_batch_begin(void)
diff --git a/xen/drivers/passthrough/io.c b/xen/drivers/passthrough/io.c
index 66869e9..ddbb890 100644
--- a/xen/drivers/passthrough/io.c
+++ b/xen/drivers/passthrough/io.c
@@ -249,14 +249,18 @@ int pt_irq_create_bind(
      */
     if ( pt_pirq_softirq_active(pirq_dpci) )
     {
+        unsigned int cpu;
+
         spin_unlock(&d->event_lock);
-        if ( pirq_dpci->cpu >= 0 && pirq_dpci->cpu != smp_processor_id() )
+
+        cpu = smp_processor_id();
+        if ( pirq_dpci->cpu >= 0 && pirq_dpci->cpu != cpu )
         {
             /*
-             * The 'raise_softirq_for' sets the CPU and raises the softirq bit
-             * so we do not need to set the target CPU's HVM_DPCI_SOFTIRQ.
+             * We do NOT want to wait for the remote CPU to run its course 
which
+             * could be a full guest time-slice. As such, send one IPI there.
              */
-            smp_send_event_check_cpu(pirq_dpci->cpu);
+            cpu_raise_softirq_ipi(cpu, pirq_dpci->cpu, HVM_DPCI_SOFTIRQ);
             pirq_dpci->cpu = -1;
         }
         cpu_relax();
diff --git a/xen/include/xen/softirq.h b/xen/include/xen/softirq.h
index 0895a16..16f7063 100644
--- a/xen/include/xen/softirq.h
+++ b/xen/include/xen/softirq.h
@@ -27,6 +27,8 @@ void open_softirq(int nr, softirq_handler handler);
 void softirq_init(void);
 
 void cpumask_raise_softirq(const cpumask_t *, unsigned int nr);
+void cpu_raise_softirq_ipi(unsigned int this_cpu, unsigned int cpu,
+                           unsigned int nr);
 void cpu_raise_softirq(unsigned int cpu, unsigned int nr);
 void raise_softirq(unsigned int nr);

Attachment: 0001-dpci-Replace-tasklet-with-an-softirq-v11.patch
Description: Text document

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