Re: [PATCH net-next v6 10/11] net: sched: atomically check-allocate action

[Vlad Buslov <vladbu@mellanox.com>]

On Fri 10 Aug 2018 at 21:45, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Fri, Aug 10, 2018 at 3:29 AM Vlad Buslov <vladbu@mellanox.com> 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.