From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1E668C63777 for ; Sat, 28 Nov 2020 23:12:09 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id D1E2A2087C for ; Sat, 28 Nov 2020 23:12:08 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727455AbgK1XLr (ORCPT ); Sat, 28 Nov 2020 18:11:47 -0500 Received: from mga06.intel.com ([134.134.136.31]:8823 "EHLO mga06.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726865AbgK1XLr (ORCPT ); Sat, 28 Nov 2020 18:11:47 -0500 IronPort-SDR: eC2er5WRzdE1gNPCX9GjCa8U68Y0HuFMcALL1VfBsi/vJB3kI1UNu2B+gbHXmvHqGOxYUKoKR9 VBDYoAfEQMKg== X-IronPort-AV: E=McAfee;i="6000,8403,9819"; a="234110434" X-IronPort-AV: E=Sophos;i="5.78,378,1599548400"; d="scan'208";a="234110434" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 28 Nov 2020 15:11:06 -0800 IronPort-SDR: Cp17kW5etUjHj4LJLPKlVMFh7+0YcRLMS4ZQIjrGS1Wi5iWABGjowTFAYXVpWCitaKaZAInPJd W9hs12lFA9Kw== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.78,378,1599548400"; d="scan'208";a="344565729" Received: from ranger.igk.intel.com ([10.102.21.164]) by orsmga002.jf.intel.com with ESMTP; 28 Nov 2020 15:11:04 -0800 Date: Sun, 29 Nov 2020 00:02:35 +0100 From: Maciej Fijalkowski To: Camelia Alexandra Groza Cc: "kuba@kernel.org" , "brouer@redhat.com" , "saeed@kernel.org" , "davem@davemloft.net" , "Madalin Bucur (OSS)" , Ioana Ciornei , "netdev@vger.kernel.org" Subject: Re: [PATCH net-next v4 7/7] dpaa_eth: implement the A050385 erratum workaround for XDP Message-ID: <20201128230235.GB45349@ranger.igk.intel.com> References: <20201124205040.GA19977@ranger.igk.intel.com> <20201127144411.GC26825@ranger.igk.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.12.1 (2019-06-15) Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Fri, Nov 27, 2020 at 05:35:00PM +0000, Camelia Alexandra Groza wrote: > > -----Original Message----- > > From: Maciej Fijalkowski > > Sent: Friday, November 27, 2020 16:44 > > To: Camelia Alexandra Groza > > Cc: kuba@kernel.org; brouer@redhat.com; saeed@kernel.org; > > davem@davemloft.net; Madalin Bucur (OSS) > > ; Ioana Ciornei ; > > 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 > > > > Sent: Tuesday, November 24, 2020 22:51 > > > > To: Camelia Alexandra Groza > > > > Cc: kuba@kernel.org; brouer@redhat.com; saeed@kernel.org; > > > > davem@davemloft.net; Madalin Bucur (OSS) > > > > ; Ioana Ciornei ; > > > > 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 > > > > > Signed-off-by: Camelia Groza > > > > > --- > > > > > 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 > > > > >