netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next] sfc: fix XDP-redirect in this driver
@ 2020-03-13 13:25 Jesper Dangaard Brouer
  2020-03-16  8:49 ` David Miller
  2020-03-17  1:22 ` David Miller
  0 siblings, 2 replies; 7+ messages in thread
From: Jesper Dangaard Brouer @ 2020-03-13 13:25 UTC (permalink / raw)
  To: Jesper Dangaard Brouer
  Cc: netdev, linux-net-drivers, ecree, mhabets, cmclachlan,
	Ilias Apalodimas, Lorenzo Bianconi, sameehj

XDP-redirect is broken in this driver sfc. XDP_REDIRECT requires
tailroom for skb_shared_info when creating an SKB based on the
redirected xdp_frame (both in cpumap and veth).

The fix requires some initial explaining. The driver uses RX page-split
when possible. It reserves the top 64 bytes in the RX-page for storing
dma_addr (struct efx_rx_page_state). It also have the XDP recommended
headroom of XDP_PACKET_HEADROOM (256 bytes). As it doesn't reserve any
tailroom, it can still fit two standard MTU (1500) frames into one page.

The sizeof struct skb_shared_info in 320 bytes. Thus drivers like ixgbe
and i40e, reduce their XDP headroom to 192 bytes, which allows them to
fit two frames with max 1536 bytes into a 4K page (192+1536+320=2048).

The fix is to reduce this drivers headroom to 128 bytes and add the 320
bytes tailroom. This account for reserved top 64 bytes in the page, and
still fit two frame in a page for normal MTUs.

We must never go below 128 bytes of headroom for XDP, as one cacheline
is for xdp_frame area and next cacheline is reserved for metadata area.

Fixes: eb9a36be7f3e ("sfc: perform XDP processing on received packets")
Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
Targetted net-next as this is part of a patchset for adding XDP frame
size and reserving tailroom for multi-buffer XDP

 drivers/net/ethernet/sfc/efx_common.c |    9 +++++----
 drivers/net/ethernet/sfc/net_driver.h |    6 ++++++
 drivers/net/ethernet/sfc/rx.c         |    2 +-
 drivers/net/ethernet/sfc/rx_common.c  |    6 +++---
 4 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/sfc/efx_common.c b/drivers/net/ethernet/sfc/efx_common.c
index b0d76bc19673..1799ff9a45d9 100644
--- a/drivers/net/ethernet/sfc/efx_common.c
+++ b/drivers/net/ethernet/sfc/efx_common.c
@@ -200,11 +200,11 @@ void efx_link_status_changed(struct efx_nic *efx)
 unsigned int efx_xdp_max_mtu(struct efx_nic *efx)
 {
 	/* The maximum MTU that we can fit in a single page, allowing for
-	 * framing, overhead and XDP headroom.
+	 * framing, overhead and XDP headroom + tailroom.
 	 */
 	int overhead = EFX_MAX_FRAME_LEN(0) + sizeof(struct efx_rx_page_state) +
 		       efx->rx_prefix_size + efx->type->rx_buffer_padding +
-		       efx->rx_ip_align + XDP_PACKET_HEADROOM;
+		       efx->rx_ip_align + EFX_XDP_HEADROOM + EFX_XDP_TAILROOM;
 
 	return PAGE_SIZE - overhead;
 }
@@ -302,8 +302,9 @@ static void efx_start_datapath(struct efx_nic *efx)
 	efx->rx_dma_len = (efx->rx_prefix_size +
 			   EFX_MAX_FRAME_LEN(efx->net_dev->mtu) +
 			   efx->type->rx_buffer_padding);
