net: rps: fix data stall after hotplug

PATCH In review [email protected] 2 participants 11 messages Mar 19, 2015 – Mar 31, 2015

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

[email protected]

author

2015-03-19 19:54 UTC #0 lore raw

PATCH 1/1 net: rps: fix data stall after hotplug

When RPS is enabled, IPI is triggered to enqueue the backlog NAPI to the poll list. If the CPU is hotplugged after the NAPI_STATE_SCHED bit is set on enqueue_to_backlog but before the IPI is delivered successfully, the poll list does not have the backlog NAPI queued. As a consequence of this, dev_cpu_callback does not clear the NAPI_STATE_SCHED bit on hotplug. Since NAPI_STATE_SCHED is set even after the cpu comes back up, packets get enqueued onto the input packet queue but are never processed since the IPI will not be triggered.

This patch handles this race by unconditionally resetting the NAPI state for the backlog NAPI on the offline CPU.

Signed-off-by Subash Abhinov Kasiviswanathan <[email protected]>

---
 net/core/dev.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

net/core/dev.c +0 -0

index 6f561de..61d9579 100644
@@ -7119,12 +7119,11 @@ static int dev_cpu_callback(struct notifier_block

*nfb,
poll_list);

list_del_init(&napi->poll_list);
- if (napi->poll == process_backlog)
- napi->state = 0;
- else
+ if (napi->poll != process_backlog)
____napi_schedule(sd, napi);
}

+ oldsd->backlog.state = 0;
raise_softirq_irqoff(NET_TX_SOFTIRQ); local_irq_enable();

--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Eric Dumazet

2015-03-19 21:50 UTC #1 lore raw

Quoted 28 lines · On Thu, 2015-03-19 at 19:54 +0000, [email protected] wrote:

When RPS is enabled, IPI is triggered to enqueue the backlog NAPI to the poll list. If the CPU is hotplugged after the NAPI_STATE_SCHED bit is set on enqueue_to_backlog but before the IPI is delivered successfully, the poll list does not have the backlog NAPI queued. As a consequence of this, dev_cpu_callback does not clear the NAPI_STATE_SCHED bit on hotplug. Since NAPI_STATE_SCHED is set even after the cpu comes back up, packets get enqueued onto the input packet queue but are never processed since the IPI will not be triggered.
This patch handles this race by unconditionally resetting the NAPI state for the backlog NAPI on the offline CPU.
Signed-off-by Subash Abhinov Kasiviswanathan <[email protected]>
---
 net/core/dev.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
net/core/dev.c +0 -0
index 6f561de..61d9579 100644
@@ -7119,12 +7119,11 @@ static int dev_cpu_callback(struct notifier_block
 *nfb,
 							    poll_list);
 
 		list_del_init(&napi->poll_list);

net/core/dev.c static int dev_cpu_callback(struct notifier_block · line 7129

Quoted 10 lines

net/core/dev.c +2 -3

-		if (napi->poll == process_backlog)
-			napi->state = 0;
-		else
+		if (napi->poll != process_backlog)
 			____napi_schedule(sd, napi);
 	}
 
+	oldsd->backlog.state = 0;
 	raise_softirq_irqoff(NET_TX_SOFTIRQ);
 	local_irq_enable();

Are you seeing this race on x86 ?

If IPI are not reliable on your arch, I am guessing you should fix them.

Otherwise, even without hotplug you'll have hangs.

Eric Dumazet

2015-03-20 11:50 UTC #2 lore raw

Quoted 5 lines · Eric Dumazet

Are you seeing this race on x86 ?
If IPI are not reliable on your arch, I am guessing you should fix them.
Otherwise, even without hotplug you'll have hangs.

Please try instead this patch :

net/core/dev.c +2 -3

index 5d43e010ef870a6ab92895297fe18d6e6a03593a..baa4bff9a6fbe0d77d7921865c038060cb5efffd 100644
@@ -4320,9 +4320,8 @@ static void net_rps_action_and_irq_enable(struct softnet_data *sd)
 		while (remsd) {
 			struct softnet_data *next = remsd->rps_ipi_next;
 
-			if (cpu_online(remsd->cpu))
-				smp_call_function_single_async(remsd->cpu,
-							   &remsd->csd);
+			smp_call_function_single_async(remsd->cpu,
+						       &remsd->csd);
 			remsd = next;
 		}
 	} else

[email protected]

author

2015-03-20 16:40 UTC #3 lore raw

Quoted 31 lines · Eric Dumazet

