netdev - RE: [E1000-devel] [RFC 2/2] igb: skb recycling

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <80769D7B14936844A23C0C43D9FBCF0F176AE5E0@orsmsx501.amr.corp.intel.com>
Date:	Wed, 22 Oct 2008 16:53:17 -0700
From:	"Duyck, Alexander H" <alexander.h.duyck@...el.com>
To:	Stephen Hemminger <shemminger@...tta.com>,
	"jeffery.t.kirsher@...el.com" <jeffery.t.kirsher@...el.com>,
	"Brandeburg, Jesse" <jesse.brandeburg@...el.com>,
	"Allan, Bruce W" <bruce.w.allan@...el.com>,
	"Waskiewicz Jr, Peter P" <peter.p.waskiewicz.jr@...el.com>
CC:	"e1000-devel@...ts.sourceforge.net" 
	<e1000-devel@...ts.sourceforge.net>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [E1000-devel] [RFC 2/2] igb: skb recycling

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;


Download attachment "skb_recycle.diff" of type "application/octet-stream" (3753 bytes)