From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vlad Buslov Subject: Re: [PATCH net-next v6 10/11] net: sched: atomically check-allocate action Date: Mon, 13 Aug 2018 10:55:39 +0300 Message-ID: References: <1530800673-12280-1-git-send-email-vladbu@mellanox.com> <1530800673-12280-11-git-send-email-vladbu@mellanox.com> Mime-Version: 1.0 Content-Type: text/plain Cc: Linux Kernel Network Developers , David Miller , Jamal Hadi Salim , Jiri Pirko , Alexei Starovoitov , Daniel Borkmann , Yevgeny Kliteynik , Jiri Pirko To: Cong Wang Return-path: Received: from mail-eopbgr00041.outbound.protection.outlook.com ([40.107.0.41]:41120 "EHLO EUR02-AM5-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1726704AbeHMKhu (ORCPT ); Mon, 13 Aug 2018 06:37:50 -0400 In-reply-to: Sender: netdev-owner@vger.kernel.org List-ID: On Fri 10 Aug 2018 at 21:45, Cong Wang wrote: > On Fri, Aug 10, 2018 at 3:29 AM Vlad Buslov wrote: >> >> Approach you suggest is valid, but has its own trade-offs: >> >> - As you noted, lock granularity becomes coarse-grained due to per-netns >> scope. > > Sure, you acquire idrinfo->lock too, the only difference is how long > you take it. > > The bottleneck of your approach is the same, also you take idrinfo->lock > twice, so the contention is heavier. > > >> >> - I am not sure it is possible to call idr_replace() without obtaining >> idrinfo->lock in this particular case. Concurrent delete of action with >> same id is possible and, according to idr_replace() description, >> unlocked execution is not supported for such use-case: > > But we can hold its refcnt before releasing idrinfo->lock, so > idr_replace() can't race with concurrent delete. Yes, for concurrent delete case I agree. Action is removed from idr only when last reference is released and, in case of existing action update, init holds a reference. What about case when multiple task race to update the same existing action? I assume idr_replace() can be used for such case, but what would be the algorithm in case init replaced some other action, and not the action it actually copied before calling idr_replace()? > > >> >> - High rate or replace request will generate a lot of unnecessary memory >> allocations and deallocations. >> > > Yes, this is literally how RCU works, always allocate and copy, > release upon error. > > Also, if this is really a problem, we have SLAB_TYPESAFE_BY_RCU > too. ;) Current action update implementation is in-place, so there is no "copy" stage, besides members of some actions that are RCU-pointers. But I guess it makes sense if your goal is to refactor all actions to be updated with RCU.