-	rx_buf_len = (sizeof(struct efx_rx_page_state) + XDP_PACKET_HEADROOM +
-		      efx->rx_ip_align + efx->rx_dma_len);
+	rx_buf_len = (sizeof(struct efx_rx_page_state)   + EFX_XDP_HEADROOM +
+		      efx->rx_ip_align + efx->rx_dma_len + EFX_XDP_TAILROOM);
+
 	if (rx_buf_len <= PAGE_SIZE) {
 		efx->rx_scatter = efx->type->always_rx_scatter;
 		efx->rx_buffer_order = 0;
diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h
index 9f9886f222c8..f96b1f9fe119 100644
--- a/drivers/net/ethernet/sfc/net_driver.h
+++ b/drivers/net/ethernet/sfc/net_driver.h
@@ -91,6 +91,12 @@
 #define EFX_RX_BUF_ALIGNMENT	4
 #endif
 
+/* Non-standard XDP_PACKET_HEADROOM and tailroom to satisfy XDP_REDIRECT and
+ * still fit two standard MTU size packets into a single 4K page.
+ */
+#define EFX_XDP_HEADROOM	128
+#define EFX_XDP_TAILROOM	SKB_DATA_ALIGN(sizeof(struct skb_shared_info))
+
 /* Forward declare Precision Time Protocol (PTP) support structure. */
 struct efx_ptp_data;
 struct hwtstamp_config;
diff --git a/drivers/net/ethernet/sfc/rx.c b/drivers/net/ethernet/sfc/rx.c
index a2042f16babc..260352d97d9d 100644
--- a/drivers/net/ethernet/sfc/rx.c
+++ b/drivers/net/ethernet/sfc/rx.c
@@ -302,7 +302,7 @@ static bool efx_do_xdp(struct efx_nic *efx, struct efx_channel *channel,
 	       efx->rx_prefix_size);
 
 	xdp.data = *ehp;
-	xdp.data_hard_start = xdp.data - XDP_PACKET_HEADROOM;
+	xdp.data_hard_start = xdp.data - EFX_XDP_HEADROOM;
 
 	/* No support yet for XDP metadata */
 	xdp_set_data_meta_invalid(&xdp);
diff --git a/drivers/net/ethernet/sfc/rx_common.c b/drivers/net/ethernet/sfc/rx_common.c
index ee8beb87bdc1..e10c23833515 100644
--- a/drivers/net/ethernet/sfc/rx_common.c
+++ b/drivers/net/ethernet/sfc/rx_common.c
@@ -412,10 +412,10 @@ static int efx_init_rx_buffers(struct efx_rx_queue *rx_queue, bool atomic)
 			index = rx_queue->added_count & rx_queue->ptr_mask;
 			rx_buf = efx_rx_buffer(rx_queue, index);
 			rx_buf->dma_addr = dma_addr + efx->rx_ip_align +
-					   XDP_PACKET_HEADROOM;
+					   EFX_XDP_HEADROOM;
 			rx_buf->page = page;
 			rx_buf->page_offset = page_offset + efx->rx_ip_align +
-					      XDP_PACKET_HEADROOM;
+					      EFX_XDP_HEADROOM;
 			rx_buf->len = efx->rx_dma_len;
 			rx_buf->flags = 0;
 			++rx_queue->added_count;
@@ -433,7 +433,7 @@ static int efx_init_rx_buffers(struct efx_rx_queue *rx_queue, bool atomic)
 void efx_rx_config_page_split(struct efx_nic *efx)
 {
 	efx->rx_page_buf_step = ALIGN(efx->rx_dma_len + efx->rx_ip_align +
-				      XDP_PACKET_HEADROOM,
+				      EFX_XDP_HEADROOM + EFX_XDP_TAILROOM,
 				      EFX_RX_BUF_ALIGNMENT);
 	efx->rx_bufs_per_page = efx->rx_buffer_order ? 1 :
 		((PAGE_SIZE - sizeof(struct efx_rx_page_state)) /



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH net-next] sfc: fix XDP-redirect in this driver
  2020-03-13 13:25 [PATCH net-next] sfc: fix XDP-redirect in this driver Jesper Dangaard Brouer
@ 2020-03-16  8:49 ` David Miller
  2020-03-16 10:10   ` Edward Cree
  2020-03-17  1:22 ` David Miller
  1 sibling, 1 reply; 7+ messages in thread
From: David Miller @ 2020-03-16  8:49 UTC (permalink / raw)
  To: brouer
  Cc: netdev, linux-net-drivers, ecree, mhabets, cmclachlan,
	ilias.apalodimas, lorenzo, sameehj

From: Jesper Dangaard Brouer <brouer@redhat.com>
Date: Fri, 13 Mar 2020 14:25:19 +0100

> XDP-redirect is broken in this driver sfc. XDP_REDIRECT requires
> tailroom for skb_shared_info when creating an SKB based on the
> redirected xdp_frame (both in cpumap and veth).
> 
> The fix requires some initial explaining. The driver uses RX page-split
> when possible. It reserves the top 64 bytes in the RX-page for storing
> dma_addr (struct efx_rx_page_state). It also have the XDP recommended
> headroom of XDP_PACKET_HEADROOM (256 bytes). As it doesn't reserve any
> tailroom, it can still fit two standard MTU (1500) frames into one page.
> 
> The sizeof struct skb_shared_info in 320 bytes. Thus drivers like ixgbe
> and i40e, reduce their XDP headroom to 192 bytes, which allows them to
> fit two frames with max 1536 bytes into a 4K page (192+1536+320=2048).
> 
> The fix is to reduce this drivers headroom to 128 bytes and add the 320
> bytes tailroom. This account for reserved top 64 bytes in the page, and
> still fit two frame in a page for normal MTUs.
> 
> We must never go below 128 bytes of headroom for XDP, as one cacheline
> is for xdp_frame area and next cacheline is reserved for metadata area.
> 
> Fixes: eb9a36be7f3e ("sfc: perform XDP processing on received packets")
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> ---
> Targetted net-next as this is part of a patchset for adding XDP frame
> size and reserving tailroom for multi-buffer XDP

Solarflare folks, please review.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net-next] sfc: fix XDP-redirect in this driver
  2020-03-16  8:49 ` David Miller
@ 2020-03-16 10:10   ` Edward Cree
  2020-03-16 10:35     ` Jesper Dangaard Brouer
  0 siblings, 1 reply; 7+ messages in thread
From: Edward Cree @ 2020-03-16 10:10 UTC (permalink / raw)
  To: David Miller, brouer
  Cc: netdev, linux-net-drivers, mhabets, cmclachlan, ilias.apalodimas,
	lorenzo, sameehj

On 16/03/2020 08:49, David Miller wrote:
> Solarflare folks, please review.
This looks like a correct implementation of what it purports to do, so
Acked-by: Edward Cree <ecree@solarflare.com>
It did take me some digging to understand _why_ it was needed though;
 Jesper, is there any documentation of the tailroom requirement?  It
 doesn't seem to be mentioned anywhere I could find.
(Is there even any up-to-date doc of the XDP driver interface?  The
 one at [1] looks a bit stale...)
-Ed

[1]: https://prototype-kernel.readthedocs.io/en/latest/networking/XDP/design/requirements.html

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net-next] sfc: fix XDP-redirect in this driver
  2020-03-16 10:10   ` Edward Cree
@ 2020-03-16 10:35     ` Jesper Dangaard Brouer
  2020-03-16 10:45       ` Denis Kirjanov
  0 siblings, 1 reply; 7+ messages in thread
From: Jesper Dangaard Brouer @ 2020-03-16 10:35 UTC (permalink / raw)
  To: Edward Cree
  Cc: David Miller, netdev, linux-net-drivers, mhabets, cmclachlan,
	ilias.apalodimas, lorenzo, sameehj, brouer

On Mon, 16 Mar 2020 10:10:01 +0000
Edward Cree <ecree@solarflare.com> wrote:

> On 16/03/2020 08:49, David Miller wrote:
> > Solarflare folks, please review.  
>
> This looks like a correct implementation of what it purports to do, so
> Acked-by: Edward Cree <ecree@solarflare.com>

Thanks for the review!

> It did take me some digging to understand _why_ it was needed though;
>  Jesper, is there any documentation of the tailroom requirement?  It
>  doesn't seem to be mentioned anywhere I could find.

I admit that is is poorly documented.  It is a requirement as both
cpumap and veth have a dependency when they process the redirected
packet.  We/I also need to update the doc on one page per packet
requirement, as it is (in-practice) no longer true.

I'm noticing these bugs, because I'm working on a patchset that add
tailroom extend, and also reserves this 'skb_shared_info' tailroom area.
The real goal is to later add XDP multi-buffer support, using this
'skb_shared_info' tailroom area, as desc here[2]

[2] https://github.com/xdp-project/xdp-project/blob/master/areas/core/xdp-multi-buffer01-design.org

> (Is there even any up-to-date doc of the XDP driver interface?  The
>  one at [1] looks a bit stale...)
> -Ed
> 
> [1]: https://prototype-kernel.readthedocs.io/en/latest/networking/XDP/design/requirements.html

This is my old and out-dated documentation. I didn't know people were
still referring to this.  I does score quite high on Google, so I
guess, that I really need to update this documentation.  (It was my
plan that this would be merged into kernel tree, but I can see it have
been bit-rotting for too long).

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net-next] sfc: fix XDP-redirect in this driver
  2020-03-16 10:35     ` Jesper Dangaard Brouer
@ 2020-03-16 10:45       ` Denis Kirjanov
  2020-03-16 11:55         ` Jesper Dangaard Brouer
  0 siblings, 1 reply; 7+ messages in thread
From: Denis Kirjanov @ 2020-03-16 10:45 UTC (permalink / raw)
  To: Jesper Dangaard Brouer
  Cc: Edward Cree, David Miller, netdev, linux-net-drivers, mhabets,
	cmclachlan, ilias.apalodimas, lorenzo, sameehj

On 3/16/20, Jesper Dangaard Brouer <brouer@redhat.com> wrote:
> On Mon, 16 Mar 2020 10:10:01 +0000
> Edward Cree <ecree@solarflare.com> wrote:
>
>> On 16/03/2020 08:49, David Miller wrote:
>> > Solarflare folks, please review.
>>
>> This looks like a correct implementation of what it purports to do, so
>> Acked-by: Edward Cree <ecree@solarflare.com>
>
> Thanks for the review!
>
>> It did take me some digging to understand _why_ it was needed though;
>>  Jesper, is there any documentation of the tailroom requirement?  It
>>  doesn't seem to be mentioned anywhere I could find.
>
> I admit that is is poorly documented.  It is a requirement as both
> cpumap and veth have a dependency when they process the redirected
> packet.  We/I also need to update the doc on one page per packet
> requirement, as it is (in-practice) no longer true.

Hi Jesper,

that means that's on-going work to add multi-buffer/page support to XDP, right?

Thanks!
>
> I'm noticing these bugs, because I'm working on a patchset that add
> tailroom extend, and also reserves this 'skb_shared_info' tailroom area.
> The real goal is to later add XDP multi-buffer support, using this
> 'skb_shared_info' tailroom area, as desc here[2]
>
> [2]
> https://github.com/xdp-project/xdp-project/blob/master/areas/core/xdp-multi-buffer01-design.org
>
>> (Is there even any up-to-date doc of the XDP driver interface?  The
>>  one at [1] looks a bit stale...)
>> -Ed
>>
>> [1]:
>> https://prototype-kernel.readthedocs.io/en/latest/networking/XDP/design/requirements.html
>
> This is my old and out-dated documentation. I didn't know people were
> still referring to this.  I does score quite high on Google, so I
> guess, that I really need to update this documentation.  (It was my
> plan that this would be merged into kernel tree, but I can see it have
> been bit-rotting for too long).
>
> --
> Best regards,
>   Jesper Dangaard Brouer
>   MSc.CS, Principal Kernel Engineer at Red Hat
>   LinkedIn: http://www.linkedin.com/in/brouer
>
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net-next] sfc: fix XDP-redirect in this driver
  2020-03-16 10:45       ` Denis Kirjanov
@ 2020-03-16 11:55         ` Jesper Dangaard Brouer
  0 siblings, 0 replies; 7+ messages in thread
