Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] gve: DQO: avoid unused variable warnings
@ 2021-07-21 15:10 Arnd Bergmann
  2021-07-21 15:36 ` Bailey Forrest
  0 siblings, 1 reply; 4+ messages in thread
From: Arnd Bergmann @ 2021-07-21 15:10 UTC (permalink / raw)
  To: Catherine Sullivan, David S. Miller, Jakub Kicinski,
	Willem de Bruijn, Bailey Forrest
  Cc: Arnd Bergmann, Sagi Shahar, Jon Olson, netdev, linux-kernel

From: Arnd Bergmann <arnd@arndb.de>

The use of dma_unmap_addr()/dma_unmap_len() in the driver causes
multiple warnings when these macros are defined as empty:

drivers/net/ethernet/google/gve/gve_tx_dqo.c: In function 'gve_tx_add_skb_no_copy_dqo':
drivers/net/ethernet/google/gve/gve_tx_dqo.c:494:40: error: unused variable 'buf' [-Werror=unused-variable]
  494 |                 struct gve_tx_dma_buf *buf =

As it turns out, there are three copies of the same loop,
and one of them is already split out into a separate function.

Fix the warning in this one place, and change the other two
to call it instead of open-coding the same loop.

Fixes: a57e5de476be ("gve: DQO: Add TX path")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
The warning is present in both 5.14-rc2 and net-next as of today
---
 drivers/net/ethernet/google/gve/gve_tx_dqo.c | 92 ++++++++------------
 1 file changed, 35 insertions(+), 57 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve_tx_dqo.c b/drivers/net/ethernet/google/gve/gve_tx_dqo.c
index 05ddb6a75c38..fffa882db493 100644
--- a/drivers/net/ethernet/google/gve/gve_tx_dqo.c
+++ b/drivers/net/ethernet/google/gve/gve_tx_dqo.c
@@ -73,6 +73,26 @@ gve_free_pending_packet(struct gve_tx_ring *tx,
 	}
 }
 
+static void gve_unmap_packet(struct device *dev,
+			     struct gve_tx_pending_packet_dqo *pending_packet)
+{
+	dma_addr_t addr;
+	size_t len;
+	int i;
+
+	/* SKB linear portion is guaranteed to be mapped */
+	addr = dma_unmap_addr(&pending_packet->bufs[0], dma);
+	len = dma_unmap_len(&pending_packet->bufs[0], len);
+	dma_unmap_single(dev, addr, len, DMA_TO_DEVICE);
+
+	for (i = 1; i < pending_packet->num_bufs; i++) {
+		addr = dma_unmap_addr(&pending_packet->bufs[i], dma);
+		len = dma_unmap_len(&pending_packet->bufs[i], len);
+		dma_unmap_page(dev, addr, len, DMA_TO_DEVICE);
+	}
+	pending_packet->num_bufs = 0;
+}
+
 /* gve_tx_free_desc - Cleans up all pending tx requests and buffers.
  */
 static void gve_tx_clean_pending_packets(struct gve_tx_ring *tx)
@@ -82,23 +102,8 @@ static void gve_tx_clean_pending_packets(struct gve_tx_ring *tx)
 	for (i = 0; i < tx->dqo.num_pending_packets; i++) {
 		struct gve_tx_pending_packet_dqo *cur_state =
 			&tx->dqo.pending_packets[i];
-		int j;
-
-		for (j = 0; j < cur_state->num_bufs; j++) {
-			struct gve_tx_dma_buf *buf = &cur_state->bufs[j];
-
-			if (j == 0) {
-				dma_unmap_single(tx->dev,
-						 dma_unmap_addr(buf, dma),
-						 dma_unmap_len(buf, len),
-						 DMA_TO_DEVICE);
-			} else {
-				dma_unmap_page(tx->dev,
-					       dma_unmap_addr(buf, dma),
-					       dma_unmap_len(buf, len),
-					       DMA_TO_DEVICE);
-			}
-		}
+
+		gve_unmap_packet(tx->dev, cur_state);
 		if (cur_state->skb) {
 			dev_consume_skb_any(cur_state->skb);
 			cur_state->skb = NULL;
@@ -445,6 +450,13 @@ gve_tx_fill_general_ctx_desc(struct gve_tx_general_context_desc_dqo *desc,
 	};
 }
 
+static inline void gve_tx_dma_buf_set(struct gve_tx_dma_buf *buf,
+				      dma_addr_t addr, size_t len)
+{
+	dma_unmap_len_set(buf, len, len);
+	dma_unmap_addr_set(buf, dma, addr);
+}
+
 /* Returns 0 on success, or < 0 on error.
  *
  * Before this function is called, the caller must ensure
@@ -459,6 +471,7 @@ static int gve_tx_add_skb_no_copy_dqo(struct gve_tx_ring *tx,
 
 	struct gve_tx_pending_packet_dqo *pending_packet;
 	struct gve_tx_metadata_dqo metadata;
+	struct gve_tx_dma_buf *buf;
 	s16 completion_tag;
 	int i;
 
@@ -493,8 +506,6 @@ static int gve_tx_add_skb_no_copy_dqo(struct gve_tx_ring *tx,
 
 	/* Map the linear portion of skb */
 	{
-		struct gve_tx_dma_buf *buf =
-			&pending_packet->bufs[pending_packet->num_bufs];
 		u32 len = skb_headlen(skb);
 		dma_addr_t addr;
 
@@ -502,8 +513,8 @@ static int gve_tx_add_skb_no_copy_dqo(struct gve_tx_ring *tx,
 		if (unlikely(dma_mapping_error(tx->dev, addr)))
 			goto err;
 
-		dma_unmap_len_set(buf, len, len);
-		dma_unmap_addr_set(buf, dma, addr);
+		buf = &pending_packet->bufs[pending_packet->num_bufs];
+		gve_tx_dma_buf_set(buf, addr, len);
 		++pending_packet->num_bufs;
 
 		gve_tx_fill_pkt_desc_dqo(tx, &desc_idx, skb, len, addr,
@@ -512,8 +523,6 @@ static int gve_tx_add_skb_no_copy_dqo(struct gve_tx_ring *tx,
 	}
 
 	for (i = 0; i < shinfo->nr_frags; i++) {
-		struct gve_tx_dma_buf *buf =
-			&pending_packet->bufs[pending_packet->num_bufs];
 		const skb_frag_t *frag = &shinfo->frags[i];
 		bool is_eop = i == (shinfo->nr_frags - 1);
 		u32 len = skb_frag_size(frag);
@@ -523,8 +532,8 @@ static int gve_tx_add_skb_no_copy_dqo(struct gve_tx_ring *tx,
 		if (unlikely(dma_mapping_error(tx->dev, addr)))
 			goto err;
 
-		dma_unmap_len_set(buf, len, len);
-		dma_unmap_addr_set(buf, dma, addr);
+		buf = &pending_packet->bufs[pending_packet->num_bufs];
+		gve_tx_dma_buf_set(buf, addr, len);
 		++pending_packet->num_bufs;
 
 		gve_tx_fill_pkt_desc_dqo(tx, &desc_idx, skb, len, addr,
@@ -552,21 +561,8 @@ static int gve_tx_add_skb_no_copy_dqo(struct gve_tx_ring *tx,
 	return 0;
 
 err:
-	for (i = 0; i < pending_packet->num_bufs; i++) {
-		struct gve_tx_dma_buf *buf = &pending_packet->bufs[i];
-
-		if (i == 0) {
-			dma_unmap_single(tx->dev, dma_unmap_addr(buf, dma),
-					 dma_unmap_len(buf, len),
-					 DMA_TO_DEVICE);
-		} else {
-			dma_unmap_page(tx->dev, dma_unmap_addr(buf, dma),
-				       dma_unmap_len(buf, len), DMA_TO_DEVICE);
-		}
-	}
-
+	gve_unmap_packet(tx->dev, pending_packet);
 	pending_packet->skb = NULL;
-	pending_packet->num_bufs = 0;
 	gve_free_pending_packet(tx, pending_packet);
 
 	return -1;
@@ -746,24 +742,6 @@ static void remove_from_list(struct gve_tx_ring *tx,
 	}
 }
 
-static void gve_unmap_packet(struct device *dev,
-			     struct gve_tx_pending_packet_dqo *pending_packet)
-{
-	struct gve_tx_dma_buf *buf;
-	int i;
-
-	/* SKB linear portion is guaranteed to be mapped */
-	buf = &pending_packet->bufs[0];
-	dma_unmap_single(dev, dma_unmap_addr(buf, dma),
-			 dma_unmap_len(buf, len), DMA_TO_DEVICE);
-	for (i = 1; i < pending_packet->num_bufs; i++) {
-		buf = &pending_packet->bufs[i];
-		dma_unmap_page(dev, dma_unmap_addr(buf, dma),
-			       dma_unmap_len(buf, len), DMA_TO_DEVICE);
-	}
-	pending_packet->num_bufs = 0;
-}
-
 /* Completion types and expected behavior:
  * No Miss compl + Packet compl = Packet completed normally.
  * Miss compl + Re-inject compl = Packet completed normally.
-- 
2.29.2


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] gve: DQO: avoid unused variable warnings
  2021-07-21 15:10 [PATCH] gve: DQO: avoid unused variable warnings Arnd Bergmann
@ 2021-07-21 15:36 ` Bailey Forrest
  2021-07-21 18:38   ` Arnd Bergmann
  0 siblings, 1 reply; 4+ messages in thread
From: Bailey Forrest @ 2021-07-21 15:36 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Catherine Sullivan, David S. Miller, Jakub Kicinski,
	Willem de Bruijn, Arnd Bergmann, Sagi Shahar, Jon Olson, netdev,
	linux-kernel

Thanks for the patch!

On Wed, Jul 21, 2021 at 8:11 AM Arnd Bergmann <arnd@kernel.org> wrote:
>
> From: Arnd Bergmann <arnd@arndb.de>
>
> The use of dma_unmap_addr()/dma_unmap_len() in the driver causes
> multiple warnings when these macros are defined as empty:
>
> drivers/net/ethernet/google/gve/gve_tx_dqo.c: In function 'gve_tx_add_skb_no_copy_dqo':
> drivers/net/ethernet/google/gve/gve_tx_dqo.c:494:40: error: unused variable 'buf' [-Werror=unused-variable]
>   494 |                 struct gve_tx_dma_buf *buf =
>
> As it turns out, there are three copies of the same loop,
> and one of them is already split out into a separate function.
>
> Fix the warning in this one place, and change the other two
> to call it instead of open-coding the same loop.
>
> Fixes: a57e5de476be ("gve: DQO: Add TX path")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> The warning is present in both 5.14-rc2 and net-next as of today
> ---
>  drivers/net/ethernet/google/gve/gve_tx_dqo.c | 92 ++++++++------------
>  1 file changed, 35 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/net/ethernet/google/gve/gve_tx_dqo.c b/drivers/net/ethernet/google/gve/gve_tx_dqo.c
> index 05ddb6a75c38..fffa882db493 100644
> --- a/drivers/net/ethernet/google/gve/gve_tx_dqo.c
> +++ b/drivers/net/ethernet/google/gve/gve_tx_dqo.c
> @@ -73,6 +73,26 @@ gve_free_pending_packet(struct gve_tx_ring *tx,
>         }
>  }
>
> +static void gve_unmap_packet(struct device *dev,
> +                            struct gve_tx_pending_packet_dqo *pending_packet)
> +{
> +       dma_addr_t addr;
> +       size_t len;
> +       int i;
> +
> +       /* SKB linear portion is guaranteed to be mapped */
> +       addr = dma_unmap_addr(&pending_packet->bufs[0], dma);
> +       len = dma_unmap_len(&pending_packet->bufs[0], len);
> +       dma_unmap_single(dev, addr, len, DMA_TO_DEVICE);

"SKB linear portion is guaranteed to be mapped" is only true if
gve_tx_add_skb_no_copy_dqo completed successfully.

This optimization is important for the success path because otherwise
there would be a per-packet branch misprediction, which I found to
have a large performance impact.

A solution which should address this would be something like:

+static void gve_unmap_packet(struct device *dev,
+     struct gve_tx_pending_packet_dqo *pending_packet
+     bool always_unmap_first)
+{
+ dma_addr_t addr;
+ size_t len;
+ int i;
+
+ if (always_unmap_first || pending_packet->num_bufs > 0) {
+  addr = dma_unmap_addr(&pending_packet->bufs[0], dma);
+  len = dma_unmap_len(&pending_packet->bufs[0], len);
+  dma_unmap_single(dev, addr, len, DMA_TO_DEVICE);
+ }
+
+ for (i = 1; i < pending_packet->num_bufs; i++) {
+  addr = dma_unmap_addr(&pending_packet->bufs[i], dma);
+  len = dma_unmap_len(&pending_packet->bufs[i], len);
+  dma_unmap_page(dev, addr, len, DMA_TO_DEVICE);
+ }
+ pending_packet->num_bufs = 0;
+}

(Sorry my email client keeps turning tabs into spaces...)

By doing this, we can rely on the compiler to optimize away the extra
branch in cases we know the first buffer will be mapped.

>
> +static inline void gve_tx_dma_buf_set(struct gve_tx_dma_buf *buf,
> +                                     dma_addr_t addr, size_t len)
> +{
> +       dma_unmap_len_set(buf, len, len);
> +       dma_unmap_addr_set(buf, dma, addr);
> +}

checkpatch.pl will complain about `inline` in a C file.

However, I would prefer to just not introduce this helper because it
introduces indirection for the reader and the risk of passing the
arguments in the wrong order. Don't have a strong opinion here though.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] gve: DQO: avoid unused variable warnings
  2021-07-21 15:36 ` Bailey Forrest
