Re: [RFC PATCH net] virtio_net: Prevent napi_weight changes with VIRTIO_NET_F_NOTF_COAL support

[Jason Wang]

On Tue, Jun 6, 2023 at 9:57 AM Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> wrote:
>
> On Mon, 5 Jun 2023 14:02:36 -0700, Brett Creeley <brett.creeley@xxxxxxx> wrote:
> > Commit 699b045a8e43 ("net: virtio_net: notifications coalescing
> > support") added support for VIRTIO_NET_F_NOTF_COAL. The get_coalesce
> > call made changes to report "1" in tx_max_coalesced_frames if
> > VIRTIO_NET_F_NOTF_COAL is not supported and napi.weight is non-zero.
> > However, the napi_weight value could still be changed by the
> > set_coalesce call regardless of whether or not the device supports
> > VIRTIO_NET_F_NOTF_COAL.
> >
> > It seems like the tx_max_coalesced_frames value should not control more
> > than 1 thing (i.e. napi_weight and the device's tx_max_packets). So, fix
> > this by only allowing the napi_weight change if VIRTIO_NET_F_NOTF_COAL
> > is not supported by the virtio device.
>
>
> @Jason I wonder should we keep this function to change the napi weight by the
> coalesec command.

I think so, explained in another thread.

Thanks

>
> Thanks.
>
> >
> > It wasn't clear to me if this was the intended behavior, so that's why
> > I'm sending this as an RFC patch initially. Based on the feedback, I
> > will resubmit as an official patch.
> >
> > Fixes: 699b045a8e43 ("net: virtio_net: notifications coalescing support")
> > Signed-off-by: Brett Creeley <brett.creeley@xxxxxxx>
> > ---
> >  drivers/net/virtio_net.c | 24 +++++++++++++-----------
> >  1 file changed, 13 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 486b5849033d..e28387866909 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -2990,19 +2990,21 @@ static int virtnet_set_coalesce(struct net_device *dev,
> >       int ret, i, napi_weight;
> >       bool update_napi = false;
> >
> > -     /* Can't change NAPI weight if the link is up */
> > -     napi_weight = ec->tx_max_coalesced_frames ? NAPI_POLL_WEIGHT : 0;
> > -     if (napi_weight ^ vi->sq[0].napi.weight) {
> > -             if (dev->flags & IFF_UP)
> > -                     return -EBUSY;
> > -             else
> > -                     update_napi = true;
> > -     }
> > -
> > -     if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_NOTF_COAL))
> > +     if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_NOTF_COAL)) {
> >               ret = virtnet_send_notf_coal_cmds(vi, ec);
> > -     else
> > +     } else {
> > +             /* Can't change NAPI weight if the link is up */
> > +             napi_weight = ec->tx_max_coalesced_frames ?
> > +                     NAPI_POLL_WEIGHT : 0;
> > +             if (napi_weight ^ vi->sq[0].napi.weight) {
> > +                     if (dev->flags & IFF_UP)
> > +                             return -EBUSY;
> > +                     else
> > +                             update_napi = true;
> > +             }
> > +
> >               ret = virtnet_coal_params_supported(ec);
> > +     }
> >
> >       if (ret)
> >               return ret;
> > --
> > 2.17.1
> >
>