Quoted 6 lines
Are you seeing this race on x86 ?
If IPI are not reliable on your arch, I am guessing you should fix them.
Otherwise, even without hotplug you'll have hangs.
Please try instead this patch :
net/core/dev.c +0 -0
index 5d43e010ef870a6ab92895297fe18d6e6a03593a..baa4bff9a6fbe0d77d7921865c038060cb5efffd 100644
net/core/dev.c +2 -3
@@ -4320,9 +4320,8 @@ static void net_rps_action_and_irq_enable(struct
 softnet_data *sd)
 		while (remsd) {
 			struct softnet_data *next = remsd->rps_ipi_next;
 
-			if (cpu_online(remsd->cpu))
-				smp_call_function_single_async(remsd->cpu,
-							   &remsd->csd);
+			smp_call_function_single_async(remsd->cpu,
+						       &remsd->csd);
 			remsd = next;
 		}
} else

Quoted 5 lines

--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Thanks for the patch Eric. We are seeing this race on ARM. I will try this and update.

--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[email protected]

author

2015-03-23 22:16 UTC #4 lore raw

Quoted 22 lines

Quoted 22 lines · Eric Dumazet
Quoted 7 lines
Are you seeing this race on x86 ?
If IPI are not reliable on your arch, I am guessing you should fix them.
Otherwise, even without hotplug you'll have hangs.
Please try instead this patch :
net/core/dev.c +0 -0
index 5d43e010ef870a6ab92895297fe18d6e6a03593a..baa4bff9a6fbe0d77d7921865c038060cb5efffd 100644
net/core/dev.c +0 -0
@@ -4320,9 +4320,8 @@ static void net_rps_action_and_irq_enable(struct
 softnet_data *sd)
 		while (remsd) {
 			struct softnet_data *next = remsd->rps_ipi_next;
 

Quoted 13 lines

Quoted 10 lines
net/core/dev.c +2 -3
-			if (cpu_online(remsd->cpu))
-				smp_call_function_single_async(remsd->cpu,
-							   &remsd->csd);
+			smp_call_function_single_async(remsd->cpu,
+						       &remsd->csd);
 			remsd = next;
 		}
} else
Thanks for the patch Eric. We are seeing this race on ARM. I will try this and update.

Unfortunately, I am not able to reproduce data stall now with or without the patch. Could you tell me more about the patch and what issue you were suspecting?

Based on the code, it looks like we BUG out on our arch if we try to call an IPI on an offline CPU. Since this condition is never hit, I feel that the IPI might not have failed.

void smp_send_reschedule(int cpu)
{
BUG_ON(cpu_is_offline(cpu)); smp_cross_call_common(cpumask_of(cpu), IPI_RESCHEDULE); }

--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Eric Dumazet

2015-03-23 22:29 UTC #5 lore raw

Quoted 44 lines · On Mon, 2015-03-23 at 22:16 +0000, [email protected] wrote:

Quoted 22 lines
Quoted 22 lines · Eric Dumazet
Quoted 7 lines
Are you seeing this race on x86 ?
If IPI are not reliable on your arch, I am guessing you should fix them.
Otherwise, even without hotplug you'll have hangs.
Please try instead this patch :
net/core/dev.c +0 -0
index 5d43e010ef870a6ab92895297fe18d6e6a03593a..baa4bff9a6fbe0d77d7921865c038060cb5efffd 100644
net/core/dev.c +0 -0
@@ -4320,9 +4320,8 @@ static void net_rps_action_and_irq_enable(struct
 softnet_data *sd)
 		while (remsd) {
 			struct softnet_data *next = remsd->rps_ipi_next;
 
Quoted 13 lines
Quoted 10 lines
net/core/dev.c +2 -3
-			if (cpu_online(remsd->cpu))
-				smp_call_function_single_async(remsd->cpu,
-							   &remsd->csd);
+			smp_call_function_single_async(remsd->cpu,
+						       &remsd->csd);
 			remsd = next;
 		}
} else
Thanks for the patch Eric. We are seeing this race on ARM. I will try this and update.
Unfortunately, I am not able to reproduce data stall now with or without the patch. Could you tell me more about the patch and what issue you were suspecting?
Based on the code, it looks like we BUG out on our arch if we try to call an IPI on an offline CPU. Since this condition is never hit, I feel that the IPI might not have failed.

Quoted 5 lines

void smp_send_reschedule(int cpu)

{
        BUG_ON(cpu_is_offline(cpu));
        smp_cross_call_common(cpumask_of(cpu), IPI_RESCHEDULE);
}

The bug I am fixing is the following :

if (cpu_is_online(x))
target = x

...

queue packet on queue of cpu x

net_rps_action_and_irq_enable()

if (cpu_is_online(x)) [2]

    smp_call_function_single_async(x, ...)

Problem is that first test in [1] can succeed, but second in [2] can fail.

But we should still send this IPI.

We run in a softirq, so it is OK to deliver the IPI to the _about to be offlined_ cpu.

We should test the cpu_is_online(x) once.

