Re: [PATCH 11/11] ixgb: Add prefetch

[Eric Dumazet <dada1@cosmosbay.com>]

Jeff Kirsher a écrit :
> - This patch is to improve performance by adding prefetch to the ixgb driver
> - Add driver comments
> 
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> Signed-off-by: John Ronciak <john.ronciak@intel.com>
> ---
> 
>  drivers/net/ixgb/ixgb_main.c |   10 +++++++++-
>  1 files changed, 9 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/ixgb/ixgb_main.c b/drivers/net/ixgb/ixgb_main.c
> index 26cb0d5..98303cb 100644
> --- a/drivers/net/ixgb/ixgb_main.c
> +++ b/drivers/net/ixgb/ixgb_main.c
> @@ -29,6 +29,13 @@
>  #include "ixgb.h"
>  
>  /* Change Log
> + * 1.0.104 10-Jan-2006
> + * - fix for copybreak/recycle
> + * 1.0.103 Oct-3
> + * - suck in some e1000 changes, including copybreak and LLTX
> + * - support for CX4 adapters
> + * 1.0.102 June-20-2005
> + * - add a workaround for a hardware issue when using TSO
>   * 1.0.96 04/19/05
>   * - Make needlessly global code static -- bunk@stusta.de
>   * - ethtool cleanup -- shemminger@osdl.org
> @@ -1916,7 +1923,7 @@ ixgb_clean_rx_irq(struct ixgb_adapter *a
>  		skb = buffer_info->skb;
>  		buffer_info->skb = NULL;
>  
> -		prefetch(skb->data);
> +		prefetch(skb->data - NET_IP_ALIGN);

I doubt this change is usefull.

skb->data and dkb->dta - NET_IP_ALIGN are on the same cache line.
So prefetch(skb->data) is cheaper for he compiler and has the same effect on 
the memory prefetch that is eventually done by the cpu.


>  
>  		if(++i == rx_ring->count) i = 0;
>  		next_rxd = IXGB_RX_DESC(*rx_ring, i);
> @@ -1929,6 +1936,7 @@ ixgb_clean_rx_irq(struct ixgb_adapter *a
>  		next_buffer = &rx_ring->buffer_info[i];
>  		next_skb = next_buffer->skb;
>  		prefetch(next_skb);
> +		prefetch(next_skb->data - NET_IP_ALIGN);

I doubt that next->skb_data is available for free just after a 
prefetch(next_skb). This second prefetch has a hidden cost : The memory 
location (&next_skb->data) must be in L1 cache. So basically the 
prefetch(next_skb) is useless...

prefetch are not magic things. They have a cost (they increase the code size), 
and should be used carefully.

Eric