[RFC,2/2] igb: skb recycling

This driver is multiqueue, so implement a small skb recycling queue per
cpu. It doesn't make sense to have a global queue since then a lock would
be required. Not sure if this is going to work; compile tested only, needs more evaluation

Signed-off-by: Stephen Hemminger <shemminger@vyatta.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

Stephen Hemminger wrote:
> This driver is multiqueue, so implement a small skb recycling queue
> per cpu. It doesn't make sense to have a global queue since then a
> lock would be required. Not sure if this is going to work; compile
> tested only, needs more evaluation

I did a bit of quick testing and found an issue in the sections of code below.  As is the current patch generates a null pointer exception on unload because the percpu_alloc and for_each_possible_cpu are working off of two different CPU masks.

The simple solution is to either replace percpu_alloc with alloc_percpu and use cpu_possible_map or replace for_each_possible_cpu with for_each_online_cpu and use the cpu_online_map.

Attached is the version I tested at my desk that used the cpu_possible_map solution.  I went that route since alloc_percpu seemed like a fairly clean macro.

Thanks,

Alex

> --- a/drivers/net/igb/igb_main.c        2008-10-21 09:39:25.000000000
> -0700 +++ b/drivers/net/igb/igb_main.c        2008-10-21
> 10:13:42.000000000 -0700 @@ -824,6 +824,11 @@ void igb_down(struct
>                 igb_adapter *adapte igb_reset(adapter);
>         igb_clean_all_tx_rings(adapter);
>         igb_clean_all_rx_rings(adapter);
> +
> +       for_each_possible_cpu(i) {
> +               struct sk_buff_head *rx_recycle =
> per_cpu_ptr(adapter->rx_recycle,i); +
> __skb_queue_purge(rx_recycle); +       }
>  }
>
>  void igb_reinit_locked(struct igb_adapter *adapter)
> @@ -1022,6 +1027,11 @@ static int __devinit igb_probe(struct pc
>         adapter = netdev_priv(netdev);
>         adapter->netdev = netdev;
>         adapter->pdev = pdev;
> +
> +       adapter->rx_recycle = percpu_alloc(sizeof(struct
> sk_buff_head), GFP_KERNEL); +       if (!adapter->rx_recycle)
> +               goto err_alloc_recycle;
> +
>         hw = &adapter->hw;
>         hw->back = adapter;
>         adapter->msg_enable = NETIF_MSG_DRV | NETIF_MSG_PROBE;

Duyck, Alexander H wrote:
> Stephen Hemminger wrote:
>> This driver is multiqueue, so implement a small skb recycling queue
>> per cpu. It doesn't make sense to have a global queue since then a
>> lock would be required. Not sure if this is going to work; compile
>> tested only, needs more evaluation
> 
> I did a bit of quick testing and found an issue in the sections of
> code below.  As is the current patch generates a null pointer
> exception on unload because the percpu_alloc and
> for_each_possible_cpu are working off of two different CPU masks.   
> 
> The simple solution is to either replace percpu_alloc with
> alloc_percpu and use cpu_possible_map or replace
> for_each_possible_cpu with for_each_online_cpu and use the
> cpu_online_map.   
> 
> Attached is the version I tested at my desk that used the
> cpu_possible_map solution.  I went that route since alloc_percpu
> seemed like a fairly clean macro.  

It turns out I missed one issue in my testing that was brought to my attention.  It looks like the issue was just due to the fact that the per cpu queues weren't initialized to store packets.  This updated patch adds a loop that goes through and calls __skb_queue_init_head on all the queues.

Thanks,

Alex