Doing this a second time is the bug.

[email protected]

author

2015-03-25 18:54 UTC #6 lore raw

Quoted 77 lines · On Mon, 2015-03-23 at 22:16 +0000, [email protected] wrote:

Quoted 47 lines
Quoted 11 lines
Quoted 11 lines · Eric Dumazet
Quoted 7 lines
Are you seeing this race on x86 ?
If IPI are not reliable on your arch, I am guessing you should fix them.
Otherwise, even without hotplug you'll have hangs.
Please try instead this patch :
Quoted 7 lines
net/core/dev.c static void
Quoted 7 lines
net/core/dev.c +0 -0
index 5d43e010ef870a6ab92895297fe18d6e6a03593a..baa4bff9a6fbe0d77d7921865c038060cb5efffd 100644
net/core/dev.c +0 -0
@@ -4320,9 +4320,8 @@ static void
net_rps_action_and_irq_enable(struct
Quoted 4 lines
Quoted 4 lines
net/core/dev.c +0 -0
 softnet_data *sd)
 		while (remsd) {
 			struct softnet_data *next = remsd->rps_ipi_next;
 
Quoted 13 lines
Quoted 10 lines
net/core/dev.c +2 -3
-			if (cpu_online(remsd->cpu))
-				smp_call_function_single_async(remsd->cpu,
-							   &remsd->csd);
+			smp_call_function_single_async(remsd->cpu,
+						       &remsd->csd);
 			remsd = next;
 		}
} else
Thanks for the patch Eric. We are seeing this race on ARM. I will try this and update.
Unfortunately, I am not able to reproduce data stall now with or without the patch. Could you tell me more about the patch and what issue you were
suspecting?
Based on the code, it looks like we BUG out on our arch if we try to call
an IPI on an offline CPU. Since this condition is never hit, I feel that the IPI might not have failed.
Quoted 5 lines
void smp_send_reschedule(int cpu)
{
        BUG_ON(cpu_is_offline(cpu));
        smp_cross_call_common(cpumask_of(cpu), IPI_RESCHEDULE);
}
The bug I am fixing is the following :
if (cpu_is_online(x))
target = x
...
queue packet on queue of cpu x
net_rps_action_and_irq_enable()
if (cpu_is_online(x)) [2]
    smp_call_function_single_async(x, ...)
Problem is that first test in [1] can succeed, but second in [2] can fail.

Quoted 8 lines

But we should still send this IPI.
We run in a softirq, so it is OK to deliver the IPI to the _about to be offlined_ cpu.
We should test the cpu_is_online(x) once.
Doing this a second time is the bug.

Thanks for the explanation. It looks like the issue is related to the way our driver is designed.

We have a legacy driver which does not use the NAPI framework (and cannot be modified for reasons beyond my control). They rely on an interrupt mitigation system which disables hardware interrupts after receiving the interrupt for the first packet and then enters a polling mode for a duration and queues packets up to the network stack using netif_rx(). We have observed that the the time between softirq raised in the worker thread context and softirq entry in softirq context is around 1-3 milliseconds (local pending softirq's might be triggered after exit of a hardware irq).
When we used netif_rx_ni() instead, the delay between softirq raise and entry is around microseconds since the local pending irq's are services immediately. We have modified the driver to use netif_rx_ni() now.

With netif_rx() I was able to reproduce the data stall consistently. Upon hotplug, I was able to see that the cpu_is_online(x) check in [1 - get_rps_cpus] returned the cpu as mentioned in the rps mask but the check in [2 - net_rps_action_and_irq_enable] returned that the cpu was offline. After I applied your patch, I was crashing since my arch explicitly BUGs out on sending IPI's on offline CPU's.

With netif_rx_ni(), I have not run into any issue as of now both with and without your patch since both [1] and [2] might have been returning the same the value for cpu_is_online(x). However, it is possible that this race occurs with a much lesser chance. I would still see the data stall if [1] returns online while [2] returns the cpu as offline. Would it be acceptable to reset NAPI state in [2] in this case?

--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[email protected]

author

2015-03-30 23:49 UTC #7 lore raw

Quoted 14 lines

Quoted 14 lines
Quoted 14 lines
Quoted 9 lines
net/core/dev.c static void
Quoted 9 lines
Please try instead this patch :
net/core/dev.c +0 -0
index 5d43e010ef870a6ab92895297fe18d6e6a03593a..baa4bff9a6fbe0d77d7921865c038060cb5efffd 100644
net/core/dev.c +0 -0
@@ -4320,9 +4320,8 @@ static void
net_rps_action_and_irq_enable(struct
Quoted 4 lines
Quoted 4 lines
net/core/dev.c +0 -0
 softnet_data *sd)
 		while (remsd) {
 			struct softnet_data *next = remsd->rps_ipi_next;
 

Quoted 10 lines

net/core/dev.c +2 -3

-			if (cpu_online(remsd->cpu))
-				smp_call_function_single_async(remsd->cpu,
-							   &remsd->csd);
+			smp_call_function_single_async(remsd->cpu,
+						       &remsd->csd);
 			remsd = next;
 		}

} else

