From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 3C37A401494 for ; Wed, 13 May 2026 12:10:59 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=170.10.133.124 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1778674260; cv=none; b=ilXoFz6BX1SuSlgMd1kBef3udL/9ZzdRPavOq6KduZ8UWSrtdkcMKppi+u39iRQsq4BwS9kkVvng/wPQhgdQvXgzbKy7WJsZGkr4gK8EqXMFLRR73c6TmsV2UgGj5sVAe0Ku7M++pCbuPkdJ3QC5UX2oBasYvz2UUoVtXI0cuF4= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1778674260; c=relaxed/simple; bh=qag4HZBEbOWMfveLQJydGBeg4Xk2bclkDFncWGVkdvY=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=vAIfFUkZw+jZJ850vP8oIHf8MDzfU6sxVbvF2NqOULj8T2bI5OEIm3RsihMRtG8PyDeFHlIqXHMKPvCUiX4rjM12jAa9fc+3VQDVFq9fUv2vdS3cigPQafcc/HAmIsiNSaXEuRSf/WDtPZQm8jk13PYD1+tfCxqaotEPEaqZj4o= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com; spf=pass smtp.mailfrom=redhat.com; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b=A6MG0Moc; dkim=pass (2048-bit key) header.d=redhat.com header.i=@redhat.com header.b=oe+9Tev7; arc=none smtp.client-ip=170.10.133.124 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="A6MG0Moc"; dkim=pass (2048-bit key) header.d=redhat.com header.i=@redhat.com header.b="oe+9Tev7" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1778674258; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=tvNsaPUwi394bBOCZmbGOFCLqPIJOdPpc1dHADKfuI8=; b=A6MG0MocZSVwUExoUkR5FYJ1CdkALFYyVbIoVxEGPI/r6E4tjBOz5rZlkmBUeylvQ4Qcqg xFSVQTlW54A9xa5pO5kEXWj2ZIwGzuF1Xjn3n3CPH0xVYOA5/RtqTyCjJMxG3rYN5A1fcl 5ozByRvF6oinUTklvebXYqCS7e3lLPo= Received: from mail-wr1-f70.google.com (mail-wr1-f70.google.com [209.85.221.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-608-gmAG_3qtNeiNHG6Q9-f0yA-1; Wed, 13 May 2026 08:10:55 -0400 X-MC-Unique: gmAG_3qtNeiNHG6Q9-f0yA-1 X-Mimecast-MFC-AGG-ID: gmAG_3qtNeiNHG6Q9-f0yA_1778674254 Received: by mail-wr1-f70.google.com with SMTP id ffacd0b85a97d-44f56d5523eso5504977f8f.1 for ; Wed, 13 May 2026 05:10:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=google; t=1778674254; x=1779279054; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=tvNsaPUwi394bBOCZmbGOFCLqPIJOdPpc1dHADKfuI8=; b=oe+9Tev7IuBa4ON3PfN8GnlyrezRpEPJXjBIroa4GHjMCgmS/wGm9sMdJFRM6LMqBW HRIY+b+idpiEyuJSCqa6A4tZyhfkXxRSdiLFC9DD5CFQ8Yn6BORRgfwC358BbJrIO5zU OU5fiZaS2BXnNX/WFgN2YzTa3WcRomKc2XgfA1owQ9xU5+7IZI32q2e/qqoes+MdL3FN zCBCbvQ87Mzse6DKg+TeOtyucC+YJlGs/INNJt4IwiUnGeGumgbajdUsvvfgtrt3Ol8e j9coCHhZ/XOmuSWQPh5FBPILRBQXbOsSrvI7ZGlvIfu9STZw1SBGgu6VUaFrlB5M4n4b fDyw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1778674254; x=1779279054; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-gg:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=tvNsaPUwi394bBOCZmbGOFCLqPIJOdPpc1dHADKfuI8=; b=B6U7SzSFqh+6+nwG7ePNnmdXITPZl2NS7FNgeks5zimmNNnCwmzxKFlKU8a5lFKFbk yGnaTuNCttuZ+IPX7T6XRjYGYnrpOaxnhp/bSFDN7RdcwUZU6wWNaiwAP8CN2m7A1b5E LNtS5yjTOY54wMKdnXM1t1wQIY4X3aSNC6qfHD7EfeTUBdBhsuUerjQBEU+RegKJ27wq DVeRW/fekoMhmEOFOdmSFn0vtpy3FMj4zNEaYtyrhbdDDS+eq93LAnKtVqtEgVgt9xyJ xIgHPJ59BQtphlW0quXWjPlojKunUnONwFCWSpWXGhVEEFOwyfwnd0cFljRX24P2Qmr3 yZ4Q== X-Forwarded-Encrypted: i=1; AFNElJ+gvre25SW1fehU7XIF8u3jF9AR9eAu/RJUfS87jcoFiDJWYWqWZ5K1dr3VSNkHQ6EIRIaW3BJLz6ggSlo=@vger.kernel.org X-Gm-Message-State: AOJu0YywqyNDkCcg+OpKuWL2bD27SBoGXHulFYRyv609RpBLYGoOESes Cr3VEDK0F+h7mm9Ua06R43HxDzPTb/WEMtaLp/WkDVIqRwokFMG+S3V5aNWpL3BWP2frkafK3EZ x9k+m88LpvCMgZg/k5VdGOeOhxijsvC7cLG9V4fi3C3JfNvGtvU1d+FDSOn/zA8tpzQ== X-Gm-Gg: Acq92OGmut5rU5BeDxLQcYvOJ4Gi7T2eH5ocUctBTiLAmmNgtEfKyXt5st1ZnZNXdC8 xwqLClzLgUWh2CcVCJMgrl0BN9puxf+EDfsW1kN1+M1BRbJVvOTf4nTBBaEjKnRZN9VW6eGmKHO EB1Zz9gjZDdPi9YR5Bdj76bvxNYNgsN1Wzy4ELiFhtFCQKakFuestcH7363w56ZcSmAxRnqWj6b PX14x7V6KGLF3R2cPagC3vazZ0N62ssRX/zLHVQ7HyoyFytOqJ9DqMhwHf1YEqO6mPvV/yVEz2t IZ1Vg4Lv5u8yDHDabEmEhumYVlneji9/ir7o9JIl8CM1E+hnPexbdxn+5ChuzZqdSYuS62ORvEl WL57CjnlaD+YnGhekxgGhgepnqpf3aTm7eiQ0zebx X-Received: by 2002:a05:6000:1847:b0:455:4288:6c34 with SMTP id ffacd0b85a97d-45c5a0b17bfmr4742593f8f.24.1778674253702; Wed, 13 May 2026 05:10:53 -0700 (PDT) X-Received: by 2002:a05:6000:1847:b0:455:4288:6c34 with SMTP id ffacd0b85a97d-45c5a0b17bfmr4742485f8f.24.1778674252920; Wed, 13 May 2026 05:10:52 -0700 (PDT) Received: from redhat.com (IGLD-80-230-48-7.inter.net.il. [80.230.48.7]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-45cb1489594sm3997592f8f.5.2026.05.13.05.10.51 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 13 May 2026 05:10:52 -0700 (PDT) Date: Wed, 13 May 2026 08:10:49 -0400 From: "Michael S. Tsirkin" To: Jinhui Guo Cc: Eugenio =?iso-8859-1?Q?P=E9rez?= , Jason Wang , Jiri Pirko , Xuan Zhuo , linux-kernel@vger.kernel.org, stable@vger.kernel.org, virtualization@lists.linux.dev Subject: Re: [PATCH] virtio_pci_modern: Use GFP_ATOMIC with spin_lock_irqsave held in virtqueue_exec_admin_cmd() Message-ID: <20260513080456-mutt-send-email-mst@kernel.org> References: <20260413034046-mutt-send-email-mst@kernel.org> <20260413100013.32399-1-guojinhui.liam@bytedance.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260413100013.32399-1-guojinhui.liam@bytedance.com> On Mon, Apr 13, 2026 at 06:00:13PM +0800, Jinhui Guo wrote: > On Mon, Apr 13, 2026 at 03:45:20 -0400, "Michael S. Tsirkin" wrote: > > GFP_ATOMIC allocations can and will fail. If using them, one must > > retry, not just propagate failures. > > Or just switch admin_vq->lock to a mutex? > > Hi Michael, > > Thank you for the review. > > Regarding the suggestion to switch admin_vq->lock to a mutex: > > The virtqueue callback vp_modern_avq_done() holds admin_vq->lock and > runs in an interrupt handler context, making it impractical to replace > the spinlock with a mutex directly. > > I considered deferring the completion to a workqueue so we could safely > use a mutex, but since this is a bug fix destined for stable@vger.kernel.org, > doing so would introduce significant code churn (e.g., handling INIT_WORK, > cancel_work_sync during cleanup, etc.) and increase the risk for backports. This is not how we do kernel development here. Please fix the bug upstream first then we will consider backporting strategies. > Therefore, using GFP_ATOMIC with the existing spinlock seems to be the most > minimal and safest approach for a fix. > > However, just replacing GFP_KERNEL with GFP_ATOMIC isn't entirely safe > because of how virtqueue_add_sgs() handles allocation failures. If kmalloc() > fails under memory pressure with GFP_ATOMIC, the function falls back to using > direct descriptors. If there are not enough free direct descriptors, it > ultimately returns -ENOSPC. > > In the current code, -ENOSPC is handled with a busy loop: > > if (ret == -ENOSPC) { > spin_unlock_irqrestore(&admin_vq->lock, flags); > cpu_relax(); > goto again; > } > > If the -ENOSPC is actually caused by a GFP_ATOMIC allocation failure under > memory pressure, this cpu_relax() loop will never yield the CPU to memory > reclaim mechanisms (like kswapd), potentially leading to a soft lockup. > > To properly handle both actual queue-full conditions and GFP_ATOMIC failures, > I propose replacing cpu_relax() with a sleep (e.g., usleep_range(10, 100)). > This allows memory reclaim to run while we wait. > > I plan to send out a v2 patch with this modification: > > --- a/drivers/virtio/virtio_pci_modern.c > +++ b/drivers/virtio/virtio_pci_modern.c > @@ -101,11 +101,11 @@ static int virtqueue_exec_admin_cmd(struct virtio_pci_admin_vq *admin_vq, > return -EIO; > > spin_lock_irqsave(&admin_vq->lock, flags); > - ret = virtqueue_add_sgs(vq, sgs, out_num, in_num, cmd, GFP_KERNEL); > + ret = virtqueue_add_sgs(vq, sgs, out_num, in_num, cmd, GFP_ATOMIC); > if (ret < 0) { > if (ret == -ENOSPC) { > spin_unlock_irqrestore(&admin_vq->lock, flags); > - cpu_relax(); > + usleep_range(10, 100); > goto again; > } > goto unlock_err; > > Does this approach align with your expectations for the fix? > > Thanks, > Jinhui Nope. I think we need to get out of peephole mode where we are just looking at the warnings and "fix" them by 1 line tweaks and actually analyze the codepaths. GFP is just for indirect allocations and VQ already falls back to using direct when that fails. The question is: - what is going on with VQ ring state, can we actually get into a situation where indirect would succeed but direct fails? - how can callers either prevent failures or get notified when buffers have been used? And it is quite possible that the fix in the end is exactly your v1 but with the analysis in the commit log explaining why this fixes the problem and does not paper over it. -- MST