From: Jesper Dangaard Brouer @ 2020-03-16 11:55 UTC (permalink / raw)
  To: Denis Kirjanov
  Cc: Edward Cree, David Miller, netdev, linux-net-drivers, mhabets,
	cmclachlan, ilias.apalodimas, lorenzo, Jubran, Samih, brouer,
	Willem de Bruijn

On Mon, 16 Mar 2020 13:45:23 +0300
Denis Kirjanov <kda@linux-powerpc.org> wrote:

> On 3/16/20, Jesper Dangaard Brouer <brouer@redhat.com> wrote:
> > On Mon, 16 Mar 2020 10:10:01 +0000
> > Edward Cree <ecree@solarflare.com> wrote:
> >  
> >> On 16/03/2020 08:49, David Miller wrote:  
> >> > Solarflare folks, please review.  
> >>
> >> This looks like a correct implementation of what it purports to do, so
> >> Acked-by: Edward Cree <ecree@solarflare.com>  
> >
> > Thanks for the review!
> >  
> >> It did take me some digging to understand _why_ it was needed though;
> >>  Jesper, is there any documentation of the tailroom requirement?  It
> >>  doesn't seem to be mentioned anywhere I could find.  
> >
> > I admit that is is poorly documented.  It is a requirement as both
> > cpumap and veth have a dependency when they process the redirected
> > packet.  We/I also need to update the doc on one page per packet
> > requirement, as it is (in-practice) no longer true.  
> 
> Hi Jesper,
> 
> that means that's on-going work to add multi-buffer/page support to XDP, right?

