| Message ID | 1432077893-4431-2-git-send-email-baptiste@arista.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Baptiste Covolato <baptiste@arista.com> Date: Tue, 19 May 2015 16:24:51 -0700 > Update flush_backlog to flush all packets in the backlog queue belonging > to a device being unregistered. Accordingly on_each_cpu no longer needs > to pass a device to flush_backlog since it handles any device in the > NETREG_UNREGISTERED state. > > Signed-off-by: Baptiste Covolato <baptiste@arista.com> > Signed-off-by: Francesco Ruggeri <fruggeri@arista.com> This is quite bogus if you ask me. This is the one spot causing a device to make the transition to unregistered state, so passing that specific device to flush_backlog() is the completely logical way to handle this. If this is some hack that is made necessary by your parallel notification scheme, I do not find it acceptable. I'm not applying this series, sorry. -- 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, May 21, 2015 at 2:02 PM, David Miller <davem@davemloft.net> wrote: > > From: Baptiste Covolato <baptiste@arista.com> > Date: Tue, 19 May 2015 16:24:51 -0700 > > > Update flush_backlog to flush all packets in the backlog queue belonging > > to a device being unregistered. Accordingly on_each_cpu no longer needs > > to pass a device to flush_backlog since it handles any device in the > > NETREG_UNREGISTERED state. > > > > Signed-off-by: Baptiste Covolato <baptiste@arista.com> > > Signed-off-by: Francesco Ruggeri <fruggeri@arista.com> > > This is quite bogus if you ask me. > > This is the one spot causing a device to make the transition > to unregistered state, so passing that specific device to > flush_backlog() is the completely logical way to handle this. > > If this is some hack that is made necessary by your parallel > notification scheme, I do not find it acceptable. Using NETREG_UNREGISTERED is not needed in order for the parallel scheme to work. One can change flush_backlog to also handle a list of net_devices instead. In both cases on_each_cpu is invoked only once instead of once per net_device. In case of the latter though more time is spent in flush_backlog, since each packet has to be compared against all interfaces being deleted. We tried both approaches and they both worked for us. Using NETREG_UNREGISTERED can result in a task flushing packets from interfaces that another task may be in the process of deleting, but that should not be a problem since those packets are expected to be flushed anyway and access to the backlog lists is protected. Francesco > > I'm not applying this series, sorry. -- 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: Francesco Ruggeri <fruggeri@arista.com> Date: Thu, 21 May 2015 17:28:47 -0700 > One can change flush_backlog to also handle a list of net_devices > instead. Then make it do so. Because that is the driving design of all of the bulk deregister/etc. facilities, they operate on a list of objects and the top-level control is responsible for the lifetime of the list. -- 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/net/core/dev.c b/net/core/dev.c index 0e7afef..db59d18 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -4005,13 +4005,12 @@ EXPORT_SYMBOL(netif_receive_skb_sk); */ static void flush_backlog(void *arg) { - struct net_device *dev = arg; struct softnet_data *sd = this_cpu_ptr(&softnet_data); struct sk_buff *skb, *tmp; rps_lock(sd); skb_queue_walk_safe(&sd->input_pkt_queue, skb, tmp) { - if (skb->dev == dev) { + if (skb->dev->reg_state == NETREG_UNREGISTERED) { __skb_unlink(skb, &sd->input_pkt_queue); kfree_skb(skb); input_queue_head_incr(sd); @@ -4020,7 +4019,7 @@ static void flush_backlog(void *arg) rps_unlock(sd); skb_queue_walk_safe(&sd->process_queue, skb, tmp) { - if (skb->dev == dev) { + if (skb->dev->reg_state == NETREG_UNREGISTERED) { __skb_unlink(skb, &sd->process_queue); kfree_skb(skb); input_queue_head_incr(sd); @@ -6790,7 +6789,7 @@ void netdev_run_todo(void) dev->reg_state = NETREG_UNREGISTERED; - on_each_cpu(flush_backlog, dev, 1); + on_each_cpu(flush_backlog, NULL, 1); netdev_wait_allrefs(dev);