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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 26 Apr 2024 13:14:43 +0000
From: Dan Jurgens <danielj@...dia.com>
To: Paolo Abeni <pabeni@...hat.com>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>
CC: "mst@...hat.com" <mst@...hat.com>, "jasowang@...hat.com"
	<jasowang@...hat.com>, "xuanzhuo@...ux.alibaba.com"
	<xuanzhuo@...ux.alibaba.com>, "virtualization@...ts.linux.dev"
	<virtualization@...ts.linux.dev>, "davem@...emloft.net"
	<davem@...emloft.net>, "edumazet@...gle.com" <edumazet@...gle.com>,
	"kuba@...nel.org" <kuba@...nel.org>, Jiri Pirko <jiri@...dia.com>
Subject: RE: [PATCH net-next v5 5/6] virtio_net: Add a lock for per queue RX
 coalesce

> From: Paolo Abeni <pabeni@...hat.com>
> Sent: Friday, April 26, 2024 4:48 AM
> To: Dan Jurgens <danielj@...dia.com>; netdev@...r.kernel.org
> Cc: mst@...hat.com; jasowang@...hat.com; xuanzhuo@...ux.alibaba.com;
> virtualization@...ts.linux.dev; davem@...emloft.net;
> edumazet@...gle.com; kuba@...nel.org; Jiri Pirko <jiri@...dia.com>
> 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@...dia.com>
> > ---
> >  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