diff mbox series

[bpf-next,06/13] xsk: optimize for aligned case

Message ID 20210922075613.12186-7-magnus.karlsson@gmail.com (mailing list archive)
State Accepted
Commit 94033cd8e73b8632bab7c8b7bb54caa4f5616db7
Delegated to: BPF
Headers show
Series xsk: i40e: ice: introduce batching for Rx buffer allocation | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for bpf-next
netdev/subject_prefix success Link
netdev/cc_maintainers warning 9 maintainers not CCed: davem@davemloft.net kpsingh@kernel.org hawk@kernel.org john.fastabend@gmail.com yhs@fb.com kuba@kernel.org andrii@kernel.org songliubraving@fb.com kafai@fb.com
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 2 this patch: 2
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch warning WARNING: line length of 84 exceeds 80 columns WARNING: line length of 86 exceeds 80 columns WARNING: line length of 90 exceeds 80 columns WARNING: line length of 91 exceeds 80 columns
netdev/build_allmodconfig_warn success Errors and warnings before: 2 this patch: 2
netdev/header_inline success Link
bpf/vmtest-bpf-next success VM_Test
bpf/vmtest-bpf-next-PR success PR summary

Commit Message

Magnus Karlsson Sept. 22, 2021, 7:56 a.m. UTC 
From: Magnus Karlsson <magnus.karlsson@intel.com>

Optimize for the aligned case by precomputing the parameter values of
the xdp_buff_xsk and xdp_buff structures in the heads array. We can do
this as the heads array size is equal to the number of chunks in the
umem for the aligned case. Then every entry in this array will reflect
a certain chunk/frame and can therefore be prepopulated with the
correct values and we can drop the use of the free_heads stack. Note
that it is not possible to allocate more buffers than what has been
allocated in the aligned case since each chunk can only contain a
single buffer.

We can unfortunately not do this in the unaligned case as one chunk
might contain multiple buffers. In this case, we keep the old scheme
of populating a heads entry every time it is used and using
the free_heads stack.

Also move xp_release() and xp_get_handle() to xsk_buff_pool.h. They
were for some reason in xsk.c even though they are buffer pool
operations.

Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
---
 include/net/xsk_buff_pool.h | 46 +++++++++++++++++++++++++++++-
 net/xdp/xsk.c               | 15 ----------
 net/xdp/xsk_buff_pool.c     | 56 ++++++++++++++++++++++---------------
 3 files changed, 79 insertions(+), 38 deletions(-)

Comments

Nathan Chancellor Sept. 28, 2021, 11:15 p.m. UTC | #1 
On Wed, Sep 22, 2021 at 09:56:06AM +0200, Magnus Karlsson wrote:
> From: Magnus Karlsson <magnus.karlsson@intel.com>
> 
> Optimize for the aligned case by precomputing the parameter values of
> the xdp_buff_xsk and xdp_buff structures in the heads array. We can do
> this as the heads array size is equal to the number of chunks in the
> umem for the aligned case. Then every entry in this array will reflect
> a certain chunk/frame and can therefore be prepopulated with the
> correct values and we can drop the use of the free_heads stack. Note
> that it is not possible to allocate more buffers than what has been
> allocated in the aligned case since each chunk can only contain a
> single buffer.
> 
> We can unfortunately not do this in the unaligned case as one chunk
> might contain multiple buffers. In this case, we keep the old scheme
> of populating a heads entry every time it is used and using
> the free_heads stack.
> 
> Also move xp_release() and xp_get_handle() to xsk_buff_pool.h. They
> were for some reason in xsk.c even though they are buffer pool
> operations.
> 
> Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>

My apologies if this has already been reported (I have not seen a report
on netdev nor a report from Intel around it) but this patch as
commit 94033cd8e73b ("xsk: Optimize for aligned case") in -next causes
the following build failure with clang + x86_64 allmodconfig:

