On Wed, Oct 07, 2020 at 05:25:04PM +0200, sven.auha...@voleatech.de wrote:
> From: Sven Auhagen <sven.auha...@voleatech.de>
>
> Also use the new helper function igb_rx_buffer_flip in
> igb_build_skb/igb_add_rx_frag.
>
> Signed-off-by: Sven Auhagen <sven.auha...@voleatech.de>
> ---
> drivers/net/ethernet/intel/igb/igb_main.c | 87 +++++++++--------------
> 1 file changed, 35 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c
> b/drivers/net/ethernet/intel/igb/igb_main.c
> index 36ff8725fdaf..f34faf24190a 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -8255,6 +8255,34 @@ static bool igb_can_reuse_rx_page(struct igb_rx_buffer
> *rx_buffer)
> return true;
> }
>
> +static unsigned int igb_rx_frame_truesize(struct igb_ring *rx_ring,
> + unsigned int size)
> +{
> + unsigned int truesize;
> +
> +#if (PAGE_SIZE < 8192)
> + truesize = igb_rx_pg_size(rx_ring) / 2; /* Must be power-of-2 */
> +#else
> + truesize = ring_uses_build_skb(rx_ring) ?
> + SKB_DATA_ALIGN(IGB_SKB_PAD + size) +
> + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) :
> + SKB_DATA_ALIGN(size);
> +#endif
> + return truesize;
> +}
> +
> +static void igb_rx_buffer_flip(struct igb_ring *rx_ring,
> + struct igb_rx_buffer *rx_buffer,
> + unsigned int size)
> +{
> + unsigned int truesize = igb_rx_frame_truesize(rx_ring, size);
> +#if (PAGE_SIZE < 8192)
> + rx_buffer->page_offset ^= truesize;
> +#else
> + rx_buffer->page_offset += truesize;
> +#endif
> +}
> +
> /**
> * igb_add_rx_frag - Add contents of Rx buffer to sk_buff
> * @rx_ring: rx descriptor ring to transact packets on
> @@ -8269,20 +8297,12 @@ static void igb_add_rx_frag(struct igb_ring *rx_ring,
> struct sk_buff *skb,
> unsigned int size)
> {
> -#if (PAGE_SIZE < 8192)
> - unsigned int truesize = igb_rx_pg_size(rx_ring) / 2;
> -#else
> - unsigned int truesize = ring_uses_build_skb(rx_ring) ?
> - SKB_DATA_ALIGN(IGB_SKB_PAD + size) :
> - SKB_DATA_ALIGN(size);
> -#endif
> + unsigned int truesize = igb_rx_frame_truesize(rx_ring, size);
I don't think we need to account the size of skb_shared_info when adding
another frag as we already have the skb in place with its skb_shared_info.
Also, please make sure that you list all of the changes that patch
contains in the commit message, you simply skipped the fact that you're
making use of igb_rx_frame_truesize on other places.
> +
> skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, rx_buffer->page,
> rx_buffer->page_offset, size, truesize);
> -#if (PAGE_SIZE < 8192)
> - rx_buffer->page_offset ^= truesize;
> -#else
> - rx_buffer->page_offset += truesize;
> -#endif
> +
> + igb_rx_buffer_flip(rx_ring, rx_buffer, size);
> }
>
> static struct sk_buff *igb_construct_skb(struct igb_ring *rx_ring,
> @@ -8345,14 +8365,9 @@ static struct sk_buff *igb_build_skb(struct igb_ring
> *rx_ring,
> struct xdp_buff *xdp,
> union e1000_adv_rx_desc *rx_desc)
> {
> + unsigned int size = xdp->data_end - xdp->data_hard_start;
> + unsigned int truesize = igb_rx_frame_truesize(rx_ring, size);
Here you will be counting the IGB_SKB_PAD twice for pages > 4k.
xdp->data_end - xdp->data_hard_start already includes the IGB_SKB_PAD and
then igb_rx_frame_truesize will add this IGB_SKB_PAD once again to the
size you're providing.
Please drop the additional usage of igb_rx_frame_truesize in this patch.
> unsigned int metasize = xdp->data - xdp->data_meta;
> -#if (PAGE_SIZE < 8192)
> - unsigned int truesize = igb_rx_pg_size(rx_ring) / 2;
> -#else
> - unsigned int truesize = SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) +
> - SKB_DATA_ALIGN(xdp->data_end -
> - xdp->data_hard_start);
> -#endif
> struct sk_buff *skb;
>
> /* prefetch first cache line of first page */
> @@ -8377,11 +8392,7 @@ static struct sk_buff *igb_build_skb(struct igb_ring
> *rx_ring,
> }
>
> /* update buffer offset */
> -#if (PAGE_SIZE < 8192)
> - rx_buffer->page_offset ^= truesize;
> -#else
> - rx_buffer->page_offset += truesize;
> -#endif
> + igb_rx_buffer_flip(rx_ring, rx_buffer, size);
>
> return skb;
> }
> @@ -8431,34 +8442,6 @@ static struct sk_buff *igb_run_xdp(struct igb_adapter
> *adapter,
> return ERR_PTR(-result);
> }
>
> -static unsigned int igb_rx_frame_truesize(struct igb_ring *rx_ring,
> - unsigned int size)
> -{
> - unsigned int truesize;
> -
> -#if (PAGE_SIZE < 8192)
> - truesize = igb_rx_pg_size(rx_ring) / 2; /* Must be power-of-2 */
> -#else
> - truesize = ring_uses_build_skb(rx_ring) ?
> - SKB_DATA_ALIGN(IGB_SKB_PAD + size) +
> - SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) :
> - SKB_DATA_ALIGN(size);
> -#endif
> - return truesize;
> -}
> -
> -static void igb_rx_buffer_flip(struct igb_ring *rx_ring,
> - struct igb_rx_buffer *rx_buffer,
> - unsigned int size)
> -{
> - unsigned int truesize = igb_rx_frame_truesize(rx_ring, size);
> -#if (PAGE_SIZE < 8192)
> - rx_buffer->page_offset ^= truesize;
> -#else
> - rx_buffer->page_offset += truesize;
> -#endif
> -}
> -
> static inline void igb_rx_checksum(struct igb_ring *ring,
> union e1000_adv_rx_desc *rx_desc,
> struct sk_buff *skb)
> --
> 2.20.1
>
