From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Hemminger Subject: Re: [PATCH] net: Make synchronize_net() be expedited only when it's really need Date: Tue, 23 Jan 2018 08:26:38 -0800 Message-ID: <20180123082638.0a4fdae9@xeon-e3> References: <151661402727.29441.3116565394148374460.stgit@localhost.localdomain> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: edumazet@google.com, netdev@vger.kernel.org, davem@davemloft.net, dsahern@gmail.com, fw@strlen.de, lucien.xin@gmail.com, daniel@iogearbox.net, mschiffer@universe-factory.net, jakub.kicinski@netronome.com, vyasevich@gmail.com, jbenc@redhat.com To: Kirill Tkhai Return-path: Received: from mail-pg0-f66.google.com ([74.125.83.66]:38184 "EHLO mail-pg0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751234AbeAWQ0l (ORCPT ); Tue, 23 Jan 2018 11:26:41 -0500 Received: by mail-pg0-f66.google.com with SMTP id y27so622667pgc.5 for ; Tue, 23 Jan 2018 08:26:40 -0800 (PST) In-Reply-To: <151661402727.29441.3116565394148374460.stgit@localhost.localdomain> Sender: netdev-owner@vger.kernel.org List-ID: On Mon, 22 Jan 2018 12:41:41 +0300 Kirill Tkhai wrote: > Commit be3fc413da9e "net: use synchronize_rcu_expedited()" introducing > synchronize_net() says: > > >When we hold RTNL mutex, we would like to spend some cpu cycles but not > >block too long other processes waiting for this mutex. > >We also want to setup/dismantle network features as fast as possible at > >boot/shutdown time. > >This patch makes synchronize_net() call the expedited version if RTNL is > >locked. > > At the time of the commit (May 23 2011) there was no possible to differ, > who is the actual owner of the mutex. Only the fact that it's locked > by someone at the moment. So (I guess) this is the only reason the generic > primitive mutex_is_locked() was used. > > But now mutex owner is available outside the locking subsystem and > __mutex_owner() may be used instead (there is an example in audit_log_start()). > So, let's make expensive synchronize_rcu_expedited() be used only > when a caller really owns rtnl_mutex(). > > There are several possibilities to fix that. The first one is > to fix synchronize_net(), the second is to change rtnl_is_locked(). > > I prefer the second, as it seems it's more intuitive for people > to think that rtnl_is_locked() is about current process, not > about the fact mutex is locked in general. Grep over kernel > sources just proves this fact: > > drivers/staging/rtl8723bs/os_dep/osdep_service.c:297 > drivers/staging/rtl8723bs/os_dep/osdep_service.c:316 > > if (!rtnl_is_locked()) > ret = register_netdev(pnetdev); > else > ret = register_netdevice(pnetdev); Conditional locking like this especially in the registration path is wrong and broken. The driver should just be removed for this. > drivers/staging/wilc1000/linux_mon.c:310 > > if (rtnl_is_locked()) { > rtnl_unlock(); > rollback_lock = true; > } This driver like most of staging is sh*t. > Side effect of this patch is three BUGs in above examples > become fixed. > > Signed-off-by: Kirill Tkhai > --- > net/core/rtnetlink.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c > index 16d644a4f974..a5ddf373ffa9 100644 > --- a/net/core/rtnetlink.c > +++ b/net/core/rtnetlink.c > @@ -117,7 +117,7 @@ EXPORT_SYMBOL(rtnl_trylock); > > int rtnl_is_locked(void) > { > - return mutex_is_locked(&rtnl_mutex); > + return __mutex_owner(&rtnl_mutex) == current; > } > EXPORT_SYMBOL(rtnl_is_locked); No. You are breaking the concept of is_locked. It means the mutex is locked anywhere, not just in the calling process. For example, rtnl_is_locked is used to break AB BA lock deadlock in sysfs.