@ 2021-07-21 18:38   ` Arnd Bergmann
  2021-07-22 20:19     ` Bailey Forrest
  0 siblings, 1 reply; 4+ messages in thread
From: Arnd Bergmann @ 2021-07-21 18:38 UTC (permalink / raw)
  To: Bailey Forrest
  Cc: Catherine Sullivan, David S. Miller, Jakub Kicinski,
	Willem de Bruijn, Arnd Bergmann, Sagi Shahar, Jon Olson,
	Networking, Linux Kernel Mailing List

dma_unmap_len_set(pending_packet->bufs[pending_packet->num_bufs], len, len);
On Wed, Jul 21, 2021 at 5:36 PM Bailey Forrest <bcf@google.com> wrote:
> On Wed, Jul 21, 2021 at 8:11 AM Arnd Bergmann <arnd@kernel.org> wrote:
> >
> >
> > +static void gve_unmap_packet(struct device *dev,
> > +                            struct gve_tx_pending_packet_dqo *pending_packet)
> > +{
> > +       dma_addr_t addr;
> > +       size_t len;
> > +       int i;
> > +
> > +       /* SKB linear portion is guaranteed to be mapped */
> > +       addr = dma_unmap_addr(&pending_packet->bufs[0], dma);
> > +       len = dma_unmap_len(&pending_packet->bufs[0], len);
> > +       dma_unmap_single(dev, addr, len, DMA_TO_DEVICE);
>
> "SKB linear portion is guaranteed to be mapped" is only true if
> gve_tx_add_skb_no_copy_dqo completed successfully.
>
> This optimization is important for the success path because otherwise
> there would be a per-packet branch misprediction, which I found to
> have a large performance impact.
>
> A solution which should address this would be something like:
>
> +static void gve_unmap_packet(struct device *dev,
> +     struct gve_tx_pending_packet_dqo *pending_packet
> +     bool always_unmap_first)
> +{
> + dma_addr_t addr;
> + size_t len;
> + int i;
> +
> + if (always_unmap_first || pending_packet->num_bufs > 0) {
> +  addr = dma_unmap_addr(&pending_packet->bufs[0], dma);
> +  len = dma_unmap_len(&pending_packet->bufs[0], len);
> +  dma_unmap_single(dev, addr, len, DMA_TO_DEVICE);
> + }
> +
> + for (i = 1; i < pending_packet->num_bufs; i++) {
> +  addr = dma_unmap_addr(&pending_packet->bufs[i], dma);
> +  len = dma_unmap_len(&pending_packet->bufs[i], len);
> +  dma_unmap_page(dev, addr, len, DMA_TO_DEVICE);
> + }
> + pending_packet->num_bufs = 0;
> +}
>
> (Sorry my email client keeps turning tabs into spaces...)
>
> By doing this, we can rely on the compiler to optimize away the extra
> branch in cases we know the first buffer will be mapped.

I didn't really change it here, I just moved the function up and changed
the dma_unmap_addr/dma_unmap_len calls to avoid the warning.

> > +static inline void gve_tx_dma_buf_set(struct gve_tx_dma_buf *buf,
> > +                                     dma_addr_t addr, size_t len)
> > +{
> > +       dma_unmap_len_set(buf, len, len);
> > +       dma_unmap_addr_set(buf, dma, addr);
> > +}
>
> checkpatch.pl will complain about `inline` in a C file.
>
> However, I would prefer to just not introduce this helper because it
> introduces indirection for the reader and the risk of passing the
> arguments in the wrong order. Don't have a strong opinion here
> though.

Sure, feel free to just treat my patch as a bug report and send a different
fix if you prefer to not have an inline function. This is usually the easiest
way to get around the macro ignoring its arguments since the compiler
does not warn for unused function arguments.

Open-codiung the call as

dma_unmap_len_set(pending_packet->bufs[pending_packet->num_bufs], len, len);
dma_unmap_len_addr(pending_packet->bufs[pending_packet->num_bufs], addr, dma);

works as well, I just found the inline function more readable.

     Arnd

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] gve: DQO: avoid unused variable warnings
  2021-07-21 18:38   ` Arnd Bergmann
@ 2021-07-22 20:19     ` Bailey Forrest
  0 siblings, 0 replies; 4+ messages in thread
From: Bailey Forrest @ 2021-07-22 20:19 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Catherine Sullivan, David S. Miller, Jakub Kicinski,
	Willem de Bruijn, Arnd Bergmann, Sagi Shahar, Jon Olson,
	Networking, Linux Kernel Mailing List

> Sure, feel free to just treat my patch as a bug report and send a different
> fix if you prefer to not have an inline function. This is usually the easiest
> way to get around the macro ignoring its arguments since the compiler
> does not warn for unused function arguments.

Okay, I will propose my own fix and send the patch hopefully today or tomorrow.

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-07-22 20:19 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-21 15:10 [PATCH] gve: DQO: avoid unused variable warnings Arnd Bergmann
2021-07-21 15:36 ` Bailey Forrest
2021-07-21 18:38   ` Arnd Bergmann
2021-07-22 20:19     ` Bailey Forrest

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).