lkml.org 
[lkml]   [2021]   [Jul]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH] gve: DQO: avoid unused variable warnings
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.

\
 
 \ /
  Last update: 2021-07-21 17:38    [W:0.019 / U:1.040 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site