Re: [PATCH net-next v4 7/7] dpaa_eth: implement the A050385 erratum workaround for XDP

[Maciej Fijalkowski <maciej.fijalkowski@intel.com>]

On Fri, Nov 27, 2020 at 05:35:00PM +0000, Camelia Alexandra Groza wrote:
> > -----Original Message-----
> > From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > Sent: Friday, November 27, 2020 16:44
> > To: Camelia Alexandra Groza <camelia.groza@nxp.com>
> > Cc: kuba@kernel.org; brouer@redhat.com; saeed@kernel.org;
> > davem@davemloft.net; Madalin Bucur (OSS)
> > <madalin.bucur@oss.nxp.com>; Ioana Ciornei <ioana.ciornei@nxp.com>;
> > netdev@vger.kernel.org
> > Subject: Re: [PATCH net-next v4 7/7] dpaa_eth: implement the A050385
> > erratum workaround for XDP
> > 
> > On Wed, Nov 25, 2020 at 03:52:33PM +0000, Camelia Alexandra Groza wrote:
> > > > -----Original Message-----
> > > > From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > > > Sent: Tuesday, November 24, 2020 22:51
> > > > To: Camelia Alexandra Groza <camelia.groza@nxp.com>
> > > > Cc: kuba@kernel.org; brouer@redhat.com; saeed@kernel.org;
> > > > davem@davemloft.net; Madalin Bucur (OSS)
> > > > <madalin.bucur@oss.nxp.com>; Ioana Ciornei <ioana.ciornei@nxp.com>;
> > > > netdev@vger.kernel.org
> > > > Subject: Re: [PATCH net-next v4 7/7] dpaa_eth: implement the A050385
> > > > erratum workaround for XDP
> > > >
> > > > On Mon, Nov 23, 2020 at 07:36:25PM +0200, Camelia Groza wrote:
> > > > > For XDP TX, even tough we start out with correctly aligned buffers, the
> > > > > XDP program might change the data's alignment. For REDIRECT, we
> > have no
> > > > > control over the alignment either.
> > > > >
> > > > > Create a new workaround for xdp_frame structures to verify the
> > erratum
> > > > > conditions and move the data to a fresh buffer if necessary. Create a
> > new
> > > > > xdp_frame for managing the new buffer and free the old one using the
> > > > XDP
> > > > > API.
> > > > >
> > > > > Due to alignment constraints, all frames have a 256 byte headroom that
> > > > > is offered fully to XDP under the erratum. If the XDP program uses all
> > > > > of it, the data needs to be move to make room for the xdpf
> > backpointer.
> > > >
> > > > Out of curiosity, wouldn't it be easier to decrease the headroom that is
> > > > given to xdp rather doing to full copy of a buffer in case you miss a few
> > > > bytes on headroom?
> > >
> > > First of all, I'm not sure if offering less than XDP_PACKET_HEADROOM to
> > XDP programs is allowed. Second, we require the data to be strictly aligned
> > under this erratum. This first condition would be broken even if we reduce
> > the size of the offered headroom.
> > >
> > > > >
> > > > > Disable the metadata support since the information can be lost.
> > > > >
> > > > > Acked-by: Madalin Bucur <madalin.bucur@oss.nxp.com>
> > > > > Signed-off-by: Camelia Groza <camelia.groza@nxp.com>
> > > > > ---
> > > > >  drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 82
> > > > +++++++++++++++++++++++++-
> > > > >  1 file changed, 79 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > > > b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > > > > index 149b549..d8fc19d 100644
> > > > > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > > > > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > > > > @@ -2170,6 +2170,52 @@ static int dpaa_a050385_wa_skb(struct
> > > > net_device *net_dev, struct sk_buff **s)
> > > > >
> > > > >  	return 0;
> > > > >  }
> > > > > +
> > > > > +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;
> > > > > +	struct page *p;
> > > > > +
> > > > > +	/* Check the data alignment and make sure the headroom is large
> > > > > +	 * enough to store the xdpf backpointer. Use an aligned headroom
> > > > > +	 * value.
> > > > > +	 *
> > > > > +	 * Due to alignment constraints, we give XDP access to the full 256
> > > > > +	 * 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) &&
> > > > > +	    xdpf->headroom >= priv->tx_headroom) {
> > > > > +		xdpf->headroom = priv->tx_headroom;
> > > > > +		return 0;
> > > > > +	}
> > > > > +
> > > > > +	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);
> > > > > +
> > > > > +	/* 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->len = xdpf->len;
> > > > > +	new_xdpf->headroom = priv->tx_headroom;
> > > >
> > > > What if ptr was not aligned so you got here but tx_headroom was less
> > than
> > > > xdpf->headroom? Shouldn't you choose the bigger one? Aren't you
> > shrinking
> > > > the headroom for this case.
> > >
> > > Yes, I am shrinking the headroom. At this point, the headroom's content
> > isn't relevant anymore (this path is executed when transmitting the frame
> > after TX or REDIRECT). What is important is the data's alignment, and it is
> > dictated by the headroom's (fd's offset) size.
> > 
> > So would it be possible to do a memmove within the existing buffer under
> > some circumstances and then have this current logic as a worst case
> > scenario? Majority of XDP progs won't consume all of the XDP headroom so I
> > think it's something worth pursuing.
> > 
> > Please also tell us the performance impact of that workaround. Grabbing
> > new page followed by memcpy is expensive.
> 
> Yes, using memmove() might be an optimization if enough headroom is available to shift the data. Thanks for the suggestion. I can send this in separately as an optimization if you don't think is a blocker and if there aren't any other comments on v5.

Fine, but please remember to provide the numbers that I asked for.

> 
> I don't have numbers to share at the moment for the performance impact.
> 
> > >
> > > > > +	new_xdpf->frame_sz = DPAA_BP_RAW_SIZE;
> > > > > +	new_xdpf->mem.type = MEM_TYPE_PAGE_ORDER0;
> > > > > +
> > > > > +	/* Release the initial buffer */
> > > > > +	xdp_return_frame_rx_napi(xdpf);
> > > > > +
> > > > > +	*init_xdpf = new_xdpf;
> > > > > +	return 0;
> > > > > +}
> > > > >  #endif
> > > > >
> > > > >  static netdev_tx_t
> > > > > @@ -2406,6 +2452,15 @@ static int dpaa_xdp_xmit_frame(struct
> > > > net_device *net_dev,
> > > > >  	percpu_priv = this_cpu_ptr(priv->percpu_priv);
> > > > >  	percpu_stats = &percpu_priv->stats;
> > > > >
> > > > > +#ifdef CONFIG_DPAA_ERRATUM_A050385
> > > > > +	if (unlikely(fman_has_errata_a050385())) {
> > > > > +		if (dpaa_a050385_wa_xdpf(priv, &xdpf)) {
> > > > > +			err = -ENOMEM;
> > > > > +			goto out_error;
> > > > > +		}
> > > > > +	}
> > > > > +#endif
> > > > > +
> > > > >  	if (xdpf->headroom < DPAA_TX_PRIV_DATA_SIZE) {
> > > > >  		err = -EINVAL;
> > > > >  		goto out_error;
> > > > > @@ -2479,6 +2534,20 @@ static u32 dpaa_run_xdp(struct dpaa_priv
> > *priv,
> > > > struct qm_fd *fd, void *vaddr,
> > > > >  	xdp.frame_sz = DPAA_BP_RAW_SIZE - DPAA_TX_PRIV_DATA_SIZE;
> > > > >  	xdp.rxq = &dpaa_fq->xdp_rxq;
> > > > >
> > > > > +	/* We reserve a fixed headroom of 256 bytes under the erratum and
> > > > we
> > > > > +	 * offer it all to XDP programs to use. If no room is left for the
> > > > > +	 * xdpf backpointer on TX, we will need to copy the data.
> > > > > +	 * Disable metadata support since data realignments might be
> > > > required
> > > > > +	 * and the information can be lost.
> > > > > +	 */
> > > > > +#ifdef CONFIG_DPAA_ERRATUM_A050385
> > > > > +	if (unlikely(fman_has_errata_a050385())) {
> > > > > +		xdp_set_data_meta_invalid(&xdp);
> > > > > +		xdp.data_hard_start = vaddr;
> > > > > +		xdp.frame_sz = DPAA_BP_RAW_SIZE;
> > > > > +	}
> > > > > +#endif
> > > > > +
> > > > >  	xdp_act = bpf_prog_run_xdp(xdp_prog, &xdp);
> > > > >
> > > > >  	/* Update the length and the offset of the FD */
> > > > > @@ -2486,7 +2555,8 @@ static u32 dpaa_run_xdp(struct dpaa_priv
> > *priv,
> > > > struct qm_fd *fd, void *vaddr,
> > > > >
> > > > >  	switch (xdp_act) {
> > > > >  	case XDP_PASS:
> > > > > -		*xdp_meta_len = xdp.data - xdp.data_meta;
> > > > > +		*xdp_meta_len = xdp_data_meta_unsupported(&xdp) ? 0 :
> > > > > +				xdp.data - xdp.data_meta;
> > > >
> > > > You could consider surrounding this with ifdef and keep the old version in
> > > > the else branch so that old case is not hurt with that additional branch
> > > > that you're introducing with that ternary operator.
> > >
> > > Sure, I'll do that. Thanks.
> > >
> > > > >  		break;
> > > > >  	case XDP_TX:
> > > > >  		/* We can access the full headroom when sending the frame
> > > > > @@ -3188,10 +3258,16 @@ static u16 dpaa_get_headroom(struct
> > > > dpaa_buffer_layout *bl,
> > > > >  	 */
> > > > >  	headroom = (u16)(bl[port].priv_data_size + DPAA_HWA_SIZE);
> > > > >
> > > > > -	if (port == RX)
> > > > > +	if (port == RX) {
> > > > > +#ifdef CONFIG_DPAA_ERRATUM_A050385
> > > > > +		if (unlikely(fman_has_errata_a050385()))
> > > > > +			headroom = XDP_PACKET_HEADROOM;
> > > > > +#endif
> > > > > +
> > > > >  		return ALIGN(headroom,
> > > > DPAA_FD_RX_DATA_ALIGNMENT);
> > > > > -	else
> > > > > +	} else {
> > > > >  		return ALIGN(headroom, DPAA_FD_DATA_ALIGNMENT);
> > > > > +	}
> > > > >  }
> > > > >
> > > > >  static int dpaa_eth_probe(struct platform_device *pdev)
> > > > > --
> > > > > 1.9.1
> > > > >