netdev - Re: [PATCH net-next] igb: use build

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]

Date:	Mon, 06 Aug 2012 10:35:34 -0700
From:	Alexander Duyck <alexander.h.duyck@...el.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
CC:	Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
	netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next] igb: use build_skb()

On 08/02/2012 08:51 AM, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@...gle.com>
>
> By using netdev_alloc_frag() & build_skb() instead of legacy
> netdev_alloc_skb_ip_align() calls, we reduce number of cache misses in
> RX path and size of working set.
>
> For a given rx workload, number of 'inuse' sk_buff can be reduced to a
> very minimum, especially when packets are dropped by our stack.
>
> (Before this patch, default sk_buff allocation was 2048 sk_buffs in rx
> ring buffer)
>
> They are initialized right before being delivered to stack, so can stay
> hot in cpu caches.
>
> Ethernet header prefetching is more effective (old prefetch of skb->data
> paid a stall to access skb->data pointer)
>
> I have 15% performance increase in a RX stress test, removing SLUB slow
> path in the profiles.
>
> Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> Cc: Alexander Duyck <alexander.h.duyck@...el.com>
> ---
>  drivers/net/ethernet/intel/igb/igb.h         |    8 ++
>  drivers/net/ethernet/intel/igb/igb_ethtool.c |   14 ++--
>  drivers/net/ethernet/intel/igb/igb_main.c    |   56 ++++++++++-------
>  3 files changed, 49 insertions(+), 29 deletions(-)
>
[...]
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index b7c2d50..8b732c9 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
[...]
> @@ -6091,6 +6095,15 @@ static bool igb_clean_rx_irq(struct igb_q_vector *q_vector, int budget)
>  		next_rxd = IGB_RX_DESC(rx_ring, i);
>  		prefetch(next_rxd);
>  
> +		if (!skb) {
> +			skb = build_skb(data, IGB_FRAGSZ);
> +			if (unlikely(!skb)) {
> +				rx_ring->rx_stats.alloc_failed++;
> +				buffer_info->data = data;
> +				goto next_desc;
> +			}
> +			skb_reserve(skb, NET_SKB_PAD + NET_IP_ALIGN);
> +		}
>  		/*
>  		 * This memory barrier is needed to keep us from reading
>  		 * any other fields out of the rx_desc until we know the
This logic is broken.  If an allocation failure occurs it would leave
the data in the ring and could possibly give you a corrupted packet.

I was planning to move igb over to an ixgbe style receive path at some
point anyway.  Since it seems like this is now a higher priority I
figured I would try to get the patches for it implemented in the next
week or so.  Would there be any issue with us rejecting this patch and
instead switching igb over to the ixgbe style path?

Thanks,

Alex
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html