Yes, it seems that both Amazon and Google have a need for this.

Do notice that there is a fair amount of work-ahead.  I'm working on
establishing a frame size/end, such that we can get reserved storage
space for multi-buffer references/segments[3].  I know Samih from
Amazon is working on the multi-buffer part of using this area.
 
> >
> > I'm noticing these bugs, because I'm working on a patchset that add
> > tailroom extend, and also reserves this 'skb_shared_info' tailroom area.
> > The real goal is to later add XDP multi-buffer support, using this
> > 'skb_shared_info' tailroom area, as desc here[2]
> >
> > [2]
> > https://github.com/xdp-project/xdp-project/blob/master/areas/core/xdp-multi-buffer01-design.org

[3] https://github.com/xdp-project/xdp-project/blob/master/areas/core/xdp-multi-buffer01-design.org#storage-space-for-multi-buffer-referencessegments
-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net-next] sfc: fix XDP-redirect in this driver
  2020-03-13 13:25 [PATCH net-next] sfc: fix XDP-redirect in this driver Jesper Dangaard Brouer
  2020-03-16  8:49 ` David Miller
@ 2020-03-17  1:22 ` David Miller
  1 sibling, 0 replies; 7+ messages in thread
From: David Miller @ 2020-03-17  1:22 UTC (permalink / raw)
  To: brouer
  Cc: netdev, linux-net-drivers, ecree, mhabets, cmclachlan,
	ilias.apalodimas, lorenzo, sameehj

