RE: [PATCH net-next v4 3/6] virtio_net: Add a lock for the command VQ.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> From: Paolo Abeni <pabeni@xxxxxxxxxx>
> Sent: Thursday, April 18, 2024 5:57 AM
> On Thu, 2024-04-18 at 15:36 +0800, Heng Qi wrote:
> >
> > 在 2024/4/18 下午2:42, Jason Wang 写道:
> > > On Wed, Apr 17, 2024 at 3:31 AM Daniel Jurgens <danielj@xxxxxxxxxx>
> wrote:
> > > > The command VQ will no longer be protected by the RTNL lock. Use a
> > > > spinlock to protect the control buffer header and the VQ.
> > > >
> > > > Signed-off-by: Daniel Jurgens <danielj@xxxxxxxxxx>
> > > > Reviewed-by: Jiri Pirko <jiri@xxxxxxxxxx>
> > > > ---
> > > >   drivers/net/virtio_net.c | 6 +++++-
> > > >   1 file changed, 5 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > index 0ee192b45e1e..d02f83a919a7 100644
> > > > --- a/drivers/net/virtio_net.c
> > > > +++ b/drivers/net/virtio_net.c
> > > > @@ -282,6 +282,7 @@ struct virtnet_info {
> > > >
> > > >          /* Has control virtqueue */
> > > >          bool has_cvq;
> > > > +       spinlock_t cvq_lock;
> > > Spinlock is instead of mutex which is problematic as there's no
> > > guarantee on when the driver will get a reply. And it became even
> > > more serious after 0d197a147164 ("virtio-net: add cond_resched() to
> > > the command waiting loop").
> > >
> > > Any reason we can't use mutex?
> >
> > Hi Jason,
> >
> > I made a patch set to enable ctrlq's irq on top of this patch set,
> > which removes cond_resched().
> >
> > But I need a little time to test, this is close to fast. So could the
> > topic about cond_resched + spin lock or mutex lock be wait?
> 
> The big problem is that until the cond_resched() is there, replacing the
> mutex with a spinlock can/will lead to scheduling while atomic splats. We
> can't intentionally introduce such scenario.

When I created the series set_rx_mode wasn't moved to a work queue, and the cond_resched wasn't there. Mutex wasn't possible, then. If the CVQ is made to be event driven, then the lock can be released right after posting the work to the VQ.

> 
> Side note: the compiler apparently does not like guard() construct, leading to
> new warning, here and in later patches. I'm unsure if the code simplification
> is worthy.

I didn't see any warnings with GCC or clang. This is used other places in the kernel as well.
gcc version 13.2.1 20230918 (Red Hat 13.2.1-3) (GCC)
clang version 17.0.6 (Fedora 17.0.6-2.fc39)
> 
> Cheers,
> 
> Paolo





[Index of Archives]     [KVM Development]     [Libvirt Development]     [Libvirt Users]     [CentOS Virtualization]     [Netdev]     [Ethernet Bridging]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]

  Powered by Linux