RE: [PATCH net-next 0/4] Remove RTNL lock protection of CVQ

[Dan Jurgens <danielj@nvidia.com>]

> From: Heng Qi <hengqi@linux.alibaba.com>
> Sent: Monday, March 25, 2024 9:54 PM
> To: Dan Jurgens <danielj@nvidia.com>; netdev@vger.kernel.org
> Cc: mst@redhat.com; jasowang@redhat.com; xuanzhuo@linux.alibaba.com;
> virtualization@lists.linux.dev; davem@davemloft.net;
> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; Jiri Pirko
> <jiri@nvidia.com>
> Subject: Re: [PATCH net-next 0/4] Remove RTNL lock protection of CVQ
> 
> 
> 
> 在 2024/3/26 上午5:49, Daniel Jurgens 写道:
> > Currently the buffer used for control VQ commands is protected by the
> > RTNL lock. Previously this wasn't a major concern because the control
> > VQ was only used during device setup and user interaction. With the
> > recent addition of dynamic interrupt moderation the control VQ may be
> > used frequently during normal operation.
> >
> > This series removes the RNTL lock dependancy by introducing a spin
> > lock to protect the control buffer and writing SGs to the control VQ.
> 
> Hi Daniel.
> 
> It's a nice piece of work, but now that we're talking about ctrlq adding
> interrupts, spin lock has some conflicts with its goals. For example, we expect
> the ethtool command to be blocked.
> Therefore, a mutex lock may be more suitable.
> 
> Any how, the final conclusion may require some waiting.

Thanks, Heng

I took this a step further and made the ctrlq interrupt driven, but an internal reviewer pointed me to this:
https://lore.kernel.org/lkml/20230413064027.13267-1-jasowang@redhat.com/ (sorry if it gets safelinked)

It seemed there was little appetite to go that route last year, because of set RX mode behavior change, and consumption of an additional IRQ.

Either way, I think the spin lock is still needed. In my interrupt driven implantation I was allocating a new control buffer instead of just the data fields. The spin lock was tighter around virtqueue_add_sgs, after the kick it would unlock and wait for a completion that would be triggered from the cvq callback. 


> 
> Regards,
> Heng
> 
> >
> > Daniel Jurgens (4):
> >    virtio_net: Store RSS setting in virtnet_info
> >    virtio_net: Remove command data from control_buf
> >    virtio_net: Add a lock for the command VQ.
> >    virtio_net: Remove rtnl lock protection of command buffers
> >
> >   drivers/net/virtio_net.c | 185 ++++++++++++++++++++++-----------------
> >   1 file changed, 104 insertions(+), 81 deletions(-)
> >