netdev - Re: [PATCH net-next v3 00/10] Refactor classifier API to work with Qdisc/blocks without rtnl lock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAM_iQpVDUEZFJiL-CdqaiGzeBQLqU1cAy8SuS+3HKVvkCvACuQ@mail.gmail.com>
Date:   Tue, 25 Sep 2018 11:16:30 -0700
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Vlad Buslov <vladbu@...lanox.com>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Jiri Pirko <jiri@...nulli.us>,
        David Miller <davem@...emloft.net>,
        Stephen Hemminger <stephen@...workplumber.org>,
        Kirill Tkhai <ktkhai@...tuozzo.com>,
        Nicolas Dichtel <nicolas.dichtel@...nd.com>,
        Greg KH <gregkh@...uxfoundation.org>, mark.rutland@....com,
        Leon Romanovsky <leon@...nel.org>,
        "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
        Florian Westphal <fw@...len.de>,
        David Ahern <dsahern@...il.com>, christian@...uner.io,
        lucien xin <lucien.xin@...il.com>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        Jiri Benc <jbenc@...hat.com>
Subject: Re: [PATCH net-next v3 00/10] Refactor classifier API to work with
 Qdisc/blocks without rtnl lock

On Mon, Sep 24, 2018 at 9:23 AM 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 third 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 implementation of
> classifiers API that prevent its control path from being executed
> concurrently. Additional changes are required to refactor classifiers
> API and individual classifiers for parallel execution. This patch set
> lays groundwork to eventually register rule update handlers as
> rtnl-unlocked by modifying code in cls API that works with Qdiscs and
> blocks. Following patch set does the same for chains and classifiers.
>
> The goal of this change is to refactor tcf_block_find() and its
> dependencies to allow concurrent execution:
> - Extend Qdisc API with rcu to lookup and take reference to Qdisc
>   without relying on rtnl lock.
> - Extend tcf_block with atomic reference counting and rcu.
> - Always take reference to tcf_block while working with it.
> - Implement tcf_block_release() to release resources obtained by
>   tcf_block_find()
> - Create infrastructure to allow registering Qdiscs with class ops that
>   do not require the caller to hold rtnl lock.
>
> All three netlink rule update handlers use tcf_block_find() to lookup
> Qdisc and block, and this patch set introduces additional means of
> synchronization to substitute rtnl lock in cls API.
>
> Some functions in cls and sch APIs have historic names that no longer
> clearly describe their intent. In order not make this code even more
> confusing when introducing their concurrency-friendly versions, rename
> these functions to describe actual implementation.
>
> Changes from V2 to V3:
> - Patch 1:
>   - Explicitly include refcount.h in rtnetlink.h.
> - Patch 3:
>   - Move rcu_head field to the end of struct Qdisc.
>   - Rearrange local variable declarations in qdisc_lookup_rcu().
> - Patch 5:
>   - Remove tcf_qdisc_put() and inline its content to callers.


Overall looks good to me,

Acked-by: Cong Wang <xiyou.wangcong@...il.com>


There are some minor issues like qdisc_free_cb() can be static,
please send follow-up patch to address it.

Thanks.