Patchwork Fix locking in flush_backlog

login
register
mail settings
Submitter Tom Herbert
Date March 23, 2010, 6:04 a.m.
Message ID <alpine.DEB.1.00.1003222257470.3718@pokey.mtv.corp.google.com>
Download mbox | patch
Permalink /patch/48321/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Tom Herbert - March 23, 2010, 6:04 a.m.
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
Eric Dumazet - March 23, 2010, 6:29 a.m.
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
Tom Herbert - March 23, 2010, 3:56 p.m.
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
Eric Dumazet - March 23, 2010, 4:14 p.m.
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

Patch

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