netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Stefano Brivio <sbrivio@redhat.com>
To: Vlad Buslov <vladbu@mellanox.com>
Cc: netdev@vger.kernel.org, jhs@mojatatu.com,
	xiyou.wangcong@gmail.com, jiri@resnulli.us, davem@davemloft.net
Subject: Re: [PATCH net-next v2 00/12] Refactor flower classifier to remove dependency on rtnl lock
Date: Tue, 5 Mar 2019 00:57:11 +0100	[thread overview]
Message-ID: <20190305005711.0dba1b69@redhat.com> (raw)
In-Reply-To: <20190227101226.26196-1-vladbu@mellanox.com>

On Wed, 27 Feb 2019 12:12:14 +0200
Vlad Buslov <vladbu@mellanox.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.
> TC rule update handlers (RTM_NEWTFILTER, RTM_DELTFILTER, etc.) are
> already registered with this flag and only take rtnl lock when qdisc or
> classifier requires it. Classifiers can indicate that their ops
> callbacks don't require caller to hold rtnl lock by setting the
> TCF_PROTO_OPS_DOIT_UNLOCKED flag. The goal of this change is to refactor
> flower classifier to support unlocked execution and register it with
> unlocked flag.
> 
> This patch set implements following changes to make flower classifier
> concurrency-safe:
> 
> - Implement reference counting for individual filters. Change fl_get to
>   take reference to filter. Implement tp->ops->put callback that was
>   introduced in cls API patch set to release reference to flower filter.
> 
> - Use tp->lock spinlock to protect internal classifier data structures
>   from concurrent modification.
> 
> - Handle concurrent tcf proto deletion by returning EAGAIN, which will
>   cause cls API to retry and create new proto instance or return error
>   to the user (depending on message type).
> 
> - Handle concurrent insertion of filter with same priority and handle by
>   returning EAGAIN, which will cause cls API to lookup filter again and
>   process it accordingly to netlink message flags.
> 
> - Extend flower mask with reference counting and protect masks list with
>   masks_lock spinlock.
> 
> - Prevent concurrent mask insertion by inserting temporary value to
>   masks hash table. This is necessary because mask initialization is a
>   sleeping operation and cannot be done while holding tp->lock.
> 
> Both chain level and classifier level conflicts are resolved by
> returning -EAGAIN to cls API that results restart of whole operation.
> This retry mechanism is a result of fine-grained locking approach used
> in this and previous changes in series and is necessary to allow
> concurrent updates on same chain instance. Alternative approach would be
> to lock the whole chain while updating filters on any of child tp's,
> adding and removing classifier instances from the chain. However, since
> most CPU-intensive parts of filter update code are specifically in
> classifier code and its dependencies (extensions and hw offloads), such
> approach would negate most of the gains introduced by this change and
> previous changes in the series when updating same chain instance.
> 
> Tcf hw offloads API is not changed by this patch set and still requires
> caller to hold rtnl lock. Refactored flower classifier tracks rtnl lock
> state by means of 'rtnl_held' flag provided by cls API and obtains the
> lock before calling hw offloads. Following patch set will lift this
> restriction and refactor cls hw offloads API to support unlocked
> execution.
> 
> With these changes flower classifier is safely registered with
> TCF_PROTO_OPS_DOIT_UNLOCKED flag in last patch.
> 
> Changes from V1 to V2:
> - Extend cover letter with explanation about retry mechanism.
> - Rebase on current net-next.
> - Patch 1:
>   - Use rcu_dereference_raw() for tp->root dereference.
>   - Update comment in fl_head_dereference().
> - Patch 2:
>   - Remove redundant check in fl_change error handling code.
>   - Add empty line between error check and new handle assignment.
> - Patch 3:
>   - Refactor loop in fl_get_next_filter() to improve readability.
> - Patch 4:
>   - Refactor __fl_delete() to improve readability.
> - Patch 6:
>   - Fix comment in fl_check_assign_mask().
> - Patch 9:
>   - Extend commit message.
>   - Fix error code in comment.
> - Patch 11:
>   - Fix fl_hw_replace_filter() to always release rtnl lock in error
>     handlers.
> - Patch 12:
>   - Don't take rtnl lock before calling __fl_destroy_filter() in
>     workqueue context.
>   - Extend commit message with explanation why flower still takes rtnl
>     lock before calling hardware offloads API.

FWIW,

Reviewed-by: Stefano Brivio <sbrivio@redhat.com>

-- 
Stefano

  parent reply	other threads:[~2019-03-04 23:57 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-02-27 10:12 [PATCH net-next v2 00/12] Refactor flower classifier to remove dependency on rtnl lock Vlad Buslov
2019-02-27 10:12 ` [PATCH net-next v2 01/12] net: sched: flower: don't check for rtnl on head dereference Vlad Buslov
2019-02-27 10:12 ` [PATCH net-next v2 02/12] net: sched: flower: refactor fl_change Vlad Buslov
2019-02-27 10:12 ` [PATCH net-next v2 03/12] net: sched: flower: introduce reference counting for filters Vlad Buslov
2019-02-27 10:12 ` [PATCH net-next v2 04/12] net: sched: flower: track filter deletion with flag Vlad Buslov
2019-03-01 23:51   ` Stefano Brivio
2019-03-04 14:24     ` Vlad Buslov
2019-03-04 23:56       ` Stefano Brivio
2019-02-27 10:12 ` [PATCH net-next v2 05/12] net: sched: flower: add reference counter to flower mask Vlad Buslov
2019-02-27 10:12 ` [PATCH net-next v2 06/12] net: sched: flower: handle concurrent mask insertion Vlad Buslov
2019-02-27 10:12 ` [PATCH net-next v2 07/12] net: sched: flower: protect masks list with spinlock Vlad Buslov
2019-02-27 10:12 ` [PATCH net-next v2 08/12] net: sched: flower: handle concurrent filter insertion in fl_change Vlad Buslov
2019-02-27 10:12 ` [PATCH net-next v2 09/12] net: sched: flower: handle concurrent tcf proto deletion Vlad Buslov
2019-02-27 10:12 ` [PATCH net-next v2 10/12] net: sched: flower: protect flower classifier state with spinlock Vlad Buslov
2019-02-27 10:12 ` [PATCH net-next v2 11/12] net: sched: flower: track rtnl lock state Vlad Buslov
2019-02-27 10:12 ` [PATCH net-next v2 12/12] net: sched: flower: set unlocked flag for flower proto ops Vlad Buslov
2019-03-04 18:28 ` [PATCH net-next v2 00/12] Refactor flower classifier to remove dependency on rtnl lock David Miller
2019-03-04 23:57 ` Stefano Brivio [this message]
2019-03-06 11:52   ` Vlad Buslov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190305005711.0dba1b69@redhat.com \
    --to=sbrivio@redhat.com \
    --cc=davem@davemloft.net \
    --cc=jhs@mojatatu.com \
    --cc=jiri@resnulli.us \
    --cc=netdev@vger.kernel.org \
    --cc=vladbu@mellanox.com \
    --cc=xiyou.wangcong@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).