[PATCH] xen-netback: fix race between napi_complete() and interrupt handler

[PATCH] xen-netback: fix race between napi_complete() and interrupt handler

[David Vrabel]

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;
-- 
1.7.2.5

Re: [PATCH] xen-netback: fix race between napi_complete() and interrupt handler

[Zoltan Kiss]

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;
>

interrupt moving and NAPI scheduling

[Zoltan Kiss]

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

Re: interrupt moving and NAPI scheduling

[Eric Dumazet]

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.

Re: [PATCH] xen-netback: fix race between napi_complete() and interrupt handler

[Wei Liu]

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

Re: [PATCH] xen-netback: fix race between napi_complete() and interrupt handler

[David Vrabel]

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

Re: [PATCH] xen-netback: fix race between napi_complete() and interrupt handler

[David Miller]

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.