RE: [PATCH net-next v5 5/6] virtio_net: Add a lock for per queue RX coalesce

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

 



> From: Paolo Abeni <pabeni@xxxxxxxxxx>
> Sent: Friday, April 26, 2024 4:48 AM
> To: Dan Jurgens <danielj@xxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx
> Cc: mst@xxxxxxxxxx; jasowang@xxxxxxxxxx; xuanzhuo@xxxxxxxxxxxxxxxxx;
> virtualization@xxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx;
> edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx; Jiri Pirko <jiri@xxxxxxxxxx>
> Subject: Re: [PATCH net-next v5 5/6] virtio_net: Add a lock for per queue RX
> coalesce
> 
> On Tue, 2024-04-23 at 06:57 +0300, Daniel Jurgens wrote:
> > Once the RTNL locking around the control buffer is removed there can
> > be contention on the per queue RX interrupt coalescing data. Use a
> > mutex per queue. A mutex is required because virtnet_send_command
> can sleep.
> >
> > Signed-off-by: Daniel Jurgens <danielj@xxxxxxxxxx>
> > ---
> >  drivers/net/virtio_net.c | 53
> > +++++++++++++++++++++++++++++++---------
> >  1 file changed, 41 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index
> > af9048ddc3c1..033e1d6ea31b 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -184,6 +184,9 @@ struct receive_queue {
> >  	/* Is dynamic interrupt moderation enabled? */
> >  	bool dim_enabled;
> >
> > +	/* Used to protect dim_enabled and inter_coal */
> > +	struct mutex dim_lock;
> > +
> >  	/* Dynamic Interrupt Moderation */
> >  	struct dim dim;
> >
> > @@ -2218,6 +2221,10 @@ static int virtnet_poll(struct napi_struct *napi, int
> budget)
> >  	/* Out of packets? */
> >  	if (received < budget) {
> >  		napi_complete = virtqueue_napi_complete(napi, rq->vq,
> received);
> > +		/* Intentionally not taking dim_lock here. This could result
> > +		 * in a net_dim call with dim now disabled. But
> virtnet_rx_dim_work
> > +		 * will take the lock not update settings if dim is now disabled.
> 
> Minor nit: the above comment looks confusing/mangled to me ?!?

I wanted to note that dim_lock is being accessed here, without the lock. But it's intentional. If there is racing a spurious net dim call can happen. But the dim_work handler will take the lock, see the correct value, and do nothing if dim is now disabled.
> 
> 		   will take the lock and will not update settings...
> 
> Thanks,
> 
> 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