net/xdp/xsk_buff_pool.c:465:15: error: variable 'xskb' is uninitialized when used here [-Werror,-Wuninitialized]
                        xp_release(xskb);
                                   ^~~~
net/xdp/xsk_buff_pool.c:455:27: note: initialize the variable 'xskb' to silence this warning
        struct xdp_buff_xsk *xskb;
                                 ^
                                  = NULL
1 error generated.

Cheers,
Nathan
Magnus Karlsson Sept. 29, 2021, 5:52 a.m. UTC | #2 
On Wed, Sep 29, 2021 at 1:15 AM Nathan Chancellor <nathan@kernel.org> wrote:
>
> On Wed, Sep 22, 2021 at 09:56:06AM +0200, Magnus Karlsson wrote:
> > From: Magnus Karlsson <magnus.karlsson@intel.com>
> >
> > Optimize for the aligned case by precomputing the parameter values of
> > the xdp_buff_xsk and xdp_buff structures in the heads array. We can do
> > this as the heads array size is equal to the number of chunks in the
> > umem for the aligned case. Then every entry in this array will reflect
> > a certain chunk/frame and can therefore be prepopulated with the
> > correct values and we can drop the use of the free_heads stack. Note
> > that it is not possible to allocate more buffers than what has been
> > allocated in the aligned case since each chunk can only contain a
> > single buffer.
> >
> > We can unfortunately not do this in the unaligned case as one chunk
> > might contain multiple buffers. In this case, we keep the old scheme
> > of populating a heads entry every time it is used and using
> > the free_heads stack.
> >
> > Also move xp_release() and xp_get_handle() to xsk_buff_pool.h. They
> > were for some reason in xsk.c even though they are buffer pool
> > operations.
> >
> > Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
>
> My apologies if this has already been reported (I have not seen a report
> on netdev nor a report from Intel around it) but this patch as
> commit 94033cd8e73b ("xsk: Optimize for aligned case") in -next causes
> the following build failure with clang + x86_64 allmodconfig:
>
> net/xdp/xsk_buff_pool.c:465:15: error: variable 'xskb' is uninitialized when used here [-Werror,-Wuninitialized]
>                         xp_release(xskb);
>                                    ^~~~
> net/xdp/xsk_buff_pool.c:455:27: note: initialize the variable 'xskb' to silence this warning
>         struct xdp_buff_xsk *xskb;
>                                  ^
>                                   = NULL
> 1 error generated.

Thanks for reporting this Nathan. Will fix right away.

/Magnus

> Cheers,
> Nathan
kernel test robot Sept. 29, 2021, 3:33 p.m. UTC | #3 
Hi Magnus,

I love your patch! Yet something to improve:

[auto build test ERROR on 17b52c226a9a170f1611f69d12a71be05748aefd]

url:    https://github.com/0day-ci/linux/commits/Magnus-Karlsson/xsk-i40e-ice-introduce-batching-for-Rx-buffer-allocation/20210929-210813
base:   17b52c226a9a170f1611f69d12a71be05748aefd
config: riscv-buildonly-randconfig-r003-20210929 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project dc6e8dfdfe7efecfda318d43a06fae18b40eb498)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install riscv cross compiling tool for clang build
        # apt-get install binutils-riscv64-linux-gnu
        # https://github.com/0day-ci/linux/commit/5a3442cd30198f6a7fb37ec0b8cad12bea1d5178
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Magnus-Karlsson/xsk-i40e-ice-introduce-batching-for-Rx-buffer-allocation/20210929-210813
        git checkout 5a3442cd30198f6a7fb37ec0b8cad12bea1d5178
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=riscv 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> net/xdp/xsk_buff_pool.c:465:15: error: variable 'xskb' is uninitialized when used here [-Werror,-Wuninitialized]
                           xp_release(xskb);
                                      ^~~~
   net/xdp/xsk_buff_pool.c:455:27: note: initialize the variable 'xskb' to silence this warning
           struct xdp_buff_xsk *xskb;
                                    ^
                                     = NULL
   1 error generated.


