| Message ID | 20190227101226.26196-1-vladbu@mellanox.com |
|---|---|
| Headers | show |
| Series | Refactor flower classifier to remove dependency on rtnl lock | expand |
Vlad, I'm unfortunately gonna defer this to the next merge window. But this will give time for more review and analysis of these changes, which are rather non-trivial in my opinion and thus deserve it. Thanks.
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>
On Tue 05 Mar 2019 at 01:57, Stefano Brivio <sbrivio@redhat.com> wrote: > 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> Thanks for reviewing this! Regards, Vlad
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. Github: [https://github.com/vbuslov/linux/tree/unlocked_flower_cong2] Vlad Buslov (12): net: sched: flower: don't check for rtnl on head dereference net: sched: flower: refactor fl_change net: sched: flower: introduce reference counting for filters net: sched: flower: track filter deletion with flag net: sched: flower: add reference counter to flower mask net: sched: flower: handle concurrent mask insertion net: sched: flower: protect masks list with spinlock net: sched: flower: handle concurrent filter insertion in fl_change net: sched: flower: handle concurrent tcf proto deletion net: sched: flower: protect flower classifier state with spinlock net: sched: flower: track rtnl lock state net: sched: flower: set unlocked flag for flower proto ops net/sched/cls_flower.c | 440 ++++++++++++++++++++++++++++++++++++------------- 1 file changed, 329 insertions(+), 111 deletions(-)