Re: [PATCH net-next v2 2/2] virtio_net: get init coalesce value when probe

[Heng Qi <hengqi@linux.alibaba.com>]

On Tue, 7 May 2024 14:24:12 +0800, Jason Wang <jasowang@redhat.com> wrote:
> On Fri, Apr 26, 2024 at 2:54 PM Heng Qi <hengqi@linux.alibaba.com> wrote:
> >
> > Currently, virtio-net lacks a way to obtain the default coalesce
> > values of the device during the probe phase. That is, the device
> > may have default experience values, but the user uses "ethtool -c"
> > to query that the values are still 0.
> >
> > Therefore, we reuse VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET to complete the goal.
> >
> > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > ---
> >  drivers/net/virtio_net.c | 68 +++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 61 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 3bc9b1e621db..fe0c15819dd3 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -4623,6 +4623,46 @@ static int virtnet_validate(struct virtio_device *vdev)
> >         return 0;
> >  }
> >
> > +static int virtnet_get_coal_init_value(struct virtnet_info *vi,
> > +                                      u16 _vqn, bool is_tx)
> > +{
> > +       struct virtio_net_ctrl_coal *coal = &vi->ctrl->coal_vq.coal;
> > +       __le16 *vqn = &vi->ctrl->coal_vq.vqn;
> > +       struct scatterlist sgs_in, sgs_out;
> > +       u32 usecs, pkts, i;
> > +       bool ret;
> > +
> > +       *vqn = cpu_to_le16(_vqn);
> > +
> > +       sg_init_one(&sgs_out, vqn, sizeof(*vqn));
> > +       sg_init_one(&sgs_in, coal, sizeof(*coal));
> > +       ret = virtnet_send_command_reply(vi, VIRTIO_NET_CTRL_NOTF_COAL,
> > +                                        VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET,
> > +                                        &sgs_out, &sgs_in);
> > +       if (!ret)
> > +               return ret;
> > +
> > +       usecs = le32_to_cpu(coal->max_usecs);
> > +       pkts = le32_to_cpu(coal->max_packets);
> > +       if (is_tx) {
> > +               vi->intr_coal_tx.max_usecs = usecs;
> > +               vi->intr_coal_tx.max_packets = pkts;
> > +               for (i = 0; i < vi->max_queue_pairs; i++) {
> > +                       vi->sq[i].intr_coal.max_usecs = usecs;
> > +                       vi->sq[i].intr_coal.max_packets = pkts;
> > +               }
> > +       } else {
> > +               vi->intr_coal_rx.max_usecs = usecs;
> > +               vi->intr_coal_rx.max_packets = pkts;
> > +               for (i = 0; i < vi->max_queue_pairs; i++) {
> > +                       vi->rq[i].intr_coal.max_usecs = usecs;
> > +                       vi->rq[i].intr_coal.max_packets = pkts;
> > +               }
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> >  static bool virtnet_check_guest_gso(const struct virtnet_info *vi)
> >  {
> >         return virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO4) ||
> > @@ -4885,13 +4925,6 @@ static int virtnet_probe(struct virtio_device *vdev)
> >                         vi->intr_coal_tx.max_packets = 0;
> >         }
> >
> > -       if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_VQ_NOTF_COAL)) {
> > -               /* The reason is the same as VIRTIO_NET_F_NOTF_COAL. */
> > -               for (i = 0; i < vi->max_queue_pairs; i++)
> > -                       if (vi->sq[i].napi.weight)
> > -                               vi->sq[i].intr_coal.max_packets = 1;
> > -       }
> > -
> >  #ifdef CONFIG_SYSFS
> >         if (vi->mergeable_rx_bufs)
> >                 dev->sysfs_rx_queue_group = &virtio_net_mrg_rx_group;
> > @@ -4926,6 +4959,27 @@ static int virtnet_probe(struct virtio_device *vdev)
> >
> >         virtio_device_ready(vdev);
> >
> > +       if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_VQ_NOTF_COAL)) {
> > +               /* The reason is the same as VIRTIO_NET_F_NOTF_COAL. */
> > +               for (i = 0; i < vi->max_queue_pairs; i++)
> > +                       if (vi->sq[i].napi.weight)
> > +                               vi->sq[i].intr_coal.max_packets = 1;
> > +
> > +               /* The loop exits if the default value from any
> > +                * queue is successfully read.
> > +                */
> 
> So this assumes the default values are the same. Is this something
> required by the spec? If not, we probably need to iterate all the
> queues.
> 

From internal practice, and from the default behavior of other existing drivers,
the queues all have the same value at the beginning, so here it seems feasible
that we get the value of queue 0 to represent the global value instead of using
a loop.

Moreover, obtaining the value once for each queue initially does not seem to be
very friendly for devices with a large number of queues.

Thanks.

> Thanks
> 
> 
> > +               for (i = 0; i < vi->max_queue_pairs; i++) {
> > +                       err = virtnet_get_coal_init_value(vi, rxq2vq(i), false);
> > +                       if (!err)
> > +                               break;
> > +               }
> > +               for (i = 0; i < vi->max_queue_pairs; i++) {
> > +                       err = virtnet_get_coal_init_value(vi, txq2vq(i), true);
> > +                       if (!err)
> > +                               break;
> > +               }
> > +       }
> > +
> >         _virtnet_set_queues(vi, vi->curr_queue_pairs);
> >
> >         /* a random MAC address has been assigned, notify the device.
> > --
> > 2.32.0.3.g01195cf9f
> >
>