Message-ID: <CAM_iQpVMcg3wb2MvChgTLuhEgH3zad5tJR2_FVWwuhkopABorw@mail.gmail.com>
Date:   Mon, 28 May 2018 21:26:16 -0700
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Vlad Buslov <vladbu@...lanox.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 Sun, May 27, 2018 at 2:17 PM, Vlad Buslov <vladbu@...lanox.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?

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


>
> 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.


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.


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?

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

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.


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.


Thanks!

Powered by blists - more mailing lists