| Message ID | 1395756505-21573-1-git-send-email-david.vrabel@citrix.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
My idea was that the current code can't race with interrupt running on a different CPU, because if the interrupt was moved since the last napi_schedule (which scheduled NAPI on the same CPU as the interrupt), the kernel would make sure that the NAPI instance is moved along with it. However I couldn't find any trace of this in the kernel so far, but the current code actually works for me, even when I used a bash script to aggressively move the interrupts around while running. I've added David and Eric to the mailing, maybe they can quickly shed some light on this: how does the kernel make sure that if the interrupt is moved away from a CPU (e.g. by irqbalance), the NAPI instance already scheduled there won't race with it? Zoli On 25/03/14 14:08, David Vrabel wrote: > When the NAPI budget was not all used, xenvif_poll() would call > napi_complete() /after/ enabling the interrupt. This resulted in a > race between the napi_complete() and the napi_schedule() in the > interrupt handler. The use of local_irq_save/restore() avoided by > race iff the handler is running on the same CPU but not if it was > running on a different CPU. > > Fix this properly by calling napi_complete() before reenabling > interrupts (in the xenvif_check_rx_xenvif() call). > > Signed-off-by: David Vrabel <david.vrabel@citrix.com> > --- > drivers/net/xen-netback/interface.c | 28 ++-------------------------- > 1 files changed, 2 insertions(+), 26 deletions(-) > > diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c > index 7669d49..ee322d9 100644 > --- a/drivers/net/xen-netback/interface.c > +++ b/drivers/net/xen-netback/interface.c > @@ -65,32 +65,8 @@ static int xenvif_poll(struct napi_struct *napi, int budget) > work_done = xenvif_tx_action(vif, budget); > > if (work_done < budget) { > - int more_to_do = 0; > - unsigned long flags; > - > - /* It is necessary to disable IRQ before calling > - * RING_HAS_UNCONSUMED_REQUESTS. Otherwise we might > - * lose event from the frontend. > - * > - * Consider: > - * RING_HAS_UNCONSUMED_REQUESTS > - * <frontend generates event to trigger napi_schedule> > - * __napi_complete > - * > - * This handler is still in scheduled state so the > - * event has no effect at all. After __napi_complete > - * this handler is descheduled and cannot get > - * scheduled again. We lose event in this case and the ring > - * will be completely stalled. > - */ > - > - local_irq_save(flags); > - > - RING_FINAL_CHECK_FOR_REQUESTS(&vif->tx, more_to_do); > - if (!more_to_do) > - __napi_complete(napi); > - > - local_irq_restore(flags); > + napi_complete(napi); > + xenvif_check_rx_xenvif(vif); > } > > return work_done; > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
You forgot to target this patch to "net" tree in subject line. On Tue, Mar 25, 2014 at 02:08:25PM +0000, David Vrabel wrote: > When the NAPI budget was not all used, xenvif_poll() would call > napi_complete() /after/ enabling the interrupt. This resulted in a > race between the napi_complete() and the napi_schedule() in the > interrupt handler. The use of local_irq_save/restore() avoided by > race iff the handler is running on the same CPU but not if it was > running on a different CPU. > OK, I understand this issue now. You mentioned it in the other email which made me a bit confused. Just curious, how do you trigger this? By re-binding the interrupt to another CPU when xenvif_poll is running? I used to run irqbalance (the one that works with xen virtual interrupt) but could not trigger a race. Probably the race window is too small to trigger? > Fix this properly by calling napi_complete() before reenabling > interrupts (in the xenvif_check_rx_xenvif() call). > > Signed-off-by: David Vrabel <david.vrabel@citrix.com> > --- > drivers/net/xen-netback/interface.c | 28 ++-------------------------- > 1 files changed, 2 insertions(+), 26 deletions(-) > > diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c > index 7669d49..ee322d9 100644 > --- a/drivers/net/xen-netback/interface.c > +++ b/drivers/net/xen-netback/interface.c > @@ -65,32 +65,8 @@ static int xenvif_poll(struct napi_struct *napi, int budget) > work_done = xenvif_tx_action(vif, budget); > > if (work_done < budget) { > - int more_to_do = 0; > - unsigned long flags; > - > - /* It is necessary to disable IRQ before calling > - * RING_HAS_UNCONSUMED_REQUESTS. Otherwise we might > - * lose event from the frontend. > - * > - * Consider: > - * RING_HAS_UNCONSUMED_REQUESTS > - * <frontend generates event to trigger napi_schedule> > - * __napi_complete > - * > - * This handler is still in scheduled state so the > - * event has no effect at all. After __napi_complete > - * this handler is descheduled and cannot get > - * scheduled again. We lose event in this case and the ring > - * will be completely stalled. > - */ > - > - local_irq_save(flags); > - > - RING_FINAL_CHECK_FOR_REQUESTS(&vif->tx, more_to_do); > - if (!more_to_do) > - __napi_complete(napi); > - > - local_irq_restore(flags); > + napi_complete(napi); You need to add comment here to say interrupt is in fact "disabled" before this point, and "enabled" by xenvif_check_rx_xenvif(). > + xenvif_check_rx_xenvif(vif); To be honest this function call is not immediately obvious about it's side effect. I don't mind you copy the code in that function here. Wei. > } > > return work_done; > -- > 1.7.2.5 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 25/03/14 14:50, Wei Liu wrote: > You forgot to target this patch to "net" tree in subject line. > > On Tue, Mar 25, 2014 at 02:08:25PM +0000, David Vrabel wrote: >> When the NAPI budget was not all used, xenvif_poll() would call >> napi_complete() /after/ enabling the interrupt. This resulted in a >> race between the napi_complete() and the napi_schedule() in the >> interrupt handler. The use of local_irq_save/restore() avoided by >> race iff the handler is running on the same CPU but not if it was >> running on a different CPU. >> > > OK, I understand this issue now. You mentioned it in the other email > which made me a bit confused. > > Just curious, how do you trigger this? By re-binding the interrupt to > another CPU when xenvif_poll is running? I used to run irqbalance (the > one that works with xen virtual interrupt) but could not trigger a race. > Probably the race window is too small to trigger? I haven't seen the race occur, but I've not tried to trigger it. It may be that as Zoltan says, that the race is harmless but I still think avoiding the race entirely is preferable. David -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Wei Liu <wei.liu2@citrix.com> Date: Tue, 25 Mar 2014 14:50:21 +0000 > You forgot to target this patch to "net" tree in subject line. > > On Tue, Mar 25, 2014 at 02:08:25PM +0000, David Vrabel wrote: >> When the NAPI budget was not all used, xenvif_poll() would call >> napi_complete() /after/ enabling the interrupt. This resulted in a >> race between the napi_complete() and the napi_schedule() in the >> interrupt handler. The use of local_irq_save/restore() avoided by >> race iff the handler is running on the same CPU but not if it was >> running on a different CPU. >> > > OK, I understand this issue now. You mentioned it in the other email > which made me a bit confused. > > Just curious, how do you trigger this? By re-binding the interrupt to > another CPU when xenvif_poll is running? I used to run irqbalance (the > one that works with xen virtual interrupt) but could not trigger a race. > Probably the race window is too small to trigger? > >> Fix this properly by calling napi_complete() before reenabling >> interrupts (in the xenvif_check_rx_xenvif() call). >> >> Signed-off-by: David Vrabel <david.vrabel@citrix.com> >> --- >> drivers/net/xen-netback/interface.c | 28 ++-------------------------- >> 1 files changed, 2 insertions(+), 26 deletions(-) >> >> diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c >> index 7669d49..ee322d9 100644 >> --- a/drivers/net/xen-netback/interface.c >> +++ b/drivers/net/xen-netback/interface.c >> @@ -65,32 +65,8 @@ static int xenvif_poll(struct napi_struct *napi, int budget) >> work_done = xenvif_tx_action(vif, budget); >> >> if (work_done < budget) { >> - int more_to_do = 0; >> - unsigned long flags; >> - >> - /* It is necessary to disable IRQ before calling >> - * RING_HAS_UNCONSUMED_REQUESTS. Otherwise we might >> - * lose event from the frontend. >> - * >> - * Consider: >> - * RING_HAS_UNCONSUMED_REQUESTS >> - * <frontend generates event to trigger napi_schedule> >> - * __napi_complete >> - * >> - * This handler is still in scheduled state so the >> - * event has no effect at all. After __napi_complete >> - * this handler is descheduled and cannot get >> - * scheduled again. We lose event in this case and the ring >> - * will be completely stalled. >> - */ >> - >> - local_irq_save(flags); >> - >> - RING_FINAL_CHECK_FOR_REQUESTS(&vif->tx, more_to_do); >> - if (!more_to_do) >> - __napi_complete(napi); >> - >> - local_irq_restore(flags); >> + napi_complete(napi); > > You need to add comment here to say interrupt is in fact "disabled" > before this point, and "enabled" by xenvif_check_rx_xenvif(). Agreed. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Let me bump this question: how does the kernel make sure that if an interrupt is moved away from CPU X to Y (e.g. by irqbalance), the NAPI instance already scheduled on CPU Y won't race with the interrupt? I mean the following scenario: 1. instance calls napi_complete on CPU X 2. that removes the instance from the list 3. interrupt happens on CPU Y 4. "!test_and_set_bit(NAPI_STATE_SCHED, &n->state)" fails, as the bit is still set 5. instance not added to the list therefore 6. napi_complete on CPU X clear the bit My assumption is that when the interrupt moved, the kernel makes sure the NAPI instance won't get scheduled on the old CPU, but I'm not sure about it On 25/03/14 14:41, Zoltan Kiss wrote: > My idea was that the current code can't race with interrupt running on a > different CPU, because if the interrupt was moved since the last > napi_schedule (which scheduled NAPI on the same CPU as the interrupt), > the kernel would make sure that the NAPI instance is moved along with > it. However I couldn't find any trace of this in the kernel so far, but > the current code actually works for me, even when I used a bash script > to aggressively move the interrupts around while running. > I've added David and Eric to the mailing, maybe they can quickly shed > some light on this: how does the kernel make sure that if the interrupt > is moved away from a CPU (e.g. by irqbalance), the NAPI instance already > scheduled there won't race with it? > > Zoli > > On 25/03/14 14:08, David Vrabel wrote: >> When the NAPI budget was not all used, xenvif_poll() would call >> napi_complete() /after/ enabling the interrupt. This resulted in a >> race between the napi_complete() and the napi_schedule() in the >> interrupt handler. The use of local_irq_save/restore() avoided by >> race iff the handler is running on the same CPU but not if it was >> running on a different CPU. >> >> Fix this properly by calling napi_complete() before reenabling >> interrupts (in the xenvif_check_rx_xenvif() call). >> >> Signed-off-by: David Vrabel <david.vrabel@citrix.com> >> --- >> drivers/net/xen-netback/interface.c | 28 ++-------------------------- >> 1 files changed, 2 insertions(+), 26 deletions(-) >> >> diff --git a/drivers/net/xen-netback/interface.c >> b/drivers/net/xen-netback/interface.c >> index 7669d49..ee322d9 100644 >> --- a/drivers/net/xen-netback/interface.c >> +++ b/drivers/net/xen-netback/interface.c >> @@ -65,32 +65,8 @@ static int xenvif_poll(struct napi_struct *napi, >> int budget) >> work_done = xenvif_tx_action(vif, budget); >> >> if (work_done < budget) { >> - int more_to_do = 0; >> - unsigned long flags; >> - >> - /* It is necessary to disable IRQ before calling >> - * RING_HAS_UNCONSUMED_REQUESTS. Otherwise we might >> - * lose event from the frontend. >> - * >> - * Consider: >> - * RING_HAS_UNCONSUMED_REQUESTS >> - * <frontend generates event to trigger napi_schedule> >> - * __napi_complete >> - * >> - * This handler is still in scheduled state so the >> - * event has no effect at all. After __napi_complete >> - * this handler is descheduled and cannot get >> - * scheduled again. We lose event in this case and the ring >> - * will be completely stalled. >> - */ >> - >> - local_irq_save(flags); >> - >> - RING_FINAL_CHECK_FOR_REQUESTS(&vif->tx, more_to_do); >> - if (!more_to_do) >> - __napi_complete(napi); >> - >> - local_irq_restore(flags); >> + napi_complete(napi); >> + xenvif_check_rx_xenvif(vif); >> } >> >> return work_done; >> > > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2014-05-15 at 17:26 +0100, Zoltan Kiss wrote: > Hi, > > Let me bump this question: how does the kernel make sure that if an > interrupt is moved away from CPU X to Y (e.g. by irqbalance), the NAPI > instance already scheduled on CPU Y won't race with the interrupt? I > mean the following scenario: > > 1. instance calls napi_complete on CPU X > 2. that removes the instance from the list > 3. interrupt happens on CPU Y > 4. "!test_and_set_bit(NAPI_STATE_SCHED, &n->state)" fails, as the bit is > still set > 5. instance not added to the list therefore > 6. napi_complete on CPU X clear the bit > > My assumption is that when the interrupt moved, the kernel makes sure > the NAPI instance won't get scheduled on the old CPU, but I'm not sure > about it Interrupts are supposed to be disabled at the time we call napi_complete(). Some drivers are buggy. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c index 7669d49..ee322d9 100644 --- a/drivers/net/xen-netback/interface.c +++ b/drivers/net/xen-netback/interface.c @@ -65,32 +65,8 @@ static int xenvif_poll(struct napi_struct *napi, int budget) work_done = xenvif_tx_action(vif, budget); if (work_done < budget) { - int more_to_do = 0; - unsigned long flags; - - /* It is necessary to disable IRQ before calling - * RING_HAS_UNCONSUMED_REQUESTS. Otherwise we might - * lose event from the frontend. - * - * Consider: - * RING_HAS_UNCONSUMED_REQUESTS - * <frontend generates event to trigger napi_schedule> - * __napi_complete - * - * This handler is still in scheduled state so the - * event has no effect at all. After __napi_complete - * this handler is descheduled and cannot get - * scheduled again. We lose event in this case and the ring - * will be completely stalled. - */ - - local_irq_save(flags); - - RING_FINAL_CHECK_FOR_REQUESTS(&vif->tx, more_to_do); - if (!more_to_do) - __napi_complete(napi); - - local_irq_restore(flags); + napi_complete(napi); + xenvif_check_rx_xenvif(vif); } return work_done;
When the NAPI budget was not all used, xenvif_poll() would call napi_complete() /after/ enabling the interrupt. This resulted in a race between the napi_complete() and the napi_schedule() in the interrupt handler. The use of local_irq_save/restore() avoided by race iff the handler is running on the same CPU but not if it was running on a different CPU. Fix this properly by calling napi_complete() before reenabling interrupts (in the xenvif_check_rx_xenvif() call). Signed-off-by: David Vrabel <david.vrabel@citrix.com> --- drivers/net/xen-netback/interface.c | 28 ++-------------------------- 1 files changed, 2 insertions(+), 26 deletions(-)