netdev - Re: [PATCH net-next 2/2] virtio_net: Implement XDP_FLAGS_NO

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 linux-cve-announce 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]

Message-ID: <pj41zllfczpyny.fsf@u68c7b5b1d2d758.ant.amazon.com>
Date:   Mon, 11 Jan 2021 12:15:29 +0200
From:   Shay Agroskin <shayagr@...zon.com>
To:     Charlie Somerville <charlie@...rlie.bz>
CC:     <davem@...emloft.net>, <kuba@...nel.org>, <mst@...hat.com>,
        <jasowang@...hat.com>, <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 2/2] virtio_net: Implement XDP_FLAGS_NO_TX support


Charlie Somerville <charlie@...rlie.bz> writes:

> On Mon, Jan 11, 2021, at 04:31, Shay Agroskin wrote:
>
>> Is this addition needed ? Seems like we don't set VIRTIO_XDP_TX 
>> bit in case of virtnet_xdp_xmit() failure, so the surrounding 
>> 'if' 
>> won't be taken.
>
> Good catch, it looks like you're right. I'm happy to remove that 
> extra branch although I would like to add a comment explaining 
> that assumption:
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index ed08998765e0..3ae7cd2f1e72 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1472,8 +1472,10 @@ static int virtnet_poll(struct 
> napi_struct *napi, int budget)
>                 xdp_do_flush();
>
>         if (xdp_xmit & VIRTIO_XDP_TX) {
> +               /* VIRTIO_XDP_TX only set on successful 
> virtnet_xdp_xmit,
> +                * implies sq != NULL */
>                 sq = virtnet_xdp_sq(vi);
> -               if (sq && virtqueue_kick_prepare(sq->vq) && 
> virtqueue_notify(sq->vq)) {
> +               if (virtqueue_kick_prepare(sq->vq) && 
> virtqueue_notify(sq->vq)) {
>                         u64_stats_update_begin(&sq->stats.syncp);
>                         sq->stats.kicks++;
>                         u64_stats_update_end(&sq->stats.syncp);

The comment itself looks good. Note that the code style dictates 
that multi-line comments should end up with a line containing '*/' 
alone.
See 
https://www.kernel.org/doc/html/v4.10/process/coding-style.html#commenting 
for more information

Also you'd probably need to send a new email containing the new 
patchset (see V# tag on emails in the mailing list)

Shay