lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 21 Jul 2021 20:38:38 +0200
From:   Arnd Bergmann <arnd@...nel.org>
To:     Bailey Forrest <bcf@...gle.com>
Cc:     Catherine Sullivan <csully@...gle.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Willem de Bruijn <willemb@...gle.com>,
        Arnd Bergmann <arnd@...db.de>, Sagi Shahar <sagis@...gle.com>,
        Jon Olson <jonolson@...gle.com>,
        Networking <netdev@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] gve: DQO: avoid unused variable warnings

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@...gle.com> wrote:
> On Wed, Jul 21, 2021 at 8:11 AM Arnd Bergmann <arnd@...nel.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

Powered by blists - more mailing lists