From mboxrd@z Thu Jan 1 00:00:00 1970 From: Santosh Shilimkar Subject: Re: [Patch net] rds: mark bound socket with SOCK_RCU_FREE Date: Mon, 10 Sep 2018 17:26:35 -0700 Message-ID: <17b2b2b8-12ce-076e-a961-2a8bee1d021f@oracle.com> References: <20180910222422.19470-1-xiyou.wangcong@gmail.com> <20180910233007.GJ4668@oracle.com> <20180911000437.GK4668@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: Linux Kernel Network Developers , rds-devel@oss.oracle.com To: Cong Wang , Sowmini Varadhan Return-path: Received: from userp2130.oracle.com ([156.151.31.86]:49894 "EHLO userp2130.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725945AbeIKFXN (ORCPT ); Tue, 11 Sep 2018 01:23:13 -0400 In-Reply-To: Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 9/10/2018 5:16 PM, Cong Wang wrote: > On Mon, Sep 10, 2018 at 5:04 PM Sowmini Varadhan > wrote: >> >> On (09/10/18 16:51), Cong Wang wrote: >>> >>> __rds_create_bind_key(key, addr, port, scope_id); >>> - rs = rhashtable_lookup_fast(&bind_hash_table, key, ht_parms); >>> + rcu_read_lock(); >>> + rs = rhashtable_lookup(&bind_hash_table, key, ht_parms); >>> if (rs && !sock_flag(rds_rs_to_sk(rs), SOCK_DEAD)) >>> rds_sock_addref(rs); >>> else >>> rs = NULL; >>> + rcu_read_unlock(); >> >> aiui, the rcu_read lock/unlock is only useful if the write >> side doing destructive operations does something to make sure readers >> are done before doing the destructive opertion. AFAIK, that does >> not exist for rds socket management today > > That is exactly why we need it here, right? > > As you mentioned previously, the sock could be freed after > rhashtable_lookup_fast() but before rds_sock_addref(), extending > the RCU read section after rds_sock_addref() exactly solves > the problem here. > Thats the case really. > Or is there any other destructive problem you didn't mention? > Mind to be specific? > > >> >> >>> Although sock release path is not a very hot path, but blocking >>> it isn't a good idea either, especially when you can use call_rcu(), >>> which has the same effect. >>> >>> I don't see any reason we should prefer synchronize_rcu() here. >> >> Usually correctness (making sure all readers are done, before nuking a >> data structure) is a little bit more important than perforamance, aka >> "safety before speed" is what I've always been taught. >> > > Hmm, so you are saying synchronize_rcu() is kinda more correct > than call_rcu()?? I never hear this before, would like to know why. > > To my best knowledge, the only difference between them is the context, > one is blocking, the other is non-blocking. Their correctness must be > equivalent. > We have burn our hands with blocking synchronize_rcu() and that was actually the main reason we moved to rw locks from rcu to localise the cost. call_rcu()should be just fine as long as it plugs the hole. I don't want to add blocking in this path since this slows down the connection shutdown path and at times lead to soft dumps. Would you mind posting an updated patch please with call_rcu and above extended RCU grace period with rcu_read_lock. Thanks !! Regards, Santosh Regards, Santosh