From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jiri Pirko Subject: Re: [PATCH net-next v2 01/17] net: sched: refactor mini_qdisc_pair_swap() to use workqueue Date: Mon, 17 Dec 2018 11:22:48 +0100 Message-ID: <20181217102248.GB2096@nanopsycho> References: <1544523120-4566-1-git-send-email-vladbu@mellanox.com> <1544523120-4566-2-git-send-email-vladbu@mellanox.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Vlad Buslov , Linux Kernel Network Developers , Jamal Hadi Salim , David Miller , Alexei Starovoitov , Daniel Borkmann To: Cong Wang Return-path: Received: from mail-wm1-f67.google.com ([209.85.128.67]:40332 "EHLO mail-wm1-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726755AbeLQKaY (ORCPT ); Mon, 17 Dec 2018 05:30:24 -0500 Received: by mail-wm1-f67.google.com with SMTP id q26so11990355wmf.5 for ; Mon, 17 Dec 2018 02:30:23 -0800 (PST) Content-Disposition: inline In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: Sun, Dec 16, 2018 at 07:52:18PM CET, xiyou.wangcong@gmail.com wrote: >On Sun, Dec 16, 2018 at 8:32 AM Vlad Buslov wrote: >> >> On Thu 13 Dec 2018 at 23:32, Cong Wang wrote: >> > On Tue, Dec 11, 2018 at 2:19 AM Vlad Buslov 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. >> > >So what's the answer to my question? :) Yeah, my concern agains mutexes was that it would be needed to have one per every block and per every chain. I find it quite heavy and I believe it is better to use spinlock in those cases. This patch is a side effect of that. Do you think it would be better to have mutexes instead of spinlocks? > > >> > >> >> >> >> 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? > >I don't look into any of your code yet, from my understanding of your >changelog, it seems all these workqueue stuffs can be gone if you can >make it a mutex instead of a spinlock. > >This is why I stopped here and wait for your answer to my above question. > >Thanks.