From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pf1-f175.google.com (mail-pf1-f175.google.com [209.85.210.175]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 8BC2E25785E for ; Thu, 11 Dec 2025 15:04:32 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.210.175 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1765465474; cv=none; b=npl3YdFKpjQy26YkdZ25Za7q63rHomL1GeoNrolcvWzu+ujcqCuLgFUW5CA//Yl18ZlYyM9Dd0Bsjl2grpqu9WFD4JW7VlDiTQ1PtT29HayEIJ9KIqmzGxnoRKPUOx98Pu65PlGQ9zQe/q8nLbVj/+U8A3nsvC1Kl4+0p4LNy4E= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1765465474; c=relaxed/simple; bh=BHz3xcbiNKi6kN2GjKxGbF0MGFW5yOnSwwfsylgU5cs=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=u2DJtciW2ky3DvibqGEsWjd4Sz51JTBIVu0i5JGwhJnv3W2BhjqL6133D74C2oRCFuRAtTI6MV0HWYfFDzVn2i/9HMZgkgMlnZkQ721E2I6zOnIP2x2TfH6vaeVltsSVnyVycmPNGRCwzbwwVwIyQu05zJXjUmqXu2ZQySizgbQ= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=k/3FbWWc; arc=none smtp.client-ip=209.85.210.175 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="k/3FbWWc" Received: by mail-pf1-f175.google.com with SMTP id d2e1a72fcca58-7aa2170adf9so182770b3a.0 for ; Thu, 11 Dec 2025 07:04:32 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1765465472; x=1766070272; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=QGiOaxxeedYSgO7KsDHaFkoLTRKOhWXoKaus+BgsSis=; b=k/3FbWWcPeZjA11G8NakVceLqCa927GzenD+CQf6b9tQV6ugl5ZvPYiRzMv1hBXrli +5bIsiuKmCI5UYxRdT1qL+HcokhbXQdPGrS1NbtDoYBvDUcXgjWdvSDOnGkjBrpuPAry pd2QczxWguowsDsmXxkUuHNO4wyPeL9FHwbYAjpsXqjhf6xRTIOb1eKjlr3rsLhJNFC2 bLUuV3WOjvcsOBUC+BYa/eVcg+nrWxfVAHUL3byVr5U2hJ+mPfxHw7hnVvTXgx8XrH7T ziFPKdW+9jRTT0NMaqCTZGdUowspdEWQx9JresLirlNaZLelUwes3ahiGD+nQJgpiLjv NUiA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1765465472; x=1766070272; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-gg:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=QGiOaxxeedYSgO7KsDHaFkoLTRKOhWXoKaus+BgsSis=; b=sVXin+lSjUnbXFYqI6v7yBM6bxV+Q/WWayDUrRFwZof8JzHPnlWaZrJoKo0ZvagunY 9p5UTqCatS3vjIJLMEDfwiX17rXXAJbG31mLtGsTuwa9cjVeTqcZl05J4uSFgf8kXEzp PuSzkZqqiZn1prVevc9MuOsjrDilVcB0kzYLOf79WzQmirkKN6/Jkc1agl81rhIxHIsn 4WsaZKEHvOzsQRA7EdOO3hGvL/kCtsByC2wbzEd+clgX3wEdPJ2cN9Du375E+J5LTn/a VR8pvbUwWQwaWMY/iVr40nPmHLxcF53FJdNYDRlt/3+VcCLFTE5KCuxFxezW/tVEM8R3 acgA== X-Forwarded-Encrypted: i=1; AJvYcCVsRWOt1cIfFatE4+oxBHEjx49YeMZCUE53wneIO0mqNGzu7WfQg7kr4h05A9GHRa37l9EuWWDcQsFTiaM=@vger.kernel.org X-Gm-Message-State: AOJu0YzVIj1dlIhT9Nm0yhlheBlDKVVJl0oXHESWEs6J8fCqSHs0vG97 /xOlyppa3jQJo5WqFKklGAXBKc9+4j5jgspENrds+7bmj8j4k16VHn0D X-Gm-Gg: AY/fxX5ur86wlElmr1IcDQshOETcUqxQYwr0zhQAUIDuWBLdQGHGGCs99diqv+nJ9ry lJCjIZGdnAejOp+VpkP/Iuq5h3ifNy+NuQng+tNSzfuP6rIh9SDt7oEjCzyvRy1mX8iD02Ckapz 0QNhV68NPaIuyT7LDmcRNskUUFSHBc2dbbqjU+i+L8WWf6tjMIY4gmKwBNeLnQDjnR5RZvX0/u2 5IWExQmkaYP835bWL/OwySLOKKFYiH53aP88xW6UZ1TR/FYUCxB9lsPIp5TVCTWgrhF925pv6qr lgnxsBxdKJWTrJbjMldZbpsCWI1r8hl7WMJFNu/8cq0O38ynXN3ooOhlneuRYYPy+JohojUMNef 01qRPaJpZs9tGxwHpk8BBFN/EL8xPVZPF5FrlTCcm8jAaiy5TzrJj62E+he9yMBZkUfi87GsKzb 77PG5gMEGITBiSxz0wfGTxP0v2LTVk0mDdTIDIMN7N7BinwmakDv1DdMsyYK+sXg== X-Google-Smtp-Source: AGHT+IEDIDXduddojC2oF/NtI7nTuLQKy8+7uwAI70KDBtBUQRB3kFaFndB46SIt3slBE1QS+pBD5w== X-Received: by 2002:a05:6a20:158b:b0:352:3695:fa64 with SMTP id adf61e73a8af0-366e2450a28mr5947102637.37.1765465471013; Thu, 11 Dec 2025 07:04:31 -0800 (PST) Received: from ?IPV6:2001:ee0:4f4c:210:6f73:e1bc:9239:c004? ([2001:ee0:4f4c:210:6f73:e1bc:9239:c004]) by smtp.gmail.com with ESMTPSA id 41be03b00d2f7-c0c2b8e0fb4sm2619107a12.25.2025.12.11.07.04.25 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 11 Dec 2025 07:04:30 -0800 (PST) Message-ID: <6281cd92-10aa-4182-a456-81538cff822a@gmail.com> Date: Thu, 11 Dec 2025 22:04:23 +0700 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH net] virtio-net: enable all napis before scheduling refill work To: Jason Wang Cc: netdev@vger.kernel.org, "Michael S. Tsirkin" , Xuan Zhuo , =?UTF-8?Q?Eugenio_P=C3=A9rez?= , Andrew Lunn , "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Alexei Starovoitov , Daniel Borkmann , Jesper Dangaard Brouer , John Fastabend , Stanislav Fomichev , virtualization@lists.linux.dev, linux-kernel@vger.kernel.org, bpf@vger.kernel.org References: <20251208153419.18196-1-minhquangbui99@gmail.com> <66d9f44c-295e-4b62-86ae-a0aff5f062bb@gmail.com> Content-Language: en-US From: Bui Quang Minh In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit On 12/11/25 14:27, Jason Wang wrote: > On Wed, Dec 10, 2025 at 11:33 PM Bui Quang Minh > wrote: >> On 12/10/25 12:45, Jason Wang wrote: >>> On Tue, Dec 9, 2025 at 11:23 PM Bui Quang Minh wrote: >>>> On 12/9/25 11:30, Jason Wang wrote: >>>>> On Mon, Dec 8, 2025 at 11:35 PM Bui Quang Minh wrote: >>>>>> Calling napi_disable() on an already disabled napi can cause the >>>>>> deadlock. In commit 4bc12818b363 ("virtio-net: disable delayed refill >>>>>> when pausing rx"), to avoid the deadlock, when pausing the RX in >>>>>> virtnet_rx_pause[_all](), we disable and cancel the delayed refill work. >>>>>> However, in the virtnet_rx_resume_all(), we enable the delayed refill >>>>>> work too early before enabling all the receive queue napis. >>>>>> >>>>>> The deadlock can be reproduced by running >>>>>> selftests/drivers/net/hw/xsk_reconfig.py with multiqueue virtio-net >>>>>> device and inserting a cond_resched() inside the for loop in >>>>>> virtnet_rx_resume_all() to increase the success rate. Because the worker >>>>>> processing the delayed refilled work runs on the same CPU as >>>>>> virtnet_rx_resume_all(), a reschedule is needed to cause the deadlock. >>>>>> In real scenario, the contention on netdev_lock can cause the >>>>>> reschedule. >>>>>> >>>>>> This fixes the deadlock by ensuring all receive queue's napis are >>>>>> enabled before we enable the delayed refill work in >>>>>> virtnet_rx_resume_all() and virtnet_open(). >>>>>> >>>>>> Fixes: 4bc12818b363 ("virtio-net: disable delayed refill when pausing rx") >>>>>> Reported-by: Paolo Abeni >>>>>> Closes: https://netdev-ctrl.bots.linux.dev/logs/vmksft/drv-hw-dbg/results/400961/3-xdp-py/stderr >>>>>> Signed-off-by: Bui Quang Minh >>>>>> --- >>>>>> drivers/net/virtio_net.c | 59 +++++++++++++++++++--------------------- >>>>>> 1 file changed, 28 insertions(+), 31 deletions(-) >>>>>> >>>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c >>>>>> index 8e04adb57f52..f2b1ea65767d 100644 >>>>>> --- a/drivers/net/virtio_net.c >>>>>> +++ b/drivers/net/virtio_net.c >>>>>> @@ -2858,6 +2858,20 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq, >>>>>> return err != -ENOMEM; >>>>>> } >>>>>> >>>>>> +static void virtnet_rx_refill_all(struct virtnet_info *vi) >>>>>> +{ >>>>>> + bool schedule_refill = false; >>>>>> + int i; >>>>>> + >>>>>> + enable_delayed_refill(vi); >>>>> This seems to be still racy? >>>>> >>>>> For example, in virtnet_open() we had: >>>>> >>>>> static int virtnet_open(struct net_device *dev) >>>>> { >>>>> struct virtnet_info *vi = netdev_priv(dev); >>>>> int i, err; >>>>> >>>>> for (i = 0; i < vi->max_queue_pairs; i++) { >>>>> err = virtnet_enable_queue_pair(vi, i); >>>>> if (err < 0) >>>>> goto err_enable_qp; >>>>> } >>>>> >>>>> virtnet_rx_refill_all(vi); >>>>> >>>>> So NAPI and refill work is enabled in this case, so the refill work >>>>> could be scheduled and run at the same time? >>>> Yes, that's what we expect. We must ensure that refill work is scheduled >>>> only when all NAPIs are enabled. The deadlock happens when refill work >>>> is scheduled but there are still disabled RX NAPIs. >>> Just to make sure we are on the same page, I meant, after refill work >>> is enabled, rq0 is NAPI is enabled, in this case the refill work could >>> be triggered by the rq0's NAPI so we may end up in the refill work >>> that it tries to disable rq1's NAPI while holding the netdev lock. >> I don't quite get your point. The current deadlock scenario is this >> >> virtnet_rx_resume_all >> napi_enable(rq0) (the rq1 napi is still disabled) >> enable_refill_work >> >> refill_work >> napi_disable(rq0) -> still okay >> napi_enable(rq0) -> still okay >> napi_disable(rq1) >> -> hold netdev_lock >> -> stuck inside the while loop in napi_disable_locked >> while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) { >> usleep_range(20, 200); >> val = READ_ONCE(n->state); >> } >> >> >> napi_enable(rq1) >> -> stuck while trying to acquire the netdev_lock >> >> The problem is that we must not call napi_disable() on an already >> disabled NAPI (rq1's NAPI in the example). >> >> In the new virtnet_open >> >> static int virtnet_open(struct net_device *dev) >> { >> struct virtnet_info *vi = netdev_priv(dev); >> int i, err; >> >> // Note that at this point, refill work is still disabled, vi->refill_enabled == false, >> // so even if virtnet_receive is called, the refill_work will not be scheduled. >> for (i = 0; i < vi->max_queue_pairs; i++) { >> err = virtnet_enable_queue_pair(vi, i); >> if (err < 0) >> goto err_enable_qp; >> } >> >> // Here all RX NAPIs are enabled so it's safe to enable refill work again >> virtnet_rx_refill_all(vi); >> > I meant this part: > > +static void virtnet_rx_refill_all(struct virtnet_info *vi) > +{ > + bool schedule_refill = false; > + int i; > + > + enable_delayed_refill(vi); > > refill_work could run here. I don't see how this can trigger the current deadlock race. However, I see that this code is racy, the try_fill_recv function is not safe to concurrently executed on the same receive queue. So there is a requirement that we need to call try_fill_recv before enabling napi. Is it what you mean? > > + for (i = 0; i < vi->curr_queue_pairs; i++) > + if (!try_fill_recv(vi, &vi->rq[i], GFP_KERNEL)) > + schedule_refill = true; > + > > I think it can be fixed by moving enable_delayed_refill() here. > > + if (schedule_refill) > + schedule_delayed_work(&vi->refill, 0); > +} Thanks, Quang Minh.