From mboxrd@z Thu Jan 1 00:00:00 1970 From: Florian Westphal Subject: Re: [PATCH net-next 2/8] rtnetlink: add rtnl_register_module Date: Tue, 7 Nov 2017 07:11:56 +0100 Message-ID: <20171107061156.GK9424@breakpoint.cc> References: <20171106105113.20476-1-fw@strlen.de> <20171106105113.20476-3-fw@strlen.de> <20171106124454.GI3165@worktop.lehotels.local> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Florian Westphal , netdev@vger.kernel.org To: Peter Zijlstra Return-path: Received: from Chamillionaire.breakpoint.cc ([146.0.238.67]:49082 "EHLO Chamillionaire.breakpoint.cc" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750913AbdKGGMc (ORCPT ); Tue, 7 Nov 2017 01:12:32 -0500 Content-Disposition: inline In-Reply-To: <20171106124454.GI3165@worktop.lehotels.local> Sender: netdev-owner@vger.kernel.org List-ID: Peter Zijlstra wrote: > On Mon, Nov 06, 2017 at 11:51:07AM +0100, Florian Westphal wrote: > > @@ -180,6 +164,12 @@ int __rtnl_register(int protocol, int msgtype, > > rcu_assign_pointer(rtnl_msg_handlers[protocol], tab); > > } > > > > + WARN_ON(tab[msgindex].owner && tab[msgindex].owner != owner); > > + > > + tab[msgindex].owner = owner; > > + /* make sure owner is always visible first */ > > + smp_wmb(); > > + > > if (doit) > > tab[msgindex].doit = doit; > > if (dumpit) > > > @@ -235,6 +279,9 @@ int rtnl_unregister(int protocol, int msgtype) > > handlers[msgindex].doit = NULL; > > handlers[msgindex].dumpit = NULL; > > handlers[msgindex].flags = 0; > > + /* make sure we clear owner last */ > > + smp_wmb(); > > + handlers[msgindex].owner = NULL; > > rtnl_unlock(); > > > > return 0; > > These wmb()'s don't make sense; and the comments are incomplete. What do > they pair with? Who cares about this ordering? rtnetlink_rcv_msg: 4406 dumpit = READ_ONCE(handlers[type].dumpit); 4407 if (!dumpit) 4408 goto err_unlock; 4409 owner = READ_ONCE(handlers[type].owner); 4410 } .. 4417 if (!try_module_get(owner)) 4418 err = -EPROTONOSUPPORT; 4419 I don't want dumpit function address to be visible before owner. Does that make sense?