vim +/xskb +465 net/xdp/xsk_buff_pool.c

2b43470add8c8f Björn Töpel     2020-05-20  452  
2b43470add8c8f Björn Töpel     2020-05-20  453  static struct xdp_buff_xsk *__xp_alloc(struct xsk_buff_pool *pool)
2b43470add8c8f Björn Töpel     2020-05-20  454  {
2b43470add8c8f Björn Töpel     2020-05-20  455  	struct xdp_buff_xsk *xskb;
2b43470add8c8f Björn Töpel     2020-05-20  456  	u64 addr;
2b43470add8c8f Björn Töpel     2020-05-20  457  	bool ok;
2b43470add8c8f Björn Töpel     2020-05-20  458  
2b43470add8c8f Björn Töpel     2020-05-20  459  	if (pool->free_heads_cnt == 0)
2b43470add8c8f Björn Töpel     2020-05-20  460  		return NULL;
2b43470add8c8f Björn Töpel     2020-05-20  461  
2b43470add8c8f Björn Töpel     2020-05-20  462  	for (;;) {
2b43470add8c8f Björn Töpel     2020-05-20  463  		if (!xskq_cons_peek_addr_unchecked(pool->fq, &addr)) {
8aa5a33578e968 Ciara Loftus    2020-07-08  464  			pool->fq->queue_empty_descs++;
2b43470add8c8f Björn Töpel     2020-05-20 @465  			xp_release(xskb);
2b43470add8c8f Björn Töpel     2020-05-20  466  			return NULL;
2b43470add8c8f Björn Töpel     2020-05-20  467  		}
2b43470add8c8f Björn Töpel     2020-05-20  468  
2b43470add8c8f Björn Töpel     2020-05-20  469  		ok = pool->unaligned ? xp_check_unaligned(pool, &addr) :
2b43470add8c8f Björn Töpel     2020-05-20  470  		     xp_check_aligned(pool, &addr);
2b43470add8c8f Björn Töpel     2020-05-20  471  		if (!ok) {
2b43470add8c8f Björn Töpel     2020-05-20  472  			pool->fq->invalid_descs++;
2b43470add8c8f Björn Töpel     2020-05-20  473  			xskq_cons_release(pool->fq);
2b43470add8c8f Björn Töpel     2020-05-20  474  			continue;
2b43470add8c8f Björn Töpel     2020-05-20  475  		}
2b43470add8c8f Björn Töpel     2020-05-20  476  		break;
2b43470add8c8f Björn Töpel     2020-05-20  477  	}
2b43470add8c8f Björn Töpel     2020-05-20  478  
5a3442cd30198f Magnus Karlsson 2021-09-22  479  	if (pool->unaligned) {
5a3442cd30198f Magnus Karlsson 2021-09-22  480  		xskb = pool->free_heads[--pool->free_heads_cnt];
5a3442cd30198f Magnus Karlsson 2021-09-22  481  		xp_init_xskb_addr(xskb, pool, addr);
5a3442cd30198f Magnus Karlsson 2021-09-22  482  		if (pool->dma_pages_cnt)
5a3442cd30198f Magnus Karlsson 2021-09-22  483  			xp_init_xskb_dma(xskb, pool, pool->dma_pages, addr);
5a3442cd30198f Magnus Karlsson 2021-09-22  484  	} else {
5a3442cd30198f Magnus Karlsson 2021-09-22  485  		xskb = &pool->heads[xp_aligned_extract_idx(pool, addr)];
2b43470add8c8f Björn Töpel     2020-05-20  486  	}
5a3442cd30198f Magnus Karlsson 2021-09-22  487  
5a3442cd30198f Magnus Karlsson 2021-09-22  488  	xskq_cons_release(pool->fq);
2b43470add8c8f Björn Töpel     2020-05-20  489  	return xskb;
2b43470add8c8f Björn Töpel     2020-05-20  490  }
2b43470add8c8f Björn Töpel     2020-05-20  491  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff mbox series

