From: Vlad Buslov <vladbu@mellanox•com>
To: Cong Wang <xiyou.wangcong@gmail•com>, Jiri Pirko <jiri@resnulli•us>
Cc: Linux Kernel Network Developers <netdev@vger•kernel.org>,
Jamal Hadi Salim <jhs@mojatatu•com>,
Jiri Pirko <jiri@resnulli•us>, David Miller <davem@davemloft•net>,
Alexei Starovoitov <ast@kernel•org>,
Daniel Borkmann <daniel@iogearbox•net>
Subject: Re: [PATCH net-next v2 01/17] net: sched: refactor mini_qdisc_pair_swap() to use workqueue
Date: Sun, 16 Dec 2018 16:32:13 +0000 [thread overview]
Message-ID: <vbfftuxzb3r.fsf@mellanox.com> (raw)
In-Reply-To: <CAM_iQpVMSYO-sKDkJ+u9=_DxGRsd=vzqnW3Ao3aP0CC6PTm2Kg@mail.gmail.com>
On Thu 13 Dec 2018 at 23:32, Cong Wang <xiyou.wangcong@gmail•com> wrote:
> On Tue, Dec 11, 2018 at 2:19 AM Vlad Buslov <vladbu@mellanox•com> wrote:
>>
>> As a part of the effort to remove dependency on rtnl lock, cls API is being
>> converted to use fine-grained locking mechanisms instead of global rtnl
>> lock. However, chain_head_change callback for ingress Qdisc is a sleeping
>> function and cannot be executed while holding a spinlock.
>
>
> Why does it have to be a spinlock not a mutex?
>
> I've read your cover letter and this changelog, I don't find any
> answer.
My initial implementation used mutex. However, it was changed to
spinlock by Jiri's request during internal review.
>
>>
>> Extend cls API with new workqueue intended to be used for tcf_proto
>> lifetime management. Modify tcf_proto_destroy() to deallocate proto
>> asynchronously on workqueue in order to ensure that all chain_head_change
>> callbacks involving the proto complete before it is freed. Convert
>> mini_qdisc_pair_swap(), that is used as a chain_head_change callback for
>> ingress and clsact Qdiscs, to use a workqueue. Move Qdisc deallocation to
>> tc_proto_wq ordered workqueue that is used to destroy tcf proto instances.
>> This is necessary to ensure that Qdisc is destroyed after all instances of
>> chain/proto that it contains in order to prevent use-after-free error in
>> tc_chain_notify_delete().
>
>
> Please avoid async unless you have to, there are almost always bugs
> when playing with deferred workqueue or any other callbacks.
Indeed, async Qdisc and tp deallocation introduces additional
complexity. What approach would you recommend to make chain_head_change
callback atomic?
>
> Thanks.
Thank you for reviewing my code!
next prev parent reply other threads:[~2018-12-16 16:32 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-12-11 10:11 [PATCH net-next v2 00/17] Refactor classifier API to work with chain/classifiers without rtnl lock Vlad Buslov
2018-12-11 10:11 ` [PATCH net-next v2 01/17] net: sched: refactor mini_qdisc_pair_swap() to use workqueue Vlad Buslov
2018-12-12 3:23 ` kbuild test robot
2018-12-13 23:32 ` Cong Wang
2018-12-16 16:32 ` Vlad Buslov [this message]
2018-12-16 18:52 ` Cong Wang
2018-12-17 10:22 ` Jiri Pirko
2018-12-19 4:27 ` Cong Wang
2018-12-20 13:55 ` Vlad Buslov
2019-01-14 19:00 ` Vlad Buslov
2019-01-14 23:00 ` Cong Wang
2018-12-19 8:12 ` kbuild test robot
2018-12-11 10:11 ` [PATCH net-next v2 02/17] net: sched: protect block state with spinlock Vlad Buslov
2018-12-11 10:11 ` [PATCH net-next v2 03/17] net: sched: refactor tc_ctl_chain() to use block->lock Vlad Buslov
2018-12-11 10:11 ` [PATCH net-next v2 04/17] net: sched: protect block->chain0 with block->lock Vlad Buslov
2018-12-11 10:11 ` [PATCH net-next v2 05/17] net: sched: traverse chains in block with tcf_get_next_chain() Vlad Buslov
2018-12-11 10:11 ` [PATCH net-next v2 06/17] net: sched: protect chain template accesses with block lock Vlad Buslov
2018-12-11 10:11 ` [PATCH net-next v2 07/17] net: sched: lock the chain when accessing filter_chain list Vlad Buslov
2018-12-11 10:11 ` [PATCH net-next v2 08/17] net: sched: introduce reference counting for tcf proto Vlad Buslov
2018-12-11 10:11 ` [PATCH net-next v2 09/17] net: sched: traverse classifiers in chain with tcf_get_next_proto() Vlad Buslov
2018-12-14 10:15 ` kbuild test robot
2018-12-11 10:11 ` [PATCH net-next v2 10/17] net: sched: refactor tp insert/delete for concurrent execution Vlad Buslov
2018-12-11 10:11 ` [PATCH net-next v2 11/17] net: sched: prevent insertion of new classifiers during chain flush Vlad Buslov
2018-12-11 10:11 ` [PATCH net-next v2 12/17] net: sched: track rtnl lock status when validating extensions Vlad Buslov
2018-12-11 10:11 ` [PATCH net-next v2 13/17] net: sched: extend proto ops with 'put' callback Vlad Buslov
2018-12-11 10:11 ` [PATCH net-next v2 14/17] net: sched: extend proto ops to support unlocked classifiers Vlad Buslov
2018-12-11 10:11 ` [PATCH net-next v2 15/17] net: sched: add flags to Qdisc class ops struct Vlad Buslov
2018-12-11 10:11 ` [PATCH net-next v2 16/17] net: sched: refactor tcf_block_find() into standalone functions Vlad Buslov
2018-12-11 10:12 ` [PATCH net-next v2 17/17] net: sched: unlock rules update API Vlad Buslov
2018-12-20 0:15 ` kbuild test robot
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=vbfftuxzb3r.fsf@mellanox.com \
--to=vladbu@mellanox$(echo .)com \
--cc=ast@kernel$(echo .)org \
--cc=daniel@iogearbox$(echo .)net \
--cc=davem@davemloft$(echo .)net \
--cc=jhs@mojatatu$(echo .)com \
--cc=jiri@resnulli$(echo .)us \
--cc=netdev@vger$(echo .)kernel.org \
--cc=xiyou.wangcong@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
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