| Message ID | 20201007152506.66217-6-sven.auhagen@voleatech.de |
|---|---|
| State | New |
| Headers | show |
| Series | igb: xdp patches followup | expand |
On Wed, Oct 07, 2020 at 05:25:04PM +0200, sven.auhagen@voleatech.de wrote: > From: Sven Auhagen <sven.auhagen@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.auhagen@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 >
On Wed, Oct 07, 2020 at 11:32:57PM +0200, Maciej Fijalkowski wrote: > On Wed, Oct 07, 2020 at 05:25:04PM +0200, sven.auhagen@voleatech.de wrote: > > From: Sven Auhagen <sven.auhagen@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.auhagen@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. Do you have a suggestion on how to add the truesize and flip function at this point? The truesize is not suited here then and returns the wrong size for frags. > > > + > > 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. > The igb_rx_buffer_flip will use the wrong truesize though even when I remove igb_rx_frame_truesize here. Should I drop the entire patch as the truesize for the flip will be incorrect in both places? > > 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 > >
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); + 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); 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)