Re: [PATCH] Fix locking in flush_backlog

[Tom Herbert <therbert@google.com>]

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