From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jason Wang Subject: Re: [PATCH] virtio_net: add local_bh_disable() around u64_stats_update_begin Date: Fri, 19 Oct 2018 10:19:56 +0800 Message-ID: <0c7c4cb9-5814-e8b1-33e5-b57aa26f0cce@redhat.com> References: <20181016184206.coukhtgmlr32hyl7@linutronix.de> <20181016114414.23ea73c3@xeon-e3> <20181018084313.oopu34iwfwgkcwwc@linutronix.de> <20181018.162308.2295937118791060714.davem@davemloft.net> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Cc: stephen@networkplumber.org, netdev@vger.kernel.org, virtualization@lists.linux-foundation.org, tglx@linutronix.de, makita.toshiaki@lab.ntt.co.jp, mst@redhat.com To: David Miller , bigeasy@linutronix.de Return-path: Received: from mx1.redhat.com ([209.132.183.28]:33166 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726463AbeJSKYC (ORCPT ); Fri, 19 Oct 2018 06:24:02 -0400 In-Reply-To: <20181018.162308.2295937118791060714.davem@davemloft.net> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 2018/10/19 上午7:23, David Miller wrote: > From: Sebastian Andrzej Siewior > Date: Thu, 18 Oct 2018 10:43:13 +0200 > >> on 32bit, lockdep notices that virtnet_open() and refill_work() invoke >> try_fill_recv() from process context while virtnet_receive() invokes the >> same function from BH context. The problem that the seqcounter within >> u64_stats_update_begin() may deadlock if it is interrupted by BH and >> then acquired again. >> >> Introduce u64_stats_update_begin_bh() which disables BH on 32bit >> architectures. Since the BH might interrupt the worker, this new >> function should not limited to SMP like the others which are expected >> to be used in softirq. >> >> With this change we might lose increments but this is okay. The >> important part that the two 32bit parts of the 64bit counter are not >> corrupted. >> >> Fixes: 461f03dc99cf6 ("virtio_net: Add kick stats"). >> Suggested-by: Stephen Hemminger >> Signed-off-by: Sebastian Andrzej Siewior > Trying to get down to the bottom of this: > > 1) virtnet_receive() runs from softirq but only if NAPI is active and > enabled. It is in this context that it invokes try_fill_recv(). > > 2) refill_work() runs from process context, but disables NAPI (and > thus invocation of virtnet_receive()) before calling > try_fill_recv(). > > 3) virtnet_open() invokes from process context as well, but before the > NAPI instances are enabled, it is same as case #2. > > 4) virtnet_restore_up() is the same situations as #3. > > Therefore I agree that this is a false positive, and simply lockdep > cannot see the NAPI synchronization done by case #2. > > I think we shouldn't add unnecessary BH disabling here, and instead > find some way to annotate this for lockdep's sake. > > Thank you. > +1 Thanks