> 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