From: Jesper Dangaard Brouer <brouer@redhat.com>
Date: Fri, 13 Mar 2020 14:25:19 +0100

> XDP-redirect is broken in this driver sfc. XDP_REDIRECT requires
> tailroom for skb_shared_info when creating an SKB based on the
> redirected xdp_frame (both in cpumap and veth).
> 
> The fix requires some initial explaining. The driver uses RX page-split
> when possible. It reserves the top 64 bytes in the RX-page for storing
> dma_addr (struct efx_rx_page_state). It also have the XDP recommended
> headroom of XDP_PACKET_HEADROOM (256 bytes). As it doesn't reserve any
> tailroom, it can still fit two standard MTU (1500) frames into one page.
> 
> The sizeof struct skb_shared_info in 320 bytes. Thus drivers like ixgbe
> and i40e, reduce their XDP headroom to 192 bytes, which allows them to
> fit two frames with max 1536 bytes into a 4K page (192+1536+320=2048).
> 
> The fix is to reduce this drivers headroom to 128 bytes and add the 320
> bytes tailroom. This account for reserved top 64 bytes in the page, and
> still fit two frame in a page for normal MTUs.
> 
> We must never go below 128 bytes of headroom for XDP, as one cacheline
> is for xdp_frame area and next cacheline is reserved for metadata area.
> 
> Fixes: eb9a36be7f3e ("sfc: perform XDP processing on received packets")
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> ---
> Targetted net-next as this is part of a patchset for adding XDP frame
> size and reserving tailroom for multi-buffer XDP

Applied, thanks Jesper.

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-03-17  1:23 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-13 13:25 [PATCH net-next] sfc: fix XDP-redirect in this driver Jesper Dangaard Brouer
2020-03-16  8:49 ` David Miller
2020-03-16 10:10   ` Edward Cree
2020-03-16 10:35     ` Jesper Dangaard Brouer
2020-03-16 10:45       ` Denis Kirjanov
2020-03-16 11:55         ` Jesper Dangaard Brouer
2020-03-17  1:22 ` David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).