| Message ID | 1549299625-28399-2-git-send-email-ruxandra.radulescu@nxp.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | dpaa2-eth: Driver updates | expand |
Hi Ioana, Can you share any results on XDP (XDP_DROP is usually useful for the hardware capabilities). > Instead of allocating page fragments via the network stack, > use the page allocator directly. For now, we consume one page > for each Rx buffer. > > With the new memory model we are free to consider adding more > XDP support. > > Performance decreases slightly in some IP forwarding cases. > No visible effect on termination traffic. The driver memory > footprint increases as a result of this change, but it is > still small enough to not really matter. > > Another side effect is that now Rx buffer alignment requirements > are naturally satisfied without any additional actions needed. > Remove alignment related code, except in the buffer layout > information conveyed to MC, as hardware still needs to know the > alignment value we guarantee. > > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com> > Signed-off-by: Ioana Radulescu <ruxandra.radulescu@nxp.com> > --- > drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c | 61 +++++++++++++----------- > drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h | 21 +++----- > 2 files changed, 38 insertions(+), 44 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > index 04925c7..6e58de6 100644 > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > @@ -86,16 +86,16 @@ static void free_rx_fd(struct dpaa2_eth_priv *priv, > for (i = 1; i < DPAA2_ETH_MAX_SG_ENTRIES; i++) { > addr = dpaa2_sg_get_addr(&sgt[i]); > sg_vaddr = dpaa2_iova_to_virt(priv->iommu_domain, addr); > - dma_unmap_single(dev, addr, DPAA2_ETH_RX_BUF_SIZE, > - DMA_BIDIRECTIONAL); > + dma_unmap_page(dev, addr, DPAA2_ETH_RX_BUF_SIZE, > + DMA_BIDIRECTIONAL); > > - skb_free_frag(sg_vaddr); > + free_pages((unsigned long)sg_vaddr, 0); > if (dpaa2_sg_is_final(&sgt[i])) > break; > } > > free_buf: > - skb_free_frag(vaddr); > + free_pages((unsigned long)vaddr, 0); > } > > /* Build a linear skb based on a single-buffer frame descriptor */ > @@ -109,7 +109,7 @@ static struct sk_buff *build_linear_skb(struct dpaa2_eth_channel *ch, > > ch->buf_count--; > > - skb = build_skb(fd_vaddr, DPAA2_ETH_SKB_SIZE); > + skb = build_skb(fd_vaddr, DPAA2_ETH_RX_BUF_RAW_SIZE); > if (unlikely(!skb)) > return NULL; > > @@ -144,19 +144,19 @@ static struct sk_buff *build_frag_skb(struct dpaa2_eth_priv *priv, > /* Get the address and length from the S/G entry */ > sg_addr = dpaa2_sg_get_addr(sge); > sg_vaddr = dpaa2_iova_to_virt(priv->iommu_domain, sg_addr); > - dma_unmap_single(dev, sg_addr, DPAA2_ETH_RX_BUF_SIZE, > - DMA_BIDIRECTIONAL); > + dma_unmap_page(dev, sg_addr, DPAA2_ETH_RX_BUF_SIZE, > + DMA_BIDIRECTIONAL); > > sg_length = dpaa2_sg_get_len(sge); > > if (i == 0) { > /* We build the skb around the first data buffer */ > - skb = build_skb(sg_vaddr, DPAA2_ETH_SKB_SIZE); > + skb = build_skb(sg_vaddr, DPAA2_ETH_RX_BUF_RAW_SIZE); > if (unlikely(!skb)) { > /* Free the first SG entry now, since we already > * unmapped it and obtained the virtual address > */ > - skb_free_frag(sg_vaddr); > + free_pages((unsigned long)sg_vaddr, 0); > > /* We still need to subtract the buffers used > * by this FD from our software counter > @@ -211,9 +211,9 @@ static void free_bufs(struct dpaa2_eth_priv *priv, u64 *buf_array, int count) > > for (i = 0; i < count; i++) { > vaddr = dpaa2_iova_to_virt(priv->iommu_domain, buf_array[i]); > - dma_unmap_single(dev, buf_array[i], DPAA2_ETH_RX_BUF_SIZE, > - DMA_BIDIRECTIONAL); > - skb_free_frag(vaddr); > + dma_unmap_page(dev, buf_array[i], DPAA2_ETH_RX_BUF_SIZE, > + DMA_BIDIRECTIONAL); > + free_pages((unsigned long)vaddr, 0); I got some hardware/XDP related questions since i have no idea how the hardware works. From what i understand on the code, you are trying to manage the buffer from the hw buffer manager and if the fails you unmap and free the pages. Since XDP relies on recycling for speed (hint check xdp_return_buff()), is it possible to recycle the buffer if the hw fails ? > } > } > > @@ -378,16 +378,16 @@ static void dpaa2_eth_rx(struct dpaa2_eth_priv *priv, > return; > } > > - dma_unmap_single(dev, addr, DPAA2_ETH_RX_BUF_SIZE, > - DMA_BIDIRECTIONAL); > + dma_unmap_page(dev, addr, DPAA2_ETH_RX_BUF_SIZE, > + DMA_BIDIRECTIONAL); > skb = build_linear_skb(ch, fd, vaddr); > } else if (fd_format == dpaa2_fd_sg) { > WARN_ON(priv->xdp_prog); > > - dma_unmap_single(dev, addr, DPAA2_ETH_RX_BUF_SIZE, > - DMA_BIDIRECTIONAL); > + dma_unmap_page(dev, addr, DPAA2_ETH_RX_BUF_SIZE, > + DMA_BIDIRECTIONAL); > skb = build_frag_skb(priv, ch, buf_data); > - skb_free_frag(vaddr); > + free_pages((unsigned long)vaddr, 0); > percpu_extras->rx_sg_frames++; > percpu_extras->rx_sg_bytes += dpaa2_fd_get_len(fd); > } else { > @@ -903,7 +903,7 @@ static int add_bufs(struct dpaa2_eth_priv *priv, > { > struct device *dev = priv->net_dev->dev.parent; > u64 buf_array[DPAA2_ETH_BUFS_PER_CMD]; > - void *buf; > + struct page *page; > dma_addr_t addr; > int i, err; > > @@ -911,14 +911,16 @@ static int add_bufs(struct dpaa2_eth_priv *priv, > /* Allocate buffer visible to WRIOP + skb shared info + > * alignment padding > */ > - buf = napi_alloc_frag(dpaa2_eth_buf_raw_size(priv)); > - if (unlikely(!buf)) > + /* allocate one page for each Rx buffer. WRIOP sees > + * the entire page except for a tailroom reserved for > + * skb shared info > + */ > + page = dev_alloc_pages(0); > + if (!page) > goto err_alloc; > > - buf = PTR_ALIGN(buf, priv->rx_buf_align); > - > - addr = dma_map_single(dev, buf, DPAA2_ETH_RX_BUF_SIZE, > - DMA_BIDIRECTIONAL); > + addr = dma_map_page(dev, page, 0, DPAA2_ETH_RX_BUF_SIZE, > + DMA_BIDIRECTIONAL); > if (unlikely(dma_mapping_error(dev, addr))) > goto err_map; > > @@ -926,7 +928,7 @@ static int add_bufs(struct dpaa2_eth_priv *priv, > > /* tracing point */ > trace_dpaa2_eth_buf_seed(priv->net_dev, > - buf, dpaa2_eth_buf_raw_size(priv), > + page, DPAA2_ETH_RX_BUF_RAW_SIZE, > addr, DPAA2_ETH_RX_BUF_SIZE, > bpid); > } > @@ -948,7 +950,7 @@ static int add_bufs(struct dpaa2_eth_priv *priv, > return i; > > err_map: > - skb_free_frag(buf); > + __free_pages(page, 0); > err_alloc: > /* If we managed to allocate at least some buffers, > * release them to hardware > @@ -2134,6 +2136,7 @@ static int set_buffer_layout(struct dpaa2_eth_priv *priv) > { > struct device *dev = priv->net_dev->dev.parent; > struct dpni_buffer_layout buf_layout = {0}; > + u16 rx_buf_align; > int err; > > /* We need to check for WRIOP version 1.0.0, but depending on the MC > @@ -2142,9 +2145,9 @@ static int set_buffer_layout(struct dpaa2_eth_priv *priv) > */ > if (priv->dpni_attrs.wriop_version == DPAA2_WRIOP_VERSION(0, 0, 0) || > priv->dpni_attrs.wriop_version == DPAA2_WRIOP_VERSION(1, 0, 0)) > - priv->rx_buf_align = DPAA2_ETH_RX_BUF_ALIGN_REV1; > + rx_buf_align = DPAA2_ETH_RX_BUF_ALIGN_REV1; > else > - priv->rx_buf_align = DPAA2_ETH_RX_BUF_ALIGN; > + rx_buf_align = DPAA2_ETH_RX_BUF_ALIGN; > > /* tx buffer */ > buf_layout.private_data_size = DPAA2_ETH_SWA_SIZE; > @@ -2184,7 +2187,7 @@ static int set_buffer_layout(struct dpaa2_eth_priv *priv) > /* rx buffer */ > buf_layout.pass_frame_status = true; > buf_layout.pass_parser_result = true; > - buf_layout.data_align = priv->rx_buf_align; > + buf_layout.data_align = rx_buf_align; > buf_layout.data_head_room = dpaa2_eth_rx_head_room(priv); > buf_layout.private_data_size = 0; > buf_layout.options = DPNI_BUF_LAYOUT_OPT_PARSER_RESULT | > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h > index 31fe486..da3d039 100644 > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h > @@ -63,9 +63,11 @@ > /* Hardware requires alignment for ingress/egress buffer addresses */ > #define DPAA2_ETH_TX_BUF_ALIGN 64 > > -#define DPAA2_ETH_RX_BUF_SIZE 2048 > -#define DPAA2_ETH_SKB_SIZE \ > - (DPAA2_ETH_RX_BUF_SIZE + SKB_DATA_ALIGN(sizeof(struct skb_shared_info))) > +#define DPAA2_ETH_RX_BUF_RAW_SIZE PAGE_SIZE > +#define DPAA2_ETH_RX_BUF_TAILROOM \ > + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) > +#define DPAA2_ETH_RX_BUF_SIZE \ > + (DPAA2_ETH_RX_BUF_RAW_SIZE - DPAA2_ETH_RX_BUF_TAILROOM) > > /* Hardware annotation area in RX/TX buffers */ > #define DPAA2_ETH_RX_HWA_SIZE 64 > @@ -343,7 +345,6 @@ struct dpaa2_eth_priv { > bool rx_tstamp; /* Rx timestamping enabled */ > > u16 tx_qdid; > - u16 rx_buf_align; > struct fsl_mc_io *mc_io; > /* Cores which have an affine DPIO/DPCON. > * This is the cpu set on which Rx and Tx conf frames are processed > @@ -418,15 +419,6 @@ enum dpaa2_eth_rx_dist { > DPAA2_ETH_RX_DIST_CLS > }; > > -/* Hardware only sees DPAA2_ETH_RX_BUF_SIZE, but the skb built around > - * the buffer also needs space for its shared info struct, and we need > - * to allocate enough to accommodate hardware alignment restrictions > - */ > -static inline unsigned int dpaa2_eth_buf_raw_size(struct dpaa2_eth_priv *priv) > -{ > - return DPAA2_ETH_SKB_SIZE + priv->rx_buf_align; > -} > - > static inline > unsigned int dpaa2_eth_needed_headroom(struct dpaa2_eth_priv *priv, > struct sk_buff *skb) > @@ -451,8 +443,7 @@ unsigned int dpaa2_eth_needed_headroom(struct dpaa2_eth_priv *priv, > */ > static inline unsigned int dpaa2_eth_rx_head_room(struct dpaa2_eth_priv *priv) > { > - return priv->tx_data_offset + DPAA2_ETH_TX_BUF_ALIGN - > - DPAA2_ETH_RX_HWA_SIZE; > + return priv->tx_data_offset - DPAA2_ETH_RX_HWA_SIZE; > } > > int dpaa2_eth_set_hash(struct net_device *net_dev, u64 flags); > -- > 2.7.4 >
> -----Original Message----- > From: Ilias Apalodimas <ilias.apalodimas@linaro.org> > Sent: Tuesday, February 5, 2019 11:35 AM > To: Ioana Ciocoi Radulescu <ruxandra.radulescu@nxp.com> > Cc: netdev@vger.kernel.org; davem@davemloft.net; Ioana Ciornei > <ioana.ciornei@nxp.com>; brouer@redhat.com > Subject: Re: [PATCH net-next 1/4] dpaa2-eth: Use a single page per Rx buffer > > Hi Ioana, > > Can you share any results on XDP (XDP_DROP is usually useful for the > hardware capabilities). XDP numbers are pretty much the same as before this patch: On a LS2088A with A72 cores @2GHz (numbers in Mpps): 1core 8cores ------------------------------------------------------------------------- XDP_DROP (no touching data) 5.37 29.6 (linerate) XDP_DROP (xdp1 sample) 3.14 24.22 XDP_TX(xdp2 sample) 1.65 13.08 > > > Instead of allocating page fragments via the network stack, > > use the page allocator directly. For now, we consume one page > > for each Rx buffer. > > > > With the new memory model we are free to consider adding more > > XDP support. > > > > Performance decreases slightly in some IP forwarding cases. > > No visible effect on termination traffic. The driver memory > > footprint increases as a result of this change, but it is > > still small enough to not really matter. > > > > Another side effect is that now Rx buffer alignment requirements > > are naturally satisfied without any additional actions needed. > > Remove alignment related code, except in the buffer layout > > information conveyed to MC, as hardware still needs to know the > > alignment value we guarantee. > > > > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com> > > Signed-off-by: Ioana Radulescu <ruxandra.radulescu@nxp.com> > > --- > > drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c | 61 +++++++++++++- > ---------- > > drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h | 21 +++----- > > 2 files changed, 38 insertions(+), 44 deletions(-) > > > > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > > index 04925c7..6e58de6 100644 > > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > > @@ -86,16 +86,16 @@ static void free_rx_fd(struct dpaa2_eth_priv *priv, > > for (i = 1; i < DPAA2_ETH_MAX_SG_ENTRIES; i++) { > > addr = dpaa2_sg_get_addr(&sgt[i]); > > sg_vaddr = dpaa2_iova_to_virt(priv->iommu_domain, addr); > > - dma_unmap_single(dev, addr, DPAA2_ETH_RX_BUF_SIZE, > > - DMA_BIDIRECTIONAL); > > + dma_unmap_page(dev, addr, DPAA2_ETH_RX_BUF_SIZE, > > + DMA_BIDIRECTIONAL); > > > > - skb_free_frag(sg_vaddr); > > + free_pages((unsigned long)sg_vaddr, 0); > > if (dpaa2_sg_is_final(&sgt[i])) > > break; > > } > > > > free_buf: > > - skb_free_frag(vaddr); > > + free_pages((unsigned long)vaddr, 0); > > } > > > > /* Build a linear skb based on a single-buffer frame descriptor */ > > @@ -109,7 +109,7 @@ static struct sk_buff *build_linear_skb(struct > dpaa2_eth_channel *ch, > > > > ch->buf_count--; > > > > - skb = build_skb(fd_vaddr, DPAA2_ETH_SKB_SIZE); > > + skb = build_skb(fd_vaddr, DPAA2_ETH_RX_BUF_RAW_SIZE); > > if (unlikely(!skb)) > > return NULL; > > > > @@ -144,19 +144,19 @@ static struct sk_buff *build_frag_skb(struct > dpaa2_eth_priv *priv, > > /* Get the address and length from the S/G entry */ > > sg_addr = dpaa2_sg_get_addr(sge); > > sg_vaddr = dpaa2_iova_to_virt(priv->iommu_domain, > sg_addr); > > - dma_unmap_single(dev, sg_addr, > DPAA2_ETH_RX_BUF_SIZE, > > - DMA_BIDIRECTIONAL); > > + dma_unmap_page(dev, sg_addr, DPAA2_ETH_RX_BUF_SIZE, > > + DMA_BIDIRECTIONAL); > > > > sg_length = dpaa2_sg_get_len(sge); > > > > if (i == 0) { > > /* We build the skb around the first data buffer */ > > - skb = build_skb(sg_vaddr, DPAA2_ETH_SKB_SIZE); > > + skb = build_skb(sg_vaddr, > DPAA2_ETH_RX_BUF_RAW_SIZE); > > if (unlikely(!skb)) { > > /* Free the first SG entry now, since we > already > > * unmapped it and obtained the virtual > address > > */ > > - skb_free_frag(sg_vaddr); > > + free_pages((unsigned long)sg_vaddr, 0); > > > > /* We still need to subtract the buffers used > > * by this FD from our software counter > > @@ -211,9 +211,9 @@ static void free_bufs(struct dpaa2_eth_priv *priv, > u64 *buf_array, int count) > > > > for (i = 0; i < count; i++) { > > vaddr = dpaa2_iova_to_virt(priv->iommu_domain, > buf_array[i]); > > - dma_unmap_single(dev, buf_array[i], > DPAA2_ETH_RX_BUF_SIZE, > > - DMA_BIDIRECTIONAL); > > - skb_free_frag(vaddr); > > + dma_unmap_page(dev, buf_array[i], > DPAA2_ETH_RX_BUF_SIZE, > > + DMA_BIDIRECTIONAL); > > + free_pages((unsigned long)vaddr, 0); > > I got some hardware/XDP related questions since i have no idea how the > hardware > works. From what i understand on the code, you are trying to manage the > buffer > from the hw buffer manager and if the fails you unmap and free the pages. > Since XDP relies on recycling for speed (hint check xdp_return_buff()), is it > possible to recycle the buffer if the hw fails ? We're still looking for a way to use the page pool mechanism in our driver. Our hardware buffer pool is shared between all Rx queues, so it's not that straightforward. This error path in particular is very unlikely to ever get exercised, it wouldn't have an impact on performance. Thanks, Ioana > > > } > > } > > > > @@ -378,16 +378,16 @@ static void dpaa2_eth_rx(struct dpaa2_eth_priv > *priv, > > return; > > } > > > > - dma_unmap_single(dev, addr, DPAA2_ETH_RX_BUF_SIZE, > > - DMA_BIDIRECTIONAL); > > + dma_unmap_page(dev, addr, DPAA2_ETH_RX_BUF_SIZE, > > + DMA_BIDIRECTIONAL); > > skb = build_linear_skb(ch, fd, vaddr); > > } else if (fd_format == dpaa2_fd_sg) { > > WARN_ON(priv->xdp_prog); > > > > - dma_unmap_single(dev, addr, DPAA2_ETH_RX_BUF_SIZE, > > - DMA_BIDIRECTIONAL); > > + dma_unmap_page(dev, addr, DPAA2_ETH_RX_BUF_SIZE, > > + DMA_BIDIRECTIONAL); > > skb = build_frag_skb(priv, ch, buf_data); > > - skb_free_frag(vaddr); > > + free_pages((unsigned long)vaddr, 0); > > percpu_extras->rx_sg_frames++; > > percpu_extras->rx_sg_bytes += dpaa2_fd_get_len(fd); > > } else { > > @@ -903,7 +903,7 @@ static int add_bufs(struct dpaa2_eth_priv *priv, > > { > > struct device *dev = priv->net_dev->dev.parent; > > u64 buf_array[DPAA2_ETH_BUFS_PER_CMD]; > > - void *buf; > > + struct page *page; > > dma_addr_t addr; > > int i, err; > > > > @@ -911,14 +911,16 @@ static int add_bufs(struct dpaa2_eth_priv *priv, > > /* Allocate buffer visible to WRIOP + skb shared info + > > * alignment padding > > */ > > - buf = napi_alloc_frag(dpaa2_eth_buf_raw_size(priv)); > > - if (unlikely(!buf)) > > + /* allocate one page for each Rx buffer. WRIOP sees > > + * the entire page except for a tailroom reserved for > > + * skb shared info > > + */ > > + page = dev_alloc_pages(0); > > + if (!page) > > goto err_alloc; > > > > - buf = PTR_ALIGN(buf, priv->rx_buf_align); > > - > > - addr = dma_map_single(dev, buf, > DPAA2_ETH_RX_BUF_SIZE, > > - DMA_BIDIRECTIONAL); > > + addr = dma_map_page(dev, page, 0, > DPAA2_ETH_RX_BUF_SIZE, > > + DMA_BIDIRECTIONAL); > > if (unlikely(dma_mapping_error(dev, addr))) > > goto err_map; > > > > @@ -926,7 +928,7 @@ static int add_bufs(struct dpaa2_eth_priv *priv, > > > > /* tracing point */ > > trace_dpaa2_eth_buf_seed(priv->net_dev, > > - buf, dpaa2_eth_buf_raw_size(priv), > > + page, > DPAA2_ETH_RX_BUF_RAW_SIZE, > > addr, DPAA2_ETH_RX_BUF_SIZE, > > bpid); > > } > > @@ -948,7 +950,7 @@ static int add_bufs(struct dpaa2_eth_priv *priv, > > return i; > > > > err_map: > > - skb_free_frag(buf); > > + __free_pages(page, 0); > > err_alloc: > > /* If we managed to allocate at least some buffers, > > * release them to hardware > > @@ -2134,6 +2136,7 @@ static int set_buffer_layout(struct > dpaa2_eth_priv *priv) > > { > > struct device *dev = priv->net_dev->dev.parent; > > struct dpni_buffer_layout buf_layout = {0}; > > + u16 rx_buf_align; > > int err; > > > > /* We need to check for WRIOP version 1.0.0, but depending on the > MC > > @@ -2142,9 +2145,9 @@ static int set_buffer_layout(struct > dpaa2_eth_priv *priv) > > */ > > if (priv->dpni_attrs.wriop_version == DPAA2_WRIOP_VERSION(0, 0, > 0) || > > priv->dpni_attrs.wriop_version == DPAA2_WRIOP_VERSION(1, 0, > 0)) > > - priv->rx_buf_align = DPAA2_ETH_RX_BUF_ALIGN_REV1; > > + rx_buf_align = DPAA2_ETH_RX_BUF_ALIGN_REV1; > > else > > - priv->rx_buf_align = DPAA2_ETH_RX_BUF_ALIGN; > > + rx_buf_align = DPAA2_ETH_RX_BUF_ALIGN; > > > > /* tx buffer */ > > buf_layout.private_data_size = DPAA2_ETH_SWA_SIZE; > > @@ -2184,7 +2187,7 @@ static int set_buffer_layout(struct > dpaa2_eth_priv *priv) > > /* rx buffer */ > > buf_layout.pass_frame_status = true; > > buf_layout.pass_parser_result = true; > > - buf_layout.data_align = priv->rx_buf_align; > > + buf_layout.data_align = rx_buf_align; > > buf_layout.data_head_room = dpaa2_eth_rx_head_room(priv); > > buf_layout.private_data_size = 0; > > buf_layout.options = DPNI_BUF_LAYOUT_OPT_PARSER_RESULT | > > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h > b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h > > index 31fe486..da3d039 100644 > > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h > > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h > > @@ -63,9 +63,11 @@ > > /* Hardware requires alignment for ingress/egress buffer addresses */ > > #define DPAA2_ETH_TX_BUF_ALIGN 64 > > > > -#define DPAA2_ETH_RX_BUF_SIZE 2048 > > -#define DPAA2_ETH_SKB_SIZE \ > > - (DPAA2_ETH_RX_BUF_SIZE + SKB_DATA_ALIGN(sizeof(struct > skb_shared_info))) > > +#define DPAA2_ETH_RX_BUF_RAW_SIZE PAGE_SIZE > > +#define DPAA2_ETH_RX_BUF_TAILROOM \ > > + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) > > +#define DPAA2_ETH_RX_BUF_SIZE \ > > + (DPAA2_ETH_RX_BUF_RAW_SIZE - > DPAA2_ETH_RX_BUF_TAILROOM) > > > > /* Hardware annotation area in RX/TX buffers */ > > #define DPAA2_ETH_RX_HWA_SIZE 64 > > @@ -343,7 +345,6 @@ struct dpaa2_eth_priv { > > bool rx_tstamp; /* Rx timestamping enabled */ > > > > u16 tx_qdid; > > - u16 rx_buf_align; > > struct fsl_mc_io *mc_io; > > /* Cores which have an affine DPIO/DPCON. > > * This is the cpu set on which Rx and Tx conf frames are processed > > @@ -418,15 +419,6 @@ enum dpaa2_eth_rx_dist { > > DPAA2_ETH_RX_DIST_CLS > > }; > > > > -/* Hardware only sees DPAA2_ETH_RX_BUF_SIZE, but the skb built > around > > - * the buffer also needs space for its shared info struct, and we need > > - * to allocate enough to accommodate hardware alignment restrictions > > - */ > > -static inline unsigned int dpaa2_eth_buf_raw_size(struct dpaa2_eth_priv > *priv) > > -{ > > - return DPAA2_ETH_SKB_SIZE + priv->rx_buf_align; > > -} > > - > > static inline > > unsigned int dpaa2_eth_needed_headroom(struct dpaa2_eth_priv *priv, > > struct sk_buff *skb) > > @@ -451,8 +443,7 @@ unsigned int dpaa2_eth_needed_headroom(struct > dpaa2_eth_priv *priv, > > */ > > static inline unsigned int dpaa2_eth_rx_head_room(struct dpaa2_eth_priv > *priv) > > { > > - return priv->tx_data_offset + DPAA2_ETH_TX_BUF_ALIGN - > > - DPAA2_ETH_RX_HWA_SIZE; > > + return priv->tx_data_offset - DPAA2_ETH_RX_HWA_SIZE; > > } > > > > int dpaa2_eth_set_hash(struct net_device *net_dev, u64 flags); > > -- > > 2.7.4 > >
On Wed, 6 Feb 2019 15:36:33 +0000 Ioana Ciocoi Radulescu <ruxandra.radulescu@nxp.com> wrote: > > From: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > > > Can you share any results on XDP (XDP_DROP is usually useful for the > > hardware capabilities). > > XDP numbers are pretty much the same as before this patch: > > On a LS2088A with A72 cores @2GHz (numbers in Mpps): > 1core 8cores > ------------------------------------------------------------------------- > XDP_DROP (no touching data) 5.37 29.6 (linerate) > XDP_DROP (xdp1 sample) 3.14 24.22 It is interesting/problematic to see that the cost of touching the data is so high 5.37Mpps -> 3.14Mpps. The Intel CPUs have solved this in hardware with DDIO, which delivers frame in L3-cache. I have some ideas on how to improve this on ARM (or CPUs without DDIO). I've previous implemented this as RFC on mlx4 tested on a CPU without DDIO, with great success 10mpps -> 20Mpps (but it was shutdown, as newer Intel HW solved the issue). The basic idea is to have an array of frames, that you start an L2/L3-prefetch on, before going "back" and process them for XDP or netstack. (p.s. this is the same DPDK does) --Jesper
> Subject: Re: [PATCH net-next 1/4] dpaa2-eth: Use a single page per Rx buffer > > > On Wed, 6 Feb 2019 15:36:33 +0000 Ioana Ciocoi Radulescu > <ruxandra.radulescu@nxp.com> wrote: > > > > From: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > > > > > Can you share any results on XDP (XDP_DROP is usually useful for the > > > hardware capabilities). > > > > XDP numbers are pretty much the same as before this patch: > > > > On a LS2088A with A72 cores @2GHz (numbers in Mpps): > > 1core 8cores > > ------------------------------------------------------------------------- > > XDP_DROP (no touching data) 5.37 29.6 (linerate) > > XDP_DROP (xdp1 sample) 3.14 24.22 > > It is interesting/problematic to see that the cost of touching the data is so high > 5.37Mpps -> 3.14Mpps. The Intel CPUs have solved this in hardware with DDIO, > which delivers frame in L3-cache. I have some ideas on how to improve this on > ARM (or CPUs without DDIO). I've previous implemented this as RFC on mlx4 > tested on a CPU without DDIO, with great success 10mpps -> 20Mpps (but it was > shutdown, as newer Intel HW solved the issue). The basic idea is to have an > array of frames, that you start an L2/L3-prefetch on, before going "back" and > process them for XDP or netstack. (p.s. this is the same DPDK does) Thanks for the hint. We are currently investigating what our options are. We'll come back with a more detailed answer once we figure out. Ioana C
> Subject: RE: [PATCH net-next 1/4] dpaa2-eth: Use a single page per Rx buffer > > > Subject: Re: [PATCH net-next 1/4] dpaa2-eth: Use a single page per Rx > > buffer > > > > > > On Wed, 6 Feb 2019 15:36:33 +0000 Ioana Ciocoi Radulescu > > <ruxandra.radulescu@nxp.com> wrote: > > > > > > From: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > > > > > > > Can you share any results on XDP (XDP_DROP is usually useful for > > > > the hardware capabilities). > > > > > > XDP numbers are pretty much the same as before this patch: > > > > > > On a LS2088A with A72 cores @2GHz (numbers in Mpps): > > > 1core 8cores > > > ------------------------------------------------------------------------- > > > XDP_DROP (no touching data) 5.37 29.6 (linerate) > > > XDP_DROP (xdp1 sample) 3.14 24.22 > > > > It is interesting/problematic to see that the cost of touching the > > data is so high 5.37Mpps -> 3.14Mpps. The Intel CPUs have solved this > > in hardware with DDIO, which delivers frame in L3-cache. I have some > > ideas on how to improve this on ARM (or CPUs without DDIO). I've > > previous implemented this as RFC on mlx4 tested on a CPU without DDIO, > > with great success 10mpps -> 20Mpps (but it was shutdown, as newer > > Intel HW solved the issue). The basic idea is to have an array of > > frames, that you start an L2/L3-prefetch on, before going "back" and > > process them for XDP or netstack. (p.s. this is the same DPDK does) > > Thanks for the hint. We are currently investigating what our options are. > We'll come back with a more detailed answer once we figure out. > Hi, We have enabled cache stashing of frame data in our solution and sent the patches upstream: https://lkml.org/lkml/2019/2/23/41. Some results with the patches applied on top of the net-next tree are below. LS2088A with A72 cores @2GHz (numbers in Mpps): 1core 8cores -------------------------------------------------------------------------------------------------------- XDP_DROP (no touching data) 5.37 29.6 (linerate) XDP_DROP (xdp1 sample, no stashing) 3.28 24.2 XDP_DROP (xdp1 sample, stashing) 4.64 26.85 Thanks a lot for your hint. Ioana C
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c index 04925c7..6e58de6 100644 --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c @@ -86,16 +86,16 @@ static void free_rx_fd(struct dpaa2_eth_priv *priv, for (i = 1; i < DPAA2_ETH_MAX_SG_ENTRIES; i++) { addr = dpaa2_sg_get_addr(&sgt[i]); sg_vaddr = dpaa2_iova_to_virt(priv->iommu_domain, addr); - dma_unmap_single(dev, addr, DPAA2_ETH_RX_BUF_SIZE, - DMA_BIDIRECTIONAL); + dma_unmap_page(dev, addr, DPAA2_ETH_RX_BUF_SIZE, + DMA_BIDIRECTIONAL); - skb_free_frag(sg_vaddr); + free_pages((unsigned long)sg_vaddr, 0); if (dpaa2_sg_is_final(&sgt[i])) break; } free_buf: - skb_free_frag(vaddr); + free_pages((unsigned long)vaddr, 0); } /* Build a linear skb based on a single-buffer frame descriptor */ @@ -109,7 +109,7 @@ static struct sk_buff *build_linear_skb(struct dpaa2_eth_channel *ch, ch->buf_count--; - skb = build_skb(fd_vaddr, DPAA2_ETH_SKB_SIZE); + skb = build_skb(fd_vaddr, DPAA2_ETH_RX_BUF_RAW_SIZE); if (unlikely(!skb)) return NULL; @@ -144,19 +144,19 @@ static struct sk_buff *build_frag_skb(struct dpaa2_eth_priv *priv, /* Get the address and length from the S/G entry */ sg_addr = dpaa2_sg_get_addr(sge); sg_vaddr = dpaa2_iova_to_virt(priv->iommu_domain, sg_addr); - dma_unmap_single(dev, sg_addr, DPAA2_ETH_RX_BUF_SIZE, - DMA_BIDIRECTIONAL); + dma_unmap_page(dev, sg_addr, DPAA2_ETH_RX_BUF_SIZE, + DMA_BIDIRECTIONAL); sg_length = dpaa2_sg_get_len(sge); if (i == 0) { /* We build the skb around the first data buffer */ - skb = build_skb(sg_vaddr, DPAA2_ETH_SKB_SIZE); + skb = build_skb(sg_vaddr, DPAA2_ETH_RX_BUF_RAW_SIZE); if (unlikely(!skb)) { /* Free the first SG entry now, since we already * unmapped it and obtained the virtual address */ - skb_free_frag(sg_vaddr); + free_pages((unsigned long)sg_vaddr, 0); /* We still need to subtract the buffers used * by this FD from our software counter @@ -211,9 +211,9 @@ static void free_bufs(struct dpaa2_eth_priv *priv, u64 *buf_array, int count) for (i = 0; i < count; i++) { vaddr = dpaa2_iova_to_virt(priv->iommu_domain, buf_array[i]); - dma_unmap_single(dev, buf_array[i], DPAA2_ETH_RX_BUF_SIZE, - DMA_BIDIRECTIONAL); - skb_free_frag(vaddr); + dma_unmap_page(dev, buf_array[i], DPAA2_ETH_RX_BUF_SIZE, + DMA_BIDIRECTIONAL); + free_pages((unsigned long)vaddr, 0); } } @@ -378,16 +378,16 @@ static void dpaa2_eth_rx(struct dpaa2_eth_priv *priv, return; } - dma_unmap_single(dev, addr, DPAA2_ETH_RX_BUF_SIZE, - DMA_BIDIRECTIONAL); + dma_unmap_page(dev, addr, DPAA2_ETH_RX_BUF_SIZE, + DMA_BIDIRECTIONAL); skb = build_linear_skb(ch, fd, vaddr); } else if (fd_format == dpaa2_fd_sg) { WARN_ON(priv->xdp_prog); - dma_unmap_single(dev, addr, DPAA2_ETH_RX_BUF_SIZE, - DMA_BIDIRECTIONAL); + dma_unmap_page(dev, addr, DPAA2_ETH_RX_BUF_SIZE, + DMA_BIDIRECTIONAL); skb = build_frag_skb(priv, ch, buf_data); - skb_free_frag(vaddr); + free_pages((unsigned long)vaddr, 0); percpu_extras->rx_sg_frames++; percpu_extras->rx_sg_bytes += dpaa2_fd_get_len(fd); } else { @@ -903,7 +903,7 @@ static int add_bufs(struct dpaa2_eth_priv *priv, { struct device *dev = priv->net_dev->dev.parent; u64 buf_array[DPAA2_ETH_BUFS_PER_CMD]; - void *buf; + struct page *page; dma_addr_t addr; int i, err; @@ -911,14 +911,16 @@ static int add_bufs(struct dpaa2_eth_priv *priv, /* Allocate buffer visible to WRIOP + skb shared info + * alignment padding */ - buf = napi_alloc_frag(dpaa2_eth_buf_raw_size(priv)); - if (unlikely(!buf)) + /* allocate one page for each Rx buffer. WRIOP sees + * the entire page except for a tailroom reserved for + * skb shared info + */ + page = dev_alloc_pages(0); + if (!page) goto err_alloc; - buf = PTR_ALIGN(buf, priv->rx_buf_align); - - addr = dma_map_single(dev, buf, DPAA2_ETH_RX_BUF_SIZE, - DMA_BIDIRECTIONAL); + addr = dma_map_page(dev, page, 0, DPAA2_ETH_RX_BUF_SIZE, + DMA_BIDIRECTIONAL); if (unlikely(dma_mapping_error(dev, addr))) goto err_map; @@ -926,7 +928,7 @@ static int add_bufs(struct dpaa2_eth_priv *priv, /* tracing point */ trace_dpaa2_eth_buf_seed(priv->net_dev, - buf, dpaa2_eth_buf_raw_size(priv), + page, DPAA2_ETH_RX_BUF_RAW_SIZE, addr, DPAA2_ETH_RX_BUF_SIZE, bpid); } @@ -948,7 +950,7 @@ static int add_bufs(struct dpaa2_eth_priv *priv, return i; err_map: - skb_free_frag(buf); + __free_pages(page, 0); err_alloc: /* If we managed to allocate at least some buffers, * release them to hardware @@ -2134,6 +2136,7 @@ static int set_buffer_layout(struct dpaa2_eth_priv *priv) { struct device *dev = priv->net_dev->dev.parent; struct dpni_buffer_layout buf_layout = {0}; + u16 rx_buf_align; int err; /* We need to check for WRIOP version 1.0.0, but depending on the MC @@ -2142,9 +2145,9 @@ static int set_buffer_layout(struct dpaa2_eth_priv *priv) */ if (priv->dpni_attrs.wriop_version == DPAA2_WRIOP_VERSION(0, 0, 0) || priv->dpni_attrs.wriop_version == DPAA2_WRIOP_VERSION(1, 0, 0)) - priv->rx_buf_align = DPAA2_ETH_RX_BUF_ALIGN_REV1; + rx_buf_align = DPAA2_ETH_RX_BUF_ALIGN_REV1; else - priv->rx_buf_align = DPAA2_ETH_RX_BUF_ALIGN; + rx_buf_align = DPAA2_ETH_RX_BUF_ALIGN; /* tx buffer */ buf_layout.private_data_size = DPAA2_ETH_SWA_SIZE; @@ -2184,7 +2187,7 @@ static int set_buffer_layout(struct dpaa2_eth_priv *priv) /* rx buffer */ buf_layout.pass_frame_status = true; buf_layout.pass_parser_result = true; - buf_layout.data_align = priv->rx_buf_align; + buf_layout.data_align = rx_buf_align; buf_layout.data_head_room = dpaa2_eth_rx_head_room(priv); buf_layout.private_data_size = 0; buf_layout.options = DPNI_BUF_LAYOUT_OPT_PARSER_RESULT | diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h index 31fe486..da3d039 100644 --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h @@ -63,9 +63,11 @@ /* Hardware requires alignment for ingress/egress buffer addresses */ #define DPAA2_ETH_TX_BUF_ALIGN 64 -#define DPAA2_ETH_RX_BUF_SIZE 2048 -#define DPAA2_ETH_SKB_SIZE \ - (DPAA2_ETH_RX_BUF_SIZE + SKB_DATA_ALIGN(sizeof(struct skb_shared_info))) +#define DPAA2_ETH_RX_BUF_RAW_SIZE PAGE_SIZE +#define DPAA2_ETH_RX_BUF_TAILROOM \ + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) +#define DPAA2_ETH_RX_BUF_SIZE \ + (DPAA2_ETH_RX_BUF_RAW_SIZE - DPAA2_ETH_RX_BUF_TAILROOM) /* Hardware annotation area in RX/TX buffers */ #define DPAA2_ETH_RX_HWA_SIZE 64 @@ -343,7 +345,6 @@ struct dpaa2_eth_priv { bool rx_tstamp; /* Rx timestamping enabled */ u16 tx_qdid; - u16 rx_buf_align; struct fsl_mc_io *mc_io; /* Cores which have an affine DPIO/DPCON. * This is the cpu set on which Rx and Tx conf frames are processed @@ -418,15 +419,6 @@ enum dpaa2_eth_rx_dist { DPAA2_ETH_RX_DIST_CLS }; -/* Hardware only sees DPAA2_ETH_RX_BUF_SIZE, but the skb built around - * the buffer also needs space for its shared info struct, and we need - * to allocate enough to accommodate hardware alignment restrictions - */ -static inline unsigned int dpaa2_eth_buf_raw_size(struct dpaa2_eth_priv *priv) -{ - return DPAA2_ETH_SKB_SIZE + priv->rx_buf_align; -} - static inline unsigned int dpaa2_eth_needed_headroom(struct dpaa2_eth_priv *priv, struct sk_buff *skb) @@ -451,8 +443,7 @@ unsigned int dpaa2_eth_needed_headroom(struct dpaa2_eth_priv *priv, */ static inline unsigned int dpaa2_eth_rx_head_room(struct dpaa2_eth_priv *priv) { - return priv->tx_data_offset + DPAA2_ETH_TX_BUF_ALIGN - - DPAA2_ETH_RX_HWA_SIZE; + return priv->tx_data_offset - DPAA2_ETH_RX_HWA_SIZE; } int dpaa2_eth_set_hash(struct net_device *net_dev, u64 flags);