Hi Eric

While the original issue of data stall due to missing IPI is no longer seen with netif_rx_ni(), the scenario of rps cpu online in [1 - get_rps_cpus] but offline in [2 - net_rps_action_and_irq_enable] could still occur. Using your patch, triggering an IPI on an offline cpu in [2] leads to a crash on my arch.

I would like to know your thoughts on how to fix this race. Could the patch which I had initially proposed help here. Alternatively, is it correct to reset NAPI state and increment dropped sd count if an offline CPU is detected in [2].

--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Eric Dumazet

2015-03-31 04:48 UTC #8 lore raw

Quoted 8 lines · On Mon, 2015-03-30 at 23:49 +0000, [email protected] wrote:

Hi Eric
While the original issue of data stall due to missing IPI is no longer seen with netif_rx_ni(), the scenario of rps cpu online in [1 - get_rps_cpus] but offline in [2 - net_rps_action_and_irq_enable] could still occur. Using your patch, triggering an IPI on an offline cpu in [2] leads to a crash on my arch.

Quoted 5 lines

I would like to know your thoughts on how to fix this race. Could the patch which I had initially proposed help here. Alternatively, is it correct to reset NAPI state and increment dropped sd count if an offline CPU is detected in [2].

Listen, I would rather disable RPS on your arch, instead of messing with it.

Reset NAPI state as you did is in direct violation of the rules.

Only cpu owning the bit is allowed to reset it.

[email protected]

author

2015-03-31 22:02 UTC #9 lore raw

Quoted 7 lines

Listen, I would rather disable RPS on your arch, instead of messing with it.
Reset NAPI state as you did is in direct violation of the rules.
Only cpu owning the bit is allowed to reset it.

Perhaps my understanding of the code in dev_cpu_callback() is incorrect? Please correct me if I am wrong.

The poll list is copied from an offline cpu to an online cpu. Specifically for process_backlog, I was under the impression that the online cpu tries to reset the state of NAPI of the offline cpu. The process and input queues are then always copied to the online cpu.

while (!list_empty(&oldsd->poll_list)) {
	struct napi_struct *napi = list_first_entry(&oldsd->poll_list,
						    struct napi_struct,
							     poll_list);

	list_del_init(&napi->poll_list);
	if (napi->poll == process_backlog)
		napi->state = 0;
	else
		____napi_schedule(sd, napi);
}

My request was to know why it would be incorrect to clear the offline cpu backlog NAPI state unconditionally.

Eric Dumazet

2015-03-31 23:44 UTC #10 lore raw

Quoted 29 lines · On Tue, 2015-03-31 at 22:02 +0000, [email protected] wrote:

Quoted 7 lines
Listen, I would rather disable RPS on your arch, instead of messing with it.
Reset NAPI state as you did is in direct violation of the rules.
Only cpu owning the bit is allowed to reset it.
Perhaps my understanding of the code in dev_cpu_callback() is incorrect? Please correct me if I am wrong.
The poll list is copied from an offline cpu to an online cpu. Specifically for process_backlog, I was under the impression that the online cpu tries to reset the state of NAPI of the offline cpu. The process and input queues are then always copied to the online cpu.
while (!list_empty(&oldsd->poll_list)) {
	struct napi_struct *napi = list_first_entry(&oldsd->poll_list,
						    struct napi_struct,
							     poll_list);

	list_del_init(&napi->poll_list);
	if (napi->poll == process_backlog)
		napi->state = 0;
	else
		____napi_schedule(sd, napi);
}

Quoted 3 lines

My request was to know why it would be incorrect to clear the offline cpu backlog NAPI state unconditionally.

It is incorrect because the moment we chose to send an IPI to a cpu, we effectively transfered NAPI bit ownership to this target cpu. Another cpu cannot take over without risking tricky corruptions.

If your arch fails to send the IPI, we have no way to clear the bit in a safe way.

Only target cpu is allowed to clear the bit by virtue of following being called :

/* Called from hardirq (IPI) context */
static void rps_trigger_softirq(void *data)

{
        struct softnet_data *sd = data;

        ____napi_schedule(sd, &sd->backlog);
        sd->received_rps++;
}

If it is not called even in 0.000001% of the cases, all bets are off.

We are not going to add yet another atomic ops for every packet just to solve this corner case. Just do not enable RPS on your arch, as it is by default not enabled.

I am currently in vacations, I will not reply to another inquiry on this topic.