Date:   Tue, 29 May 2018 13:20:16 +0300
From:   Vlad Buslov <vladbu@...lanox.com>
To:     Cong Wang <xiyou.wangcong@...il.com>
Cc:     Jiri Pirko <jiri@...nulli.us>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        David Miller <davem@...emloft.net>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>, kliteyn@...lanox.com
Subject: Re: [PATCH v3 00/11] Modify action API for implementing lockless actions

On Tue 29 May 2018 at 04:26, Cong Wang <xiyou.wangcong@...il.com> wrote:
>> Currently, all netlink protocol handlers for updating rules, actions and
>> qdiscs are protected with single global rtnl lock which removes any
>> possibility for parallelism. This patch set is a first step to remove
>> rtnl lock dependency from TC rules update path.
>>
>> Recently, new rtnl registration flag RTNL_FLAG_DOIT_UNLOCKED was added.
>> Handlers registered with this flag are called without RTNL taken. End
>> goal is to have rule update handlers(RTM_NEWTFILTER, RTM_DELTFILTER,
>> etc.) to be registered with UNLOCKED flag to allow parallel execution.
>> However, there is no intention to completely remove or split rtnl lock
>> itself. This patch set addresses specific problems in action API that
>> prevents it from being executed concurrently.
>
>
> Great, your goal is much clear now! So can I expect this patchset is to
> _completely_ get rid of RTNL lock from action update paths, correct?

No, this patch set is preparation only and deals with specific issues in
act API. I guess I should specify it in cover letter. There is one more
patch set that changes individual actions and...

>
> I ask because this is your first step, RTNL is still acquired on upper layer,
> that is, filter update paths.

... several more patch sets that update cls API, including filter update path.

>
>
>>
>> As a preparation for executing TC rules update handlers without rtnl
>> lock, action API code was audited to determine areas that assume
>> external synchronization with rtnl lock and must be changed to allow
>> safe concurrent access with following results:
>>
>> 1. Action idr is already protected with spinlock. However, some code
>>    paths assume that idr state is not changes between several
>>    consecutive tcf_idr_* function calls.
>> 2. tc_action reference and bind counters are implemented as plain
>>    integers. They purpose was to allow single actions to be shared
>>    between multiple filters, not to provide means for concurrent
>>    modification.
>> 3. tc_action 'cookie' pointer field is not protected against
>>    modification.
>> 4. Action API functions, that work with set of actions, use intrusive
>>    linked list, which cannot be used concurrently without additional
>>    synchronization.
>> 5. Action API functions don't take reference to actions while using
>>    them, assuming external synchronization with rtnl lock.
>
>
> Fair enough, thanks for the details, but some high-level things are still
> missing:
>
> 1. What lock protects action updates with your patches? Since you remove
> RTNL from these paths, I assume no lock at all except the existing spinlock?
> Please state here in your cover letter.

Next patch set updates every action implementation. However, it is safe
to apply this patchset without next one because I didn't re-register any
handlers with UNLOCKED flag yet, so all rules/actions update paths are
still synchronized with RTNL.

>
>
> 2. Assume 1) is correct, how do you guarantee an action update is atomic?
> Let's say I have action foo:
>
> struct act_foo
> {
>   int a;
>   int b;
> };
>
> With RTNL:
>
> rtnl_lock();
> act_foo->a = a;
> if (a == X)
>   act_foo->b = b;
> rtnl_unlock();
>
> Without any lock (as I assumed):
>
>
> act_foo->a = a;
> // fast path now reads new ->a but old ->b
> if (act_foo->a == X)
> // Other slow path may be changing ->a too
>   act_foo->b = b;
>
> If my assumption is correct, please explain the above question in your
> cover letter, it is very important for understanding your 11 patches.
>
> If my assumption is wrong, please be specific on which lock protects
> which paths here.

Your observation, that this patch is not enough and individual actions
must be updates in order to be executed concurrently, is correct. This
issue is dealt with in next patch set. For most of actions that require
additional synchronization, I used tcf_spinlock or rcu + atomic
exchange.

>
>
> 3. How are actions like act_mirred and act_ipt updated w/o RTNL?
>
> act_mirred requires to hold a refcnt for target device:
>
>         if (dev != NULL) {
>                 if (ret != ACT_P_CREATED)
>                         dev_put(rcu_dereference_protected(m->tcfm_dev, 1));
>                 dev_hold(dev);
>                 rcu_assign_pointer(m->tcfm_dev, dev);
>                 m->tcfm_mac_header_xmit = mac_header_xmit;
>         }
>
> Without RTNL, how is dev_put()+dev_hold() be atomic in !CREATED case?

Again, next set.

>
> act_ipt calls xt_request_find_target() and xt_check_target(), I guess both
> assumes RTNL?

Do they? Internally, target list is protected with mutex.
Anyway, third patch in this series adds 'rtnl_held' argument to action
init function, that allows actions to take(or release) rtnl lock when
necessary. It is intended to be used specifically in this kind of
situations when some external API requires caller to have rtnl lock. I
guess I should also add this info to cover letter. I have omitted it
because that argument is not used in this patch set.

>
> Or you just leave these exceptions as they are but make the rest actions
> lockless? If so, please list all of them here and describe why are they
> special.

All actions will be updated in next set, without exceptions.

>
>
> Last, since your end goal is to remove RTNL from filter update paths,
> how does it work if a tc filter block shared among different qdiscs?
> Assume a tc filter block can be shared by different qdiscs on different
> devs.

Basically, what I do with blocks in patch set, that changes
qdisc->block->chain->proto->filter tree to remove rtnl lock dependency,
is:
 - Change block reference counter type to refcount_t.
 - Take reference to block when using it in slow path.
 - Protect block idr in tcf_net with additional spinlock.
(Same approach that I employed in current patch set for actions and
action idr)

Anything specific that I'm missing? Could you describe potential issue
with shared blocks in more details?

>
>
> Thanks!

Powered by blists - more mailing lists