| Message ID | alpine.DEB.1.00.1003222257470.3718@pokey.mtv.corp.google.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Le lundi 22 mars 2010 à 23:04 -0700, Tom Herbert a écrit : > Need to take spinlocks when dequeuing from input_pkt_queue in > flush_backlog. Also, with the spinlock the backlog queues can > be flushed directly from netdev_run_todo. > > Signed-off-by: Tom Herbert <therbert@google.com> > --- > diff --git a/net/core/dev.c b/net/core/dev.c > index a03aab4..e7db656 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -2765,20 +2765,6 @@ int netif_receive_skb(struct sk_buff *skb) > } > EXPORT_SYMBOL(netif_receive_skb); > > -/* Network device is going away, flush any packets still pending */ > -static void flush_backlog(void *arg) > -{ > - struct net_device *dev = arg; > - struct softnet_data *queue = &__get_cpu_var(softnet_data); > - struct sk_buff *skb, *tmp; > - > - skb_queue_walk_safe(&queue->input_pkt_queue, skb, tmp) > - if (skb->dev == dev) { > - __skb_unlink(skb, &queue->input_pkt_queue); > - kfree_skb(skb); > - } > -} > - > static int napi_gro_complete(struct sk_buff *skb) > { > struct packet_type *ptype; > @@ -5545,6 +5531,7 @@ void netdev_run_todo(void) > while (!list_empty(&list)) { > struct net_device *dev > = list_first_entry(&list, struct net_device, todo_list); > + int i; > list_del(&dev->todo_list); > > if (unlikely(dev->reg_state != NETREG_UNREGISTERING)) { > @@ -5556,7 +5543,22 @@ void netdev_run_todo(void) > > dev->reg_state = NETREG_UNREGISTERED; > > - on_each_cpu(flush_backlog, dev, 1); > + /* Flush backlog queues of any pending packets */ > + for_each_online_cpu(i) { > + struct softnet_data *queue = &per_cpu(softnet_data, i); > + struct sk_buff *skb, *tmp; > + unsigned long flags; > + > + spin_lock_irqsave(&queue->input_pkt_queue.lock, flags); > + skb_queue_walk_safe(&queue->input_pkt_queue, skb, tmp) > + if (skb->dev == dev) { > + __skb_unlink(skb, > + &queue->input_pkt_queue); > + kfree_skb(skb); > + } > + spin_unlock_irqrestore(&queue->input_pkt_queue.lock, > + flags); > + } > > netdev_wait_allrefs(dev); > OK (this is a patch for net-next-2.6) Could you please keep the function, to ease netdev_run_todo() review ? Thanks -- 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 Mon, Mar 22, 2010 at 11:29 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > Le lundi 22 mars 2010 à 23:04 -0700, Tom Herbert a écrit : >> Need to take spinlocks when dequeuing from input_pkt_queue in >> flush_backlog. Also, with the spinlock the backlog queues can >> be flushed directly from netdev_run_todo. >> >> Signed-off-by: Tom Herbert <therbert@google.com> >> --- >> diff --git a/net/core/dev.c b/net/core/dev.c >> index a03aab4..e7db656 100644 >> --- a/net/core/dev.c >> +++ b/net/core/dev.c >> @@ -2765,20 +2765,6 @@ int netif_receive_skb(struct sk_buff *skb) >> } >> EXPORT_SYMBOL(netif_receive_skb); >> >> -/* Network device is going away, flush any packets still pending */ >> -static void flush_backlog(void *arg) >> -{ >> - struct net_device *dev = arg; >> - struct softnet_data *queue = &__get_cpu_var(softnet_data); >> - struct sk_buff *skb, *tmp; >> - >> - skb_queue_walk_safe(&queue->input_pkt_queue, skb, tmp) >> - if (skb->dev == dev) { >> - __skb_unlink(skb, &queue->input_pkt_queue); >> - kfree_skb(skb); >> - } >> -} >> - >> static int napi_gro_complete(struct sk_buff *skb) >> { >> struct packet_type *ptype; >> @@ -5545,6 +5531,7 @@ void netdev_run_todo(void) >> while (!list_empty(&list)) { >> struct net_device *dev >> = list_first_entry(&list, struct net_device, todo_list); >> + int i; >> list_del(&dev->todo_list); >> >> if (unlikely(dev->reg_state != NETREG_UNREGISTERING)) { >> @@ -5556,7 +5543,22 @@ void netdev_run_todo(void) >> >> dev->reg_state = NETREG_UNREGISTERED; >> >> - on_each_cpu(flush_backlog, dev, 1); >> + /* Flush backlog queues of any pending packets */ >> + for_each_online_cpu(i) { >> + struct softnet_data *queue = &per_cpu(softnet_data, i); >> + struct sk_buff *skb, *tmp; >> + unsigned long flags; >> + >> + spin_lock_irqsave(&queue->input_pkt_queue.lock, flags); >> + skb_queue_walk_safe(&queue->input_pkt_queue, skb, tmp) >> + if (skb->dev == dev) { >> + __skb_unlink(skb, >> + &queue->input_pkt_queue); >> + kfree_skb(skb); >> + } >> + spin_unlock_irqrestore(&queue->input_pkt_queue.lock, >> + flags); >> + } >> >> netdev_wait_allrefs(dev); >> > > OK (this is a patch for net-next-2.6) > > Could you please keep the function, to ease netdev_run_todo() review ? > Eric, I'm not sure what you're asking. Do you just want to add spinlocks in the flush_backlog function without changing the mechanism to call the function on each CPU, or keep flush_backlog but call it from netdev_run_todo for each queue? Tom -- 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
Le mardi 23 mars 2010 à 08:56 -0700, Tom Herbert a écrit : > Eric, > > I'm not sure what you're asking. Do you just want to add spinlocks in > the flush_backlog function without changing the mechanism to call the > function on each CPU, or keep flush_backlog but call it from > netdev_run_todo for each queue? > keep flush_backlog() so that its role is obvious and indentation level not too big. static void flush_backlog(int cpu) { struct softnet_data *queue = &per_cpu(softnet_data, cpu); struct sk_buff *skb, *tmp; unsigned long flags; spin_lock_irqsave(&queue->input_pkt_queue.lock, flags); skb_queue_walk_safe(&queue->input_pkt_queue, skb, tmp) if (skb->dev == dev) { __skb_unlink(skb, &queue->input_pkt_queue); kfree_skb(skb); } spin_unlock_irqrestore(&queue->input_pkt_queue.lock, flags); } And call it from netdev_run_todo() : for_each_online_cpu(i) flush_backlog(i); This adds two lines to netdev_run_todo() only. Thanks -- 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 a03aab4..e7db656 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -2765,20 +2765,6 @@ int netif_receive_skb(struct sk_buff *skb) } EXPORT_SYMBOL(netif_receive_skb); -/* Network device is going away, flush any packets still pending */ -static void flush_backlog(void *arg) -{ - struct net_device *dev = arg; - struct softnet_data *queue = &__get_cpu_var(softnet_data); - struct sk_buff *skb, *tmp; - - skb_queue_walk_safe(&queue->input_pkt_queue, skb, tmp) - if (skb->dev == dev) { - __skb_unlink(skb, &queue->input_pkt_queue); - kfree_skb(skb); - } -} - static int napi_gro_complete(struct sk_buff *skb) { struct packet_type *ptype; @@ -5545,6 +5531,7 @@ void netdev_run_todo(void) while (!list_empty(&list)) { struct net_device *dev = list_first_entry(&list, struct net_device, todo_list); + int i; list_del(&dev->todo_list); if (unlikely(dev->reg_state != NETREG_UNREGISTERING)) { @@ -5556,7 +5543,22 @@ void netdev_run_todo(void) dev->reg_state = NETREG_UNREGISTERED; - on_each_cpu(flush_backlog, dev, 1); + /* Flush backlog queues of any pending packets */ + for_each_online_cpu(i) { + struct softnet_data *queue = &per_cpu(softnet_data, i); + struct sk_buff *skb, *tmp; + unsigned long flags; + + spin_lock_irqsave(&queue->input_pkt_queue.lock, flags); + skb_queue_walk_safe(&queue->input_pkt_queue, skb, tmp) + if (skb->dev == dev) { + __skb_unlink(skb, + &queue->input_pkt_queue); + kfree_skb(skb); + } + spin_unlock_irqrestore(&queue->input_pkt_queue.lock, + flags); + } netdev_wait_allrefs(dev);
Need to take spinlocks when dequeuing from input_pkt_queue in flush_backlog. Also, with the spinlock the backlog queues can be flushed directly from netdev_run_todo. Signed-off-by: Tom Herbert <therbert@google.com> --- -- 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