Skip to contentSkip to conversation

igb: skb recycling

RFC In review Stephen Hemminger 2 participants 3 messages Oct 21, 2008 – Oct 24, 2008

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Stephen Hemminger

author
#0 lore raw

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 <[email protected]>
drivers/net/igb/igb.h +1 -0
@@ -232,6 +232,7 @@ struct igb_adapter {
 
 	/* TX */
 	struct igb_ring *tx_ring;      /* One per active queue */
+	struct sk_buff_head *rx_recycle; /* One per cpu */
 	unsigned int restart_queue;
 	unsigned long tx_queue_len;
 	u32 txd_cmd;
drivers/net/igb/igb_main.c +38 -11

93 lines not on this page · raw message

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK & win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

#1 lore raw
Quoted 4 lines · Stephen Hemminger

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

Quoted 16 lines

--- 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);
(quoted diff) +3 -0
+
+       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)

(quoted diff) +0 -0
@@ -1022,6 +1027,11 @@ static int __devinit igb_probe(struct pc
        adapter = netdev_priv(netdev);

Review static int __devinit igb_probe(struct pc

Quoted 10 lines
(quoted diff) +4 -0
        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;

---

 drivers/net/igb/igb.h      |    1 +
 drivers/net/igb/igb_main.c |   48 +++++++++++++++++++++++++++++++++++---------
 2 files changed, 39 insertions(+), 10 deletions(-)
drivers/net/igb/igb.h +1 -0
index 4ff6f05..84e83a3 100644
@@ -232,6 +232,7 @@ struct igb_adapter {
 
 	/* TX */
 	struct igb_ring *tx_ring;      /* One per active queue */
+	struct sk_buff_head *rx_recycle; /* One per cpu */
 	unsigned int restart_queue;
 	unsigned long tx_queue_len;
 	u32 txd_cmd;
drivers/net/igb/igb_main.c +38 -10

93 lines not on this page · raw message

#2 lore raw
Quoted 11 lines · Alexander H
Quoted 4 lines · Stephen Hemminger

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.

Quoted 8 lines

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

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK & win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

_______________________________________________
E1000-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/e1000-devel