From mboxrd@z Thu Jan  1 00:00:00 1970
From: Vlad Buslov <vladbu@mellanox.com>
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
Message-ID: <vbfftuxzb3r.fsf@mellanox.com>
References: <1544523120-4566-1-git-send-email-vladbu@mellanox.com>
 <1544523120-4566-2-git-send-email-vladbu@mellanox.com>
 <CAM_iQpVMSYO-sKDkJ+u9=_DxGRsd=vzqnW3Ao3aP0CC6PTm2Kg@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
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>
To: Cong Wang <xiyou.wangcong@gmail.com>, Jiri Pirko <jiri@resnulli.us>
Return-path: <netdev-owner@vger.kernel.org>
Received: from mail-eopbgr10059.outbound.protection.outlook.com ([40.107.1.59]:43712
        "EHLO EUR02-HE1-obe.outbound.protection.outlook.com"
        rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP
        id S1730337AbeLPQcS (ORCPT <rfc822;netdev@vger.kernel.org>);
        Sun, 16 Dec 2018 11:32:18 -0500
In-Reply-To: <CAM_iQpVMSYO-sKDkJ+u9=_DxGRsd=vzqnW3Ao3aP0CC6PTm2Kg@mail.gmail.com>
Content-Language: en-US
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

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 be=
ing
>> converted to use fine-grained locking mechanisms instead of global rtnl
>> lock. However, chain_head_change callback for ingress Qdisc is a sleepin=
g
>> 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_chang=
e
>> 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 t=
o
>> tc_proto_wq ordered workqueue that is used to destroy tcf proto instance=
s.
>> 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!