Patch

diff --git a/include/net/xsk_buff_pool.h b/include/net/xsk_buff_pool.h
index b7068f97639f..ddeefc4a1040 100644
--- a/include/net/xsk_buff_pool.h
+++ b/include/net/xsk_buff_pool.h
@@ -7,6 +7,7 @@ 
 #include <linux/if_xdp.h>
 #include <linux/types.h>
 #include <linux/dma-mapping.h>
+#include <linux/bpf.h>
 #include <net/xdp.h>
 
 struct xsk_buff_pool;
@@ -66,6 +67,7 @@  struct xsk_buff_pool {
 	u32 free_heads_cnt;
 	u32 headroom;
 	u32 chunk_size;
+	u32 chunk_shift;
 	u32 frame_len;
 	u8 cached_need_wakeup;
 	bool uses_need_wakeup;
@@ -80,6 +82,13 @@  struct xsk_buff_pool {
 	struct xdp_buff_xsk *free_heads[];
 };
 
+/* Masks for xdp_umem_page flags.
+ * The low 12-bits of the addr will be 0 since this is the page address, so we
+ * can use them for flags.
+ */
+#define XSK_NEXT_PG_CONTIG_SHIFT 0
+#define XSK_NEXT_PG_CONTIG_MASK BIT_ULL(XSK_NEXT_PG_CONTIG_SHIFT)
+
 /* AF_XDP core. */
 struct xsk_buff_pool *xp_create_and_assign_umem(struct xdp_sock *xs,
 						struct xdp_umem *umem);
@@ -88,7 +97,6 @@  int xp_assign_dev(struct xsk_buff_pool *pool, struct net_device *dev,
 int xp_assign_dev_shared(struct xsk_buff_pool *pool, struct xdp_umem *umem,
 			 struct net_device *dev, u16 queue_id);
 void xp_destroy(struct xsk_buff_pool *pool);
-void xp_release(struct xdp_buff_xsk *xskb);
 void xp_get_pool(struct xsk_buff_pool *pool);
 bool xp_put_pool(struct xsk_buff_pool *pool);
 void xp_clear_dev(struct xsk_buff_pool *pool);
@@ -98,6 +106,21 @@  void xp_del_xsk(struct xsk_buff_pool *pool, struct xdp_sock *xs);
 /* AF_XDP, and XDP core. */
 void xp_free(struct xdp_buff_xsk *xskb);
 
+static inline void xp_init_xskb_addr(struct xdp_buff_xsk *xskb, struct xsk_buff_pool *pool,
+				     u64 addr)
+{
+	xskb->orig_addr = addr;
+	xskb->xdp.data_hard_start = pool->addrs + addr + pool->headroom;
+}
+
+static inline void xp_init_xskb_dma(struct xdp_buff_xsk *xskb, struct xsk_buff_pool *pool,
+				    dma_addr_t *dma_pages, u64 addr)
+{
+	xskb->frame_dma = (dma_pages[addr >> PAGE_SHIFT] & ~XSK_NEXT_PG_CONTIG_MASK) +
+		(addr & ~PAGE_MASK);
+	xskb->dma = xskb->frame_dma + pool->headroom + XDP_PACKET_HEADROOM;
+}
+
 /* AF_XDP ZC drivers, via xdp_sock_buff.h */
 void xp_set_rxq_info(struct xsk_buff_pool *pool, struct xdp_rxq_info *rxq);
 int xp_dma_map(struct xsk_buff_pool *pool, struct device *dev,
@@ -180,4 +203,25 @@  static inline u64 xp_unaligned_add_offset_to_addr(u64 addr)
 		xp_unaligned_extract_offset(addr);
 }
 
+static inline u32 xp_aligned_extract_idx(struct xsk_buff_pool *pool, u64 addr)
+{
+	return xp_aligned_extract_addr(pool, addr) >> pool->chunk_shift;
+}
+
+static inline void xp_release(struct xdp_buff_xsk *xskb)
+{
+	if (xskb->pool->unaligned)
+		xskb->pool->free_heads[xskb->pool->free_heads_cnt++] = xskb;
+}
+
+static inline u64 xp_get_handle(struct xdp_buff_xsk *xskb)
+{
+	u64 offset = xskb->xdp.data - xskb->xdp.data_hard_start;
+
+	offset += xskb->pool->headroom;
+	if (!xskb->pool->unaligned)
+		return xskb->orig_addr + offset;
+	return xskb->orig_addr + (offset << XSK_UNALIGNED_BUF_OFFSET_SHIFT);
+}
+
 #endif /* XSK_BUFF_POOL_H_ */
diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index d6b500dc4208..f16074eb53c7 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -134,21 +134,6 @@  int xsk_reg_pool_at_qid(struct net_device *dev, struct xsk_buff_pool *pool,
 	return 0;
 }
 
-void xp_release(struct xdp_buff_xsk *xskb)
-{
-	xskb->pool->free_heads[xskb->pool->free_heads_cnt++] = xskb;
-}
-
-static u64 xp_get_handle(struct xdp_buff_xsk *xskb)
-{
-	u64 offset = xskb->xdp.data - xskb->xdp.data_hard_start;
-
-	offset += xskb->pool->headroom;
-	if (!xskb->pool->unaligned)
-		return xskb->orig_addr + offset;
-	return xskb->orig_addr + (offset << XSK_UNALIGNED_BUF_OFFSET_SHIFT);
-}
-
 static int __xsk_rcv_zc(struct xdp_sock *xs, struct xdp_buff *xdp, u32 len)
 {
 	struct xdp_buff_xsk *xskb = container_of(xdp, struct xdp_buff_xsk, xdp);
diff --git a/net/xdp/xsk_buff_pool.c b/net/xdp/xsk_buff_pool.c
index 884d95d70f5e..96b14e51ba7e 100644
--- a/net/xdp/xsk_buff_pool.c
+++ b/net/xdp/xsk_buff_pool.c
@@ -44,12 +44,13 @@  void xp_destroy(struct xsk_buff_pool *pool)
 struct xsk_buff_pool *xp_create_and_assign_umem(struct xdp_sock *xs,
 						struct xdp_umem *umem)
 {
+	bool unaligned = umem->flags & XDP_UMEM_UNALIGNED_CHUNK_FLAG;
 	struct xsk_buff_pool *pool;
 	struct xdp_buff_xsk *xskb;
-	u32 i;
+	u32 i, entries;
 
-	pool = kvzalloc(struct_size(pool, free_heads, umem->chunks),
-			GFP_KERNEL);
+	entries = unaligned ? umem->chunks : 0;
+	pool = kvzalloc(struct_size(pool, free_heads, entries),	GFP_KERNEL);
 	if (!pool)
 		goto out;
 
@@ -63,7 +64,8 @@  struct xsk_buff_pool *xp_create_and_assign_umem(struct xdp_sock *xs,
 	pool->free_heads_cnt = umem->chunks;
 	pool->headroom = umem->headroom;
 	pool->chunk_size = umem->chunk_size;
-	pool->unaligned = umem->flags & XDP_UMEM_UNALIGNED_CHUNK_FLAG;
+	pool->chunk_shift = ffs(umem->chunk_size) - 1;
+	pool->unaligned = unaligned;
 	pool->frame_len = umem->chunk_size - umem->headroom -
 		XDP_PACKET_HEADROOM;
 	pool->umem = umem;
@@ -81,7 +83,10 @@  struct xsk_buff_pool *xp_create_and_assign_umem(struct xdp_sock *xs,
 		xskb = &pool->heads[i];
 		xskb->pool = pool;
 		xskb->xdp.frame_sz = umem->chunk_size - umem->headroom;
-		pool->free_heads[i] = xskb;
+		if (pool->unaligned)
+			pool->free_heads[i] = xskb;
+		else
+			xp_init_xskb_addr(xskb, pool, i * pool->chunk_size);
 	}
 
 	return pool;
@@ -406,6 +411,12 @@  int xp_dma_map(struct xsk_buff_pool *pool, struct device *dev,
 
 	if (pool->unaligned)
 		xp_check_dma_contiguity(dma_map);
+	else
+		for (i = 0; i < pool->heads_cnt; i++) {
+			struct xdp_buff_xsk *xskb = &pool->heads[i];
+
+			xp_init_xskb_dma(xskb, pool, dma_map->dma_pages, xskb->orig_addr);
+		}
 
 	err = xp_init_dma_info(pool, dma_map);
 	if (err) {
@@ -448,8 +459,6 @@  static struct xdp_buff_xsk *__xp_alloc(struct xsk_buff_pool *pool)
 	if (pool->free_heads_cnt == 0)
 		return NULL;
 
-	xskb = pool->free_heads[--pool->free_heads_cnt];
-
 	for (;;) {
 		if (!xskq_cons_peek_addr_unchecked(pool->fq, &addr)) {
 			pool->fq->queue_empty_descs++;
@@ -466,17 +475,17 @@  static struct xdp_buff_xsk *__xp_alloc(struct xsk_buff_pool *pool)
 		}
 		break;
 	}
-	xskq_cons_release(pool->fq);
 
-	xskb->orig_addr = addr;
-	xskb->xdp.data_hard_start = pool->addrs + addr + pool->headroom;
-	if (pool->dma_pages_cnt) {
-		xskb->frame_dma = (pool->dma_pages[addr >> PAGE_SHIFT] &
-				   ~XSK_NEXT_PG_CONTIG_MASK) +
-				  (addr & ~PAGE_MASK);
-		xskb->dma = xskb->frame_dma + pool->headroom +
-			    XDP_PACKET_HEADROOM;
+	if (pool->unaligned) {
+		xskb = pool->free_heads[--pool->free_heads_cnt];
+		xp_init_xskb_addr(xskb, pool, addr);
+		if (pool->dma_pages_cnt)
+			xp_init_xskb_dma(xskb, pool, pool->dma_pages, addr);
+	} else {
+		xskb = &pool->heads[xp_aligned_extract_idx(pool, addr)];
 	}
+
+	xskq_cons_release(pool->fq);
 	return xskb;
 }
 
@@ -533,13 +542,16 @@  static u32 xp_alloc_new_from_fq(struct xsk_buff_pool *pool, struct xdp_buff **xd
 			continue;
 		}
 
-		xskb = pool->free_heads[--pool->free_heads_cnt];
+		if (pool->unaligned) {
+			xskb = pool->free_heads[--pool->free_heads_cnt];
+			xp_init_xskb_addr(xskb, pool, addr);
+			if (pool->dma_pages_cnt)
+				xp_init_xskb_dma(xskb, pool, pool->dma_pages, addr);
+		} else {
+			xskb = &pool->heads[xp_aligned_extract_idx(pool, addr)];
+		}
+
 		*xdp = &xskb->xdp;
-		xskb->orig_addr = addr;
-		xskb->xdp.data_hard_start = pool->addrs + addr + pool->headroom;
-		xskb->frame_dma = (pool->dma_pages[addr >> PAGE_SHIFT] &
-				   ~XSK_NEXT_PG_CONTIG_MASK) + (addr & ~PAGE_MASK);
-		xskb->dma = xskb->frame_dma + pool->headroom + XDP_PACKET_HEADROOM;
 		xdp++;
 	}