On Tue 29 May 2018 at 04:26, Cong Wang [off-list ref] wrote:

quoted

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.

quoted

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!