[PATCH bpf-next 0/3] xsk: misc code cleanup

[PATCH bpf-next 0/3] xsk: misc code cleanup

[Magnus Karlsson]

This patch set cleans up two code style issues with the xsk zero-copy
code. The resulting code is smaller and simpler.

Patch 1: Removes a potential compiler warning reported by the Intel
         0-DAY kernel test infrastructure.
Patches 2-3: Removes the xdp_umem_props structure. At some point, it
             was used to break a dependency, but the members are these
             days much better off in the xdp_umem since the dependency
             does not exist anymore.

I based this patch set on bpf-next commit 234dbe3dc1db ("Merge branch
'verifier-liveness-simplification'")

Thanks: Magnus

Magnus Karlsson (3):
  i40e: fix possible compiler warning in xsk TX path
  xsk: get rid of useless struct xdp_umem_props
  i40e: adapt driver to new xdp_umem structure

 drivers/net/ethernet/intel/i40e/i40e_xsk.c | 10 ++++------
 include/net/xdp_sock.h                     |  8 ++------
 net/xdp/xdp_umem.c                         |  4 ++--
 net/xdp/xdp_umem_props.h                   | 14 --------------
 net/xdp/xsk.c                              | 10 ++++++----
 net/xdp/xsk_queue.c                        |  5 +++--
 net/xdp/xsk_queue.h                        | 13 +++++++------
 7 files changed, 24 insertions(+), 40 deletions(-)
 delete mode 100644 net/xdp/xdp_umem_props.h

[PATCH bpf-next 1/3] i40e: fix possible compiler warning in xsk TX path

[Magnus Karlsson]

With certain gcc versions, it was possible to get the warning
"'tx_desc' may be used uninitialized in this function" for the
i40e_xmit_zc. This was not possible, however this commit simplifies
the code path so that this warning is no longer emitted.

Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_xsk.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
index 94947a8..41ca7e1 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
@@ -668,9 +668,8 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
  **/
 static bool i40e_xmit_zc(struct i40e_ring *xdp_ring, unsigned int budget)
 {
-	unsigned int total_packets = 0;
+	struct i40e_tx_desc *tx_desc = NULL;
 	struct i40e_tx_buffer *tx_bi;
-	struct i40e_tx_desc *tx_desc;
 	bool work_done = true;
 	dma_addr_t dma;
 	u32 len;
@@ -697,14 +696,13 @@ static bool i40e_xmit_zc(struct i40e_ring *xdp_ring, unsigned int budget)
 			build_ctob(I40E_TX_DESC_CMD_ICRC
 				   | I40E_TX_DESC_CMD_EOP,
 				   0, len, 0);
-		total_packets++;
 
 		xdp_ring->next_to_use++;
 		if (xdp_ring->next_to_use == xdp_ring->count)
 			xdp_ring->next_to_use = 0;
 	}
 
-	if (total_packets > 0) {
+	if (tx_desc) {
 		/* Request an interrupt for the last frame and bump tail ptr. */
 		tx_desc->cmd_type_offset_bsz |= (I40E_TX_DESC_CMD_RS <<
 						 I40E_TXD_QW1_CMD_SHIFT);
-- 
2.7.4

[PATCH bpf-next 2/3] xsk: get rid of useless struct xdp_umem_props

[Magnus Karlsson]

This commit gets rid of the structure xdp_umem_props. It was there to
be able to break a dependency at one point, but this is no longer
needed. The values in the struct are instead stored directly in the
xdp_umem structure. This simplifies the xsk code as well as af_xdp
zero-copy drivers and as a bonus gets rid of one internal header file.

Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
---
 include/net/xdp_sock.h   |  8 ++------
 net/xdp/xdp_umem.c       |  4 ++--
 net/xdp/xdp_umem_props.h | 14 --------------
 net/xdp/xsk.c            | 10 ++++++----
 net/xdp/xsk_queue.c      |  5 +++--
 net/xdp/xsk_queue.h      | 13 +++++++------
 6 files changed, 20 insertions(+), 34 deletions(-)
 delete mode 100644 net/xdp/xdp_umem_props.h

diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h
index 56994ad..932ca0d 100644
--- a/include/net/xdp_sock.h
+++ b/include/net/xdp_sock.h
@@ -16,11 +16,6 @@
 struct net_device;
 struct xsk_queue;
 
-struct xdp_umem_props {
-	u64 chunk_mask;
-	u64 size;
-};
-
 struct xdp_umem_page {
 	void *addr;
 	dma_addr_t dma;
@@ -30,7 +25,8 @@ struct xdp_umem {
 	struct xsk_queue *fq;
 	struct xsk_queue *cq;
 	struct xdp_umem_page *pages;
-	struct xdp_umem_props props;
+	u64 chunk_mask;
+	u64 size;
 	u32 headroom;
 	u32 chunk_size_nohr;
 	struct user_struct *user;
diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c
index bfe2dbe..2471614 100644
--- a/net/xdp/xdp_umem.c
+++ b/net/xdp/xdp_umem.c
@@ -314,8 +314,8 @@ static int xdp_umem_reg(struct xdp_umem *umem, struct xdp_umem_reg *mr)
 
 	umem->pid = get_task_pid(current, PIDTYPE_PID);
 	umem->address = (unsigned long)addr;
-	umem->props.chunk_mask = ~((u64)chunk_size - 1);
-	umem->props.size = size;
+	umem->chunk_mask = ~((u64)chunk_size - 1);
+	umem->size = size;
 	umem->headroom = headroom;
 	umem->chunk_size_nohr = chunk_size - headroom;
 	umem->npgs = size / PAGE_SIZE;
diff --git a/net/xdp/xdp_umem_props.h b/net/xdp/xdp_umem_props.h
deleted file mode 100644
index 40eab10..0000000
--- a/net/xdp/xdp_umem_props.h
+++ /dev/null
@@ -1,14 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-/* XDP user-space packet buffer
- * Copyright(c) 2018 Intel Corporation.
- */
-
-#ifndef XDP_UMEM_PROPS_H_
-#define XDP_UMEM_PROPS_H_
-
-struct xdp_umem_props {
-	u64 chunk_mask;
-	u64 size;
-};
-
-#endif /* XDP_UMEM_PROPS_H_ */
diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index 4e937cd..acbe5b5 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -458,8 +458,10 @@ static int xsk_bind(struct socket *sock, struct sockaddr *addr, int addr_len)
 		goto out_unlock;
 	} else {
 		/* This xsk has its own umem. */
-		xskq_set_umem(xs->umem->fq, &xs->umem->props);
-		xskq_set_umem(xs->umem->cq, &xs->umem->props);
+		xskq_set_umem(xs->umem->fq, xs->umem->size,
+			      xs->umem->chunk_mask);
+		xskq_set_umem(xs->umem->cq, xs->umem->size,
+			      xs->umem->chunk_mask);
 
 		err = xdp_umem_assign_dev(xs->umem, dev, qid, flags);
 		if (err)
@@ -469,8 +471,8 @@ static int xsk_bind(struct socket *sock, struct sockaddr *addr, int addr_len)
 	xs->dev = dev;
 	xs->zc = xs->umem->zc;
 	xs->queue_id = qid;
-	xskq_set_umem(xs->rx, &xs->umem->props);
-	xskq_set_umem(xs->tx, &xs->umem->props);
+	xskq_set_umem(xs->rx, xs->umem->size, xs->umem->chunk_mask);
+	xskq_set_umem(xs->tx, xs->umem->size, xs->umem->chunk_mask);
 	xdp_add_sk_umem(xs->umem, xs);
 
 out_unlock:
diff --git a/net/xdp/xsk_queue.c b/net/xdp/xsk_queue.c
index 6c32e92..2dc1384d 100644
--- a/net/xdp/xsk_queue.c
+++ b/net/xdp/xsk_queue.c
@@ -7,12 +7,13 @@
 
 #include "xsk_queue.h"
 
-void xskq_set_umem(struct xsk_queue *q, struct xdp_umem_props *umem_props)
+void xskq_set_umem(struct xsk_queue *q, u64 size, u64 chunk_mask)
 {
 	if (!q)
 		return;
 
-	q->umem_props = *umem_props;
+	q->size = size;
+	q->chunk_mask = chunk_mask;
 }
 
 static u32 xskq_umem_get_ring_size(struct xsk_queue *q)
diff --git a/net/xdp/xsk_queue.h b/net/xdp/xsk_queue.h
index 8a64b15..82252cc 100644
--- a/net/xdp/xsk_queue.h
+++ b/net/xdp/xsk_queue.h
@@ -31,7 +31,8 @@ struct xdp_umem_ring {
 };
 
 struct xsk_queue {
-	struct xdp_umem_props umem_props;
+	u64 chunk_mask;
+	u64 size;
 	u32 ring_mask;
 	u32 nentries;
 	u32 prod_head;
@@ -78,7 +79,7 @@ static inline u32 xskq_nb_free(struct xsk_queue *q, u32 producer, u32 dcnt)
 
 static inline bool xskq_is_valid_addr(struct xsk_queue *q, u64 addr)
 {
-	if (addr >= q->umem_props.size) {
+	if (addr >= q->size) {
 		q->invalid_descs++;
 		return false;
 	}
@@ -92,7 +93,7 @@ static inline u64 *xskq_validate_addr(struct xsk_queue *q, u64 *addr)
 		struct xdp_umem_ring *ring = (struct xdp_umem_ring *)q->ring;
 		unsigned int idx = q->cons_tail & q->ring_mask;
 
-		*addr = READ_ONCE(ring->desc[idx]) & q->umem_props.chunk_mask;
+		*addr = READ_ONCE(ring->desc[idx]) & q->chunk_mask;
 		if (xskq_is_valid_addr(q, *addr))
 			return addr;
 
@@ -173,8 +174,8 @@ static inline bool xskq_is_valid_desc(struct xsk_queue *q, struct xdp_desc *d)
 	if (!xskq_is_valid_addr(q, d->addr))
 		return false;
 
-	if (((d->addr + d->len) & q->umem_props.chunk_mask) !=
-	    (d->addr & q->umem_props.chunk_mask)) {
+	if (((d->addr + d->len) & q->chunk_mask) !=
+	    (d->addr & q->chunk_mask)) {
 		q->invalid_descs++;
 		return false;
 	}
@@ -253,7 +254,7 @@ static inline bool xskq_empty_desc(struct xsk_queue *q)
 	return xskq_nb_free(q, q->prod_tail, q->nentries) == q->nentries;
 }
 
-void xskq_set_umem(struct xsk_queue *q, struct xdp_umem_props *umem_props);
+void xskq_set_umem(struct xsk_queue *q, u64 size, u64 chunk_mask);
 struct xsk_queue *xskq_create(u32 nentries, bool umem_queue);
 void xskq_destroy(struct xsk_queue *q_ops);
 
-- 
2.7.4

[PATCH bpf-next 3/3] i40e: adapt driver to new xdp_umem structure

[Magnus Karlsson]

The struct xdp_umem_props was removed in the xsk code and this commit
adapts the i40e af_xdp zero-copy driver code to the new xdp_umem
structure.

Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_xsk.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
index 41ca7e1..2ebfc78 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
@@ -442,7 +442,7 @@ static void i40e_reuse_rx_buffer_zc(struct i40e_ring *rx_ring,
 				    struct i40e_rx_buffer *old_bi)
 {
 	struct i40e_rx_buffer *new_bi = &rx_ring->rx_bi[rx_ring->next_to_alloc];
-	unsigned long mask = (unsigned long)rx_ring->xsk_umem->props.chunk_mask;
+	unsigned long mask = (unsigned long)rx_ring->xsk_umem->chunk_mask;
 	u64 hr = rx_ring->xsk_umem->headroom + XDP_PACKET_HEADROOM;
 	u16 nta = rx_ring->next_to_alloc;
 
@@ -477,7 +477,7 @@ void i40e_zca_free(struct zero_copy_allocator *alloc, unsigned long handle)
 
 	rx_ring = container_of(alloc, struct i40e_ring, zca);
 	hr = rx_ring->xsk_umem->headroom + XDP_PACKET_HEADROOM;
-	mask = rx_ring->xsk_umem->props.chunk_mask;
+	mask = rx_ring->xsk_umem->chunk_mask;
 
 	nta = rx_ring->next_to_alloc;
 	bi = &rx_ring->rx_bi[nta];
-- 
2.7.4

Re: [PATCH bpf-next 0/3] xsk: misc code cleanup

[Alexei Starovoitov]

On Thu, Aug 30, 2018 at 03:56:40PM +0200, Magnus Karlsson wrote:
> This patch set cleans up two code style issues with the xsk zero-copy
> code. The resulting code is smaller and simpler.
> 
> Patch 1: Removes a potential compiler warning reported by the Intel
>          0-DAY kernel test infrastructure.
> Patches 2-3: Removes the xdp_umem_props structure. At some point, it
>              was used to break a dependency, but the members are these
>              days much better off in the xdp_umem since the dependency
>              does not exist anymore.
> 
> I based this patch set on bpf-next commit 234dbe3dc1db ("Merge branch
> 'verifier-liveness-simplification'")

Applied, Thanks

Re: [PATCH bpf-next 2/3] xsk: get rid of useless struct xdp_umem_props

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 4229 bytes --]

Hi Magnus,

I love your patch! Yet something to improve:

[auto build test ERROR on bpf-next/master]

url:    https://github.com/0day-ci/linux/commits/Magnus-Karlsson/i40e-fix-possible-compiler-warning-in-xsk-TX-path/20180831-095442
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 8.1.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=8.1.0 make.cross ARCH=ia64 

Note: the linux-review/Magnus-Karlsson/i40e-fix-possible-compiler-warning-in-xsk-TX-path/20180831-095442 HEAD 9b1f94c22aa3118f7657ec5a5124135bab6eb50b builds fine.
      It only hurts bisectibility.

All errors (new ones prefixed by >>):

   drivers/net//ethernet/intel/i40e/i40e_xsk.c: In function 'i40e_reuse_rx_buffer_zc':
>> drivers/net//ethernet/intel/i40e/i40e_xsk.c:445:55: error: 'struct xdp_umem' has no member named 'props'
     unsigned long mask = (unsigned long)rx_ring->xsk_umem->props.chunk_mask;
                                                          ^~
   drivers/net//ethernet/intel/i40e/i40e_xsk.c: In function 'i40e_zca_free':
   drivers/net//ethernet/intel/i40e/i40e_xsk.c:480:26: error: 'struct xdp_umem' has no member named 'props'
     mask = rx_ring->xsk_umem->props.chunk_mask;
                             ^~

vim +445 drivers/net//ethernet/intel/i40e/i40e_xsk.c

0a714186 Björn Töpel 2018-08-28  432  
0a714186 Björn Töpel 2018-08-28  433  /**
0a714186 Björn Töpel 2018-08-28  434   * i40e_reuse_rx_buffer_zc - Recycle an Rx buffer
0a714186 Björn Töpel 2018-08-28  435   * @rx_ring: Rx ring
0a714186 Björn Töpel 2018-08-28  436   * @old_bi: The Rx buffer to recycle
0a714186 Björn Töpel 2018-08-28  437   *
0a714186 Björn Töpel 2018-08-28  438   * This function recycles a finished Rx buffer, and places it on the
0a714186 Björn Töpel 2018-08-28  439   * recycle queue (next_to_alloc).
0a714186 Björn Töpel 2018-08-28  440   **/
0a714186 Björn Töpel 2018-08-28  441  static void i40e_reuse_rx_buffer_zc(struct i40e_ring *rx_ring,
0a714186 Björn Töpel 2018-08-28  442  				    struct i40e_rx_buffer *old_bi)
0a714186 Björn Töpel 2018-08-28  443  {
0a714186 Björn Töpel 2018-08-28  444  	struct i40e_rx_buffer *new_bi = &rx_ring->rx_bi[rx_ring->next_to_alloc];
0a714186 Björn Töpel 2018-08-28 @445  	unsigned long mask = (unsigned long)rx_ring->xsk_umem->props.chunk_mask;
0a714186 Björn Töpel 2018-08-28  446  	u64 hr = rx_ring->xsk_umem->headroom + XDP_PACKET_HEADROOM;
0a714186 Björn Töpel 2018-08-28  447  	u16 nta = rx_ring->next_to_alloc;
0a714186 Björn Töpel 2018-08-28  448  
0a714186 Björn Töpel 2018-08-28  449  	/* update, and store next to alloc */
0a714186 Björn Töpel 2018-08-28  450  	nta++;
0a714186 Björn Töpel 2018-08-28  451  	rx_ring->next_to_alloc = (nta < rx_ring->count) ? nta : 0;
0a714186 Björn Töpel 2018-08-28  452  
0a714186 Björn Töpel 2018-08-28  453  	/* transfer page from old buffer to new buffer */
0a714186 Björn Töpel 2018-08-28  454  	new_bi->dma = old_bi->dma & mask;
0a714186 Björn Töpel 2018-08-28  455  	new_bi->dma += hr;
0a714186 Björn Töpel 2018-08-28  456  
0a714186 Björn Töpel 2018-08-28  457  	new_bi->addr = (void *)((unsigned long)old_bi->addr & mask);
0a714186 Björn Töpel 2018-08-28  458  	new_bi->addr += hr;
0a714186 Björn Töpel 2018-08-28  459  
0a714186 Björn Töpel 2018-08-28  460  	new_bi->handle = old_bi->handle & mask;
0a714186 Björn Töpel 2018-08-28  461  	new_bi->handle += rx_ring->xsk_umem->headroom;
0a714186 Björn Töpel 2018-08-28  462  
0a714186 Björn Töpel 2018-08-28  463  	old_bi->addr = NULL;
0a714186 Björn Töpel 2018-08-28  464  }
0a714186 Björn Töpel 2018-08-28  465  

:::::: The code at line 445 was first introduced by commit
:::::: 0a714186d3c0f7c563a03537f98716457c1f5ae0 i40e: add AF_XDP zero-copy Rx support

:::::: TO: Björn Töpel <bjorn.topel@intel.com>
:::::: CC: Alexei Starovoitov <ast@kernel.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 51759 bytes --]

Re: [PATCH bpf-next 3/3] i40e: adapt driver to new xdp_umem structure

[Daniel Borkmann]

On 08/30/2018 03:56 PM, Magnus Karlsson wrote:
> The struct xdp_umem_props was removed in the xsk code and this commit
> adapts the i40e af_xdp zero-copy driver code to the new xdp_umem
> structure.
> 
> Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_xsk.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> index 41ca7e1..2ebfc78 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> @@ -442,7 +442,7 @@ static void i40e_reuse_rx_buffer_zc(struct i40e_ring *rx_ring,
>  				    struct i40e_rx_buffer *old_bi)
>  {
>  	struct i40e_rx_buffer *new_bi = &rx_ring->rx_bi[rx_ring->next_to_alloc];
> -	unsigned long mask = (unsigned long)rx_ring->xsk_umem->props.chunk_mask;
> +	unsigned long mask = (unsigned long)rx_ring->xsk_umem->chunk_mask;

That is buggy indeed, kbuild bot complained that this hurts bisectability
because you removed struct xdp_umem_props props from umem in prior patch.
Please respin the full series with this one squashed into prior commit instead
and I'll toss the old one from bpf-next tree.

>  	u64 hr = rx_ring->xsk_umem->headroom + XDP_PACKET_HEADROOM;
>  	u16 nta = rx_ring->next_to_alloc;
>  
> @@ -477,7 +477,7 @@ void i40e_zca_free(struct zero_copy_allocator *alloc, unsigned long handle)
>  
>  	rx_ring = container_of(alloc, struct i40e_ring, zca);
>  	hr = rx_ring->xsk_umem->headroom + XDP_PACKET_HEADROOM;
> -	mask = rx_ring->xsk_umem->props.chunk_mask;
> +	mask = rx_ring->xsk_umem->chunk_mask;
>  
>  	nta = rx_ring->next_to_alloc;
>  	bi = &rx_ring->rx_bi[nta];
>