From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kirill Tkhai Subject: Re: [PATCH] net: Make synchronize_net() be expedited only when it's really need Date: Wed, 24 Jan 2018 05:16:33 +0300 Message-ID: References: <151661402727.29441.3116565394148374460.stgit@localhost.localdomain> <20180123082638.0a4fdae9@xeon-e3> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 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: Stephen Hemminger Return-path: Received: from mail-eopbgr40106.outbound.protection.outlook.com ([40.107.4.106]:60543 "EHLO EUR03-DB5-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752079AbeAXCQs (ORCPT ); Tue, 23 Jan 2018 21:16:48 -0500 In-Reply-To: <20180123082638.0a4fdae9@xeon-e3> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 23.01.2018 19:26, Stephen Hemminger wrote: > 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. If it's so, could you please point the place, where rtnl_is_locked() actually breaks the AB BA deadlock you referred? Thanks, Kirill