> From: Heng Qi <hengqi@xxxxxxxxxxxxxxxxx>
> Sent: Tuesday, March 26, 2024 3:55 AM
> To: Dan Jurgens <danielj@xxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx
> Cc: mst@xxxxxxxxxx; jasowang@xxxxxxxxxx; xuanzhuo@xxxxxxxxxxxxxxxxx;
> virtualization@xxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx;
> edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx; Jiri Pirko
> <jiri@xxxxxxxxxx>
> Subject: Re: [PATCH net-next 4/4] virtio_net: Remove rtnl lock protection of
> command buffers
>
>
>
> 在 2024/3/26 上午5:49, Daniel Jurgens 写道:
> > The rtnl lock is no longer needed to protect the control buffer and
> > command VQ.
> >
> > Signed-off-by: Daniel Jurgens <danielj@xxxxxxxxxx>
> > Reviewed-by: Jiri Pirko <jiri@xxxxxxxxxx>
> > ---
> > drivers/net/virtio_net.c | 27 +++++----------------------
> > 1 file changed, 5 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index
> > 41f8dc16ff38..d09ea20b16be 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -2639,14 +2639,12 @@ static void virtnet_stats(struct net_device
> > *dev,
> >
> > static void virtnet_ack_link_announce(struct virtnet_info *vi)
> > {
> > - rtnl_lock();
> > if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_ANNOUNCE,
> > VIRTIO_NET_CTRL_ANNOUNCE_ACK,
> NULL))
> > dev_warn(&vi->dev->dev, "Failed to ack link announce.\n");
> > - rtnl_unlock();
> > }
> >
> > -static int _virtnet_set_queues(struct virtnet_info *vi, u16
> > queue_pairs)
> > +static int virtnet_set_queues(struct virtnet_info *vi, u16
> > +queue_pairs)
> > {
> > struct virtio_net_ctrl_mq *mq __free(kfree) = NULL;
> > struct scatterlist sg;
> > @@ -2677,16 +2675,6 @@ static int _virtnet_set_queues(struct
> virtnet_info *vi, u16 queue_pairs)
> > return 0;
> > }
> >
> > -static int virtnet_set_queues(struct virtnet_info *vi, u16
> > queue_pairs) -{
> > - int err;
> > -
> > - rtnl_lock();
> > - err = _virtnet_set_queues(vi, queue_pairs);
> > - rtnl_unlock();
> > - return err;
> > -}
> > -
> > static int virtnet_close(struct net_device *dev)
> > {
> > struct virtnet_info *vi = netdev_priv(dev); @@ -3268,7 +3256,7 @@
> > static int virtnet_set_channels(struct net_device *dev,
> > return -EINVAL;
> >
> > cpus_read_lock();
> > - err = _virtnet_set_queues(vi, queue_pairs);
> > + err = virtnet_set_queues(vi, queue_pairs);
> > if (err) {
> > cpus_read_unlock();
> > goto err;
> > @@ -3558,14 +3546,11 @@ static void virtnet_rx_dim_work(struct
> work_struct *work)
> > struct dim_cq_moder update_moder;
> > int i, qnum, err;
> >
> > - if (!rtnl_trylock())
> > - return;
> > -
>
> Does this guarantee that the synchronization is completely correct?
>
> The purpose of this patch set is to add a separate lock for ctrlq rather than
> reusing the RTNL lock.
> But for dim workers, it not only involves the use of ctrlq, but also involves
> reading shared variables in interfaces such as .set_coalesce, .get_coalesce,
> etc.
It looks like there is a risk of a dirty read in the get (usecs updated, but not max_packets). In the set it will return -EINVAL if trying to adjust the settings aside from DIM enabled. I can add a lock for this if you think it's needed, but it doesn't seem like a major problem for debug info.
>
> In addition, assuming there are 10 queues, each queue is scheduled with its
> own dim worker at the same time, then these 10 workers may issue
> parameters to rxq0 10 times in parallel, just because the RTNL lock is
> removed here.
>
> Therefore, when the RTNL lock is removed, a 'for loop' is no longer needed in
> virtnet_rx_dim_work, and the dim worker of each queue only configures its
> own parameters.
>
Good point. I'll add a new patch to remove the for loop.
> Alternatively, please keep RTNL lock here.
>
> Regards,
> Heng
>
> > /* Each rxq's work is queued by "net_dim()->schedule_work()"
> > * in response to NAPI traffic changes. Note that dim->profile_ix
> > * for each rxq is updated prior to the queuing action.
> > * So we only need to traverse and update profiles for all rxqs
> > - * in the work which is holding rtnl_lock.
> > + * in the work.
> > */
> > for (i = 0; i < vi->curr_queue_pairs; i++) {
> > rq = &vi->rq[i];
> > @@ -3587,8 +3572,6 @@ static void virtnet_rx_dim_work(struct
> work_struct *work)
> > dim->state = DIM_START_MEASURE;
> > }
> > }
> > -
> > - rtnl_unlock();
> > }
> >
> > static int virtnet_coal_params_supported(struct ethtool_coalesce
> > *ec) @@ -4036,7 +4019,7 @@ static int virtnet_xdp_set(struct net_device
> *dev, struct bpf_prog *prog,
> > synchronize_net();
> > }
> >
> > - err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
> > + err = virtnet_set_queues(vi, curr_qp + xdp_qp);
> > if (err)
> > goto err;
> > netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp); @@ -
> 4852,7
> > +4835,7 @@ static int virtnet_probe(struct virtio_device *vdev)
> >
> > virtio_device_ready(vdev);
> >
> > - _virtnet_set_queues(vi, vi->curr_queue_pairs);
> > + virtnet_set_queues(vi, vi->curr_queue_pairs);
> >
> > /* a random MAC address has been assigned, notify the device.
> > * We don't fail probe if VIRTIO_NET_F_CTRL_MAC_ADDR is not
> there
