[PATCH net 0/3] dpaa_eth: A050385 erratum workaround fixes under XDP

[PATCH net 0/3] dpaa_eth: A050385 erratum workaround fixes under XDP

[Camelia Groza]

This series addresses issue with the current workaround for the A050385
erratum in XDP scenarios.

The first patch makes sure the xdp_frame structure stored at the start of
new buffers isn't overwritten.

The second patch decreases the required data alignment value, thus
preventing unnecessary realignments.

The third patch moves the data in place to align it, instead of allocating
a new buffer for each frame that breaks the alignment rules, thus bringing
an up to 40% performance increase. With this change, the impact of the
erratum workaround is reduced in many cases to a single digit decrease, and
to lower double digits in single flow scenarios.

Camelia Groza (3):
  dpaa_eth: reserve space for the xdp_frame under the A050385 erratum
  dpaa_eth: reduce data alignment requirements for the A050385 erratum
  dpaa_eth: try to move the data in place for the A050385 erratum

 .../net/ethernet/freescale/dpaa/dpaa_eth.c    | 39 +++++++++++++++++--
 1 file changed, 35 insertions(+), 4 deletions(-)

--
2.17.1

[PATCH net 1/3] dpaa_eth: reserve space for the xdp_frame under the A050385 erratum

[Camelia Groza]

When the erratum workaround is triggered, the newly created xdp_frame
structure is stored at the start of the newly allocated buffer. Avoid
the structure from being overwritten by explicitly reserving enough
space in the buffer for storing it.

Account for the fact that the structure's size might increase in time by
aligning the headroom to DPAA_FD_DATA_ALIGNMENT bytes, thus guaranteeing
the data's alignment.

Fixes: ae680bcbd06a ("dpaa_eth: implement the A050385 erratum workaround for XDP")
Signed-off-by: Camelia Groza <camelia.groza@nxp.com>
---
 drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
index 4360ce4d3fb6..e1d041c35ad9 100644
--- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
+++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
@@ -2182,6 +2182,7 @@ static int dpaa_a050385_wa_xdpf(struct dpaa_priv *priv,
 	struct xdp_frame *new_xdpf, *xdpf = *init_xdpf;
 	void *new_buff;
 	struct page *p;
+	int headroom;
 
 	/* Check the data alignment and make sure the headroom is large
 	 * enough to store the xdpf backpointer. Use an aligned headroom
@@ -2197,19 +2198,31 @@ static int dpaa_a050385_wa_xdpf(struct dpaa_priv *priv,
 		return 0;
 	}
 
+	/* The new xdp_frame is stored in the new buffer. Reserve enough space
+	 * in the headroom for storing it along with the driver's private
+	 * info. The headroom needs to be aligned to DPAA_FD_DATA_ALIGNMENT to
+	 * guarantee the data's alignment in the buffer.
+	 */
+	headroom = ALIGN(sizeof(*new_xdpf) + priv->tx_headroom,
+			 DPAA_FD_DATA_ALIGNMENT);
+
+	/* Assure the extended headroom and data fit in a one-paged buffer */
+	if (headroom + xdpf->len > DPAA_BP_RAW_SIZE)
+		return -ENOMEM;
+
 	p = dev_alloc_pages(0);
 	if (unlikely(!p))
 		return -ENOMEM;
 
 	/* Copy the data to the new buffer at a properly aligned offset */
 	new_buff = page_address(p);
-	memcpy(new_buff + priv->tx_headroom, xdpf->data, xdpf->len);
+	memcpy(new_buff + headroom, xdpf->data, xdpf->len);
 
 	/* Create an XDP frame around the new buffer in a similar fashion
 	 * to xdp_convert_buff_to_frame.
 	 */
 	new_xdpf = new_buff;
-	new_xdpf->data = new_buff + priv->tx_headroom;
+	new_xdpf->data = new_buff + headroom;
 	new_xdpf->len = xdpf->len;
 	new_xdpf->headroom = priv->tx_headroom;
 	new_xdpf->frame_sz = DPAA_BP_RAW_SIZE;
-- 
2.17.1

Re: [PATCH net 1/3] dpaa_eth: reserve space for the xdp_frame under the A050385 erratum

[Maciej Fijalkowski]

On Tue, Feb 02, 2021 at 07:34:42PM +0200, Camelia Groza wrote:
> When the erratum workaround is triggered, the newly created xdp_frame
> structure is stored at the start of the newly allocated buffer. Avoid
> the structure from being overwritten by explicitly reserving enough
> space in the buffer for storing it.
> 
> Account for the fact that the structure's size might increase in time by
> aligning the headroom to DPAA_FD_DATA_ALIGNMENT bytes, thus guaranteeing
> the data's alignment.
> 
> Fixes: ae680bcbd06a ("dpaa_eth: implement the A050385 erratum workaround for XDP")
> Signed-off-by: Camelia Groza <camelia.groza@nxp.com>
> ---
>  drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> index 4360ce4d3fb6..e1d041c35ad9 100644
> --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> @@ -2182,6 +2182,7 @@ static int dpaa_a050385_wa_xdpf(struct dpaa_priv *priv,
>  	struct xdp_frame *new_xdpf, *xdpf = *init_xdpf;
>  	void *new_buff;
>  	struct page *p;
> +	int headroom;
>  
>  	/* Check the data alignment and make sure the headroom is large
>  	 * enough to store the xdpf backpointer. Use an aligned headroom
> @@ -2197,19 +2198,31 @@ static int dpaa_a050385_wa_xdpf(struct dpaa_priv *priv,
>  		return 0;
>  	}
>  
> +	/* The new xdp_frame is stored in the new buffer. Reserve enough space
> +	 * in the headroom for storing it along with the driver's private
> +	 * info. The headroom needs to be aligned to DPAA_FD_DATA_ALIGNMENT to
> +	 * guarantee the data's alignment in the buffer.
> +	 */
> +	headroom = ALIGN(sizeof(*new_xdpf) + priv->tx_headroom,
> +			 DPAA_FD_DATA_ALIGNMENT);
> +
> +	/* Assure the extended headroom and data fit in a one-paged buffer */
> +	if (headroom + xdpf->len > DPAA_BP_RAW_SIZE)

This check might make more sense if you would be accounting for
skb_shared_info as well I suppose, so that you know you'll still provide
enough tailroom for future xdp multibuf support. Didn't all the previous
code path make sure that there's a room for that?

> +		return -ENOMEM;
> +
>  	p = dev_alloc_pages(0);
>  	if (unlikely(!p))
>  		return -ENOMEM;
>  
>  	/* Copy the data to the new buffer at a properly aligned offset */
>  	new_buff = page_address(p);
> -	memcpy(new_buff + priv->tx_headroom, xdpf->data, xdpf->len);
> +	memcpy(new_buff + headroom, xdpf->data, xdpf->len);
>  
>  	/* Create an XDP frame around the new buffer in a similar fashion
>  	 * to xdp_convert_buff_to_frame.
>  	 */
>  	new_xdpf = new_buff;
> -	new_xdpf->data = new_buff + priv->tx_headroom;
> +	new_xdpf->data = new_buff + headroom;
>  	new_xdpf->len = xdpf->len;
>  	new_xdpf->headroom = priv->tx_headroom;
>  	new_xdpf->frame_sz = DPAA_BP_RAW_SIZE;
> -- 
> 2.17.1
> 

RE: [PATCH net 1/3] dpaa_eth: reserve space for the xdp_frame under the A050385 erratum

[Camelia Alexandra Groza]

> -----Original Message-----
> From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> Sent: Thursday, February 4, 2021 10:52
> To: Camelia Alexandra Groza <camelia.groza@nxp.com>
> Cc: kuba@kernel.org; davem@davemloft.net; Madalin Bucur (OSS)
> <madalin.bucur@oss.nxp.com>; netdev@vger.kernel.org
> Subject: Re: [PATCH net 1/3] dpaa_eth: reserve space for the xdp_frame
> under the A050385 erratum
> 
> On Tue, Feb 02, 2021 at 07:34:42PM +0200, Camelia Groza wrote:
> > When the erratum workaround is triggered, the newly created xdp_frame
> > structure is stored at the start of the newly allocated buffer. Avoid
> > the structure from being overwritten by explicitly reserving enough
> > space in the buffer for storing it.
> >
> > Account for the fact that the structure's size might increase in time by
> > aligning the headroom to DPAA_FD_DATA_ALIGNMENT bytes, thus
> guaranteeing
> > the data's alignment.
> >
> > Fixes: ae680bcbd06a ("dpaa_eth: implement the A050385 erratum
> workaround for XDP")
> > Signed-off-by: Camelia Groza <camelia.groza@nxp.com>
> > ---
> >  drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 17
> +++++++++++++++--
> >  1 file changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > index 4360ce4d3fb6..e1d041c35ad9 100644
> > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > @@ -2182,6 +2182,7 @@ static int dpaa_a050385_wa_xdpf(struct
> dpaa_priv *priv,
> >  	struct xdp_frame *new_xdpf, *xdpf = *init_xdpf;
> >  	void *new_buff;
> >  	struct page *p;
> > +	int headroom;
> >
> >  	/* Check the data alignment and make sure the headroom is large
> >  	 * enough to store the xdpf backpointer. Use an aligned headroom
> > @@ -2197,19 +2198,31 @@ static int dpaa_a050385_wa_xdpf(struct
> dpaa_priv *priv,
> >  		return 0;
> >  	}
> >
> > +	/* The new xdp_frame is stored in the new buffer. Reserve enough
> space
> > +	 * in the headroom for storing it along with the driver's private
> > +	 * info. The headroom needs to be aligned to
> DPAA_FD_DATA_ALIGNMENT to
> > +	 * guarantee the data's alignment in the buffer.
> > +	 */
> > +	headroom = ALIGN(sizeof(*new_xdpf) + priv->tx_headroom,
> > +			 DPAA_FD_DATA_ALIGNMENT);
> > +
> > +	/* Assure the extended headroom and data fit in a one-paged buffer
> */
> > +	if (headroom + xdpf->len > DPAA_BP_RAW_SIZE)
> 
> This check might make more sense if you would be accounting for
> skb_shared_info as well I suppose, so that you know you'll still provide
> enough tailroom for future xdp multibuf support. Didn't all the previous
> code path make sure that there's a room for that?

Hi Maciej

This function will have to go through large changes anyway once we enable
multibuf support, so making it future-proof isn't a priority.

Instead, I do want to return a valid xdp_frame so I need to guarantee the
tailroom is there, even if we don't access it . I'll send a v2. Thanks.

Camelia

[PATCH net 2/3] dpaa_eth: reduce data alignment requirements for the A050385 erratum

[Camelia Groza]

The 256 byte data alignment is required for preventing DMA transaction
splits when crossing 4K page boundaries. Since XDP deals only with page
sized buffers or less, this restriction isn't needed. Instead, the data
only needs to be aligned to 64 bytes to prevent DMA transaction splits.

These lessened restrictions can increase performance by widening the pool
of permitted data alignments and preventing unnecessary realignments.

Fixes: ae680bcbd06a ("dpaa_eth: implement the A050385 erratum workaround for XDP")
Signed-off-by: Camelia Groza <camelia.groza@nxp.com>
---
 drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
index e1d041c35ad9..78dfa05f6d55 100644
--- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
+++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
@@ -2192,7 +2192,7 @@ static int dpaa_a050385_wa_xdpf(struct dpaa_priv *priv,
 	 * byte frame headroom. If the XDP program uses all of it, copy the
 	 * data to a new buffer and make room for storing the backpointer.
 	 */
-	if (PTR_IS_ALIGNED(xdpf->data, DPAA_A050385_ALIGN) &&
+	if (PTR_IS_ALIGNED(xdpf->data, DPAA_FD_DATA_ALIGNMENT) &&
 	    xdpf->headroom >= priv->tx_headroom) {
 		xdpf->headroom = priv->tx_headroom;
 		return 0;
-- 
2.17.1

Re: [PATCH net 2/3] dpaa_eth: reduce data alignment requirements for the A050385 erratum

[Maciej Fijalkowski]

On Tue, Feb 02, 2021 at 07:34:43PM +0200, Camelia Groza wrote:
> The 256 byte data alignment is required for preventing DMA transaction
> splits when crossing 4K page boundaries. Since XDP deals only with page
> sized buffers or less, this restriction isn't needed. Instead, the data
> only needs to be aligned to 64 bytes to prevent DMA transaction splits.
> 
> These lessened restrictions can increase performance by widening the pool
> of permitted data alignments and preventing unnecessary realignments.
> 
> Fixes: ae680bcbd06a ("dpaa_eth: implement the A050385 erratum workaround for XDP")
> Signed-off-by: Camelia Groza <camelia.groza@nxp.com>

Acked-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

> ---
>  drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> index e1d041c35ad9..78dfa05f6d55 100644
> --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> @@ -2192,7 +2192,7 @@ static int dpaa_a050385_wa_xdpf(struct dpaa_priv *priv,
>  	 * byte frame headroom. If the XDP program uses all of it, copy the
>  	 * data to a new buffer and make room for storing the backpointer.
>  	 */
> -	if (PTR_IS_ALIGNED(xdpf->data, DPAA_A050385_ALIGN) &&
> +	if (PTR_IS_ALIGNED(xdpf->data, DPAA_FD_DATA_ALIGNMENT) &&
>  	    xdpf->headroom >= priv->tx_headroom) {
>  		xdpf->headroom = priv->tx_headroom;
>  		return 0;
> -- 
> 2.17.1
> 

[PATCH net 3/3] dpaa_eth: try to move the data in place for the A050385 erratum

[Camelia Groza]

The XDP frame's headroom might be large enough to accommodate the
xdpf backpointer as well as shifting the data to an aligned address.

Try this first before resorting to allocating a new buffer and copying
the data.

Suggested-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Signed-off-by: Camelia Groza <camelia.groza@nxp.com>
---
 .../net/ethernet/freescale/dpaa/dpaa_eth.c    | 20 ++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
index 78dfa05f6d55..d093b56dc30f 100644
--- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
+++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
@@ -2180,8 +2180,9 @@ static int dpaa_a050385_wa_xdpf(struct dpaa_priv *priv,
 				struct xdp_frame **init_xdpf)
 {
 	struct xdp_frame *new_xdpf, *xdpf = *init_xdpf;
-	void *new_buff;
+	void *new_buff, *aligned_data;
 	struct page *p;
+	u32 data_shift;
 	int headroom;
 
 	/* Check the data alignment and make sure the headroom is large
@@ -2198,6 +2199,23 @@ static int dpaa_a050385_wa_xdpf(struct dpaa_priv *priv,
 		return 0;
 	}
 
+	/* Try to move the data inside the buffer just enough to align it and
+	 * store the xdpf backpointer. If the available headroom isn't large
+	 * enough, resort to allocating a new buffer and copying the data.
+	 */
+	aligned_data = PTR_ALIGN_DOWN(xdpf->data, DPAA_FD_DATA_ALIGNMENT);
+	data_shift = xdpf->data - aligned_data;
+
+	/* The XDP frame's headroom needs to be large enough to accommodate
+	 * shifting the data as well as storing the xdpf backpointer.
+	 */
+	if (xdpf->headroom  >= data_shift + priv->tx_headroom) {
+		memmove(aligned_data, xdpf->data, xdpf->len);
+		xdpf->data = aligned_data;
+		xdpf->headroom = priv->tx_headroom;
+		return 0;
+	}
+
 	/* The new xdp_frame is stored in the new buffer. Reserve enough space
 	 * in the headroom for storing it along with the driver's private
 	 * info. The headroom needs to be aligned to DPAA_FD_DATA_ALIGNMENT to
-- 
2.17.1

Re: [PATCH net 3/3] dpaa_eth: try to move the data in place for the A050385 erratum

[Maciej Fijalkowski]

On Tue, Feb 02, 2021 at 07:34:44PM +0200, Camelia Groza wrote:
> The XDP frame's headroom might be large enough to accommodate the
> xdpf backpointer as well as shifting the data to an aligned address.
> 
> Try this first before resorting to allocating a new buffer and copying
> the data.
> 
> Suggested-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> Signed-off-by: Camelia Groza <camelia.groza@nxp.com>

Acked-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

> ---
>  .../net/ethernet/freescale/dpaa/dpaa_eth.c    | 20 ++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> index 78dfa05f6d55..d093b56dc30f 100644
> --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> @@ -2180,8 +2180,9 @@ static int dpaa_a050385_wa_xdpf(struct dpaa_priv *priv,
>  				struct xdp_frame **init_xdpf)
>  {
>  	struct xdp_frame *new_xdpf, *xdpf = *init_xdpf;
> -	void *new_buff;
> +	void *new_buff, *aligned_data;
>  	struct page *p;
> +	u32 data_shift;
>  	int headroom;
>  
>  	/* Check the data alignment and make sure the headroom is large
> @@ -2198,6 +2199,23 @@ static int dpaa_a050385_wa_xdpf(struct dpaa_priv *priv,
>  		return 0;
>  	}
>  
> +	/* Try to move the data inside the buffer just enough to align it and
> +	 * store the xdpf backpointer. If the available headroom isn't large
> +	 * enough, resort to allocating a new buffer and copying the data.
> +	 */
> +	aligned_data = PTR_ALIGN_DOWN(xdpf->data, DPAA_FD_DATA_ALIGNMENT);
> +	data_shift = xdpf->data - aligned_data;
> +
> +	/* The XDP frame's headroom needs to be large enough to accommodate
> +	 * shifting the data as well as storing the xdpf backpointer.
> +	 */
> +	if (xdpf->headroom  >= data_shift + priv->tx_headroom) {
> +		memmove(aligned_data, xdpf->data, xdpf->len);
> +		xdpf->data = aligned_data;
> +		xdpf->headroom = priv->tx_headroom;
> +		return 0;
> +	}
> +
>  	/* The new xdp_frame is stored in the new buffer. Reserve enough space
>  	 * in the headroom for storing it along with the driver's private
>  	 * info. The headroom needs to be aligned to DPAA_FD_DATA_ALIGNMENT to
> -- 
> 2.17.1
>