From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.5 required=3.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_PASS,URIBL_BLOCKED,USER_AGENT_MUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 28819C282C4 for ; Mon, 4 Feb 2019 15:32:53 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id DAD612082F for ; Mon, 4 Feb 2019 15:32:52 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=resnulli-us.20150623.gappssmtp.com header.i=@resnulli-us.20150623.gappssmtp.com header.b="cCIeWHqR" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730762AbfBDPcv (ORCPT ); Mon, 4 Feb 2019 10:32:51 -0500 Received: from mail-wm1-f65.google.com ([209.85.128.65]:50538 "EHLO mail-wm1-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726565AbfBDPcv (ORCPT ); Mon, 4 Feb 2019 10:32:51 -0500 Received: by mail-wm1-f65.google.com with SMTP id z5so320466wmf.0 for ; Mon, 04 Feb 2019 07:32:49 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=resnulli-us.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=BVg/3YL2q61ewd90Skqhkp5zoAM//6TvZKy3DMyahDs=; b=cCIeWHqRp0LhQ+NgvN6JYyLb2VN9CMDgH6/xJ6r/WM0c1B3TSvfHnXbGTxxL/FmTtw hXbL0EnbnAzq1OZKzsNrClqvciNsal94+0CZlHscNMu6/FOQS7qGXUvkMB9/lkdTF12N Sq7xMwwKJXXpQHQq9f+1/r1GFhRIBA7h9/Eh84Y5hpcR/2LNSI/5rJb7427qANj0qfjN R40R9oWpw3EWeYsBMw+xeEAMEP/AxgUvIG8gGaurLdMjrVLRC4AUqxDK26KZSutpWb+F PShpdmz8Qnvr4PvLDQY67ECbEa+Db2SZeIOcZI9IV2tj4mocabV/eKq2RXIyKOs0oAC0 qH+A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=BVg/3YL2q61ewd90Skqhkp5zoAM//6TvZKy3DMyahDs=; b=inA1PT+ErCp/lqwx/vzDwxh3qb/lNsNV2t18CNH60HgmWfusiQ3ysjUmnDdA0aOulF NMVGbZe5gynVgRZKuArNd4s7pqPMC+NMpYe6JCA6mlRlPeu4uekQ+q5KgYgp/7R2nFkt 5201URunolGEJLn27tytdVqVdpHHXgENAjlkpyR7FFQxVDRNUJdvZQw306s9JIvWKK5u dc3FFBl/nAMjQRl80dcA6EreKAWuHL7EZXbgp7QKPaSgWjdHMWHCCjw7Bdm5W3FlvJeF kcryRHFshDbpIfQPN2GFh+AQAoJGZE1hRlJI5WV0w5vy+uXteUunwYf1O0hLCNDL/Uq+ eLSw== X-Gm-Message-State: AHQUAuaoIkxi0E2zjPnKqdxxITaKYjU5T4sAFbFgcmrDIOtDMz+kauP5 zbXMzHX/knPglnrWlm4ptnIJMwJJjyE= X-Google-Smtp-Source: AHgI3IbKTBqGd+5QTa7MnNdbmROr2oiPDh8t85T3kX6ijRyF8TthXahblju7jNHXGzWn6alw4yTrAA== X-Received: by 2002:a1c:570d:: with SMTP id l13mr14239064wmb.139.1549294368608; Mon, 04 Feb 2019 07:32:48 -0800 (PST) Received: from localhost (mail.chocen-mesto.cz. [85.163.43.2]) by smtp.gmail.com with ESMTPSA id f22sm6302471wmj.26.2019.02.04.07.32.47 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 04 Feb 2019 07:32:47 -0800 (PST) Date: Mon, 4 Feb 2019 16:23:39 +0100 From: Jiri Pirko To: Vlad Buslov Cc: netdev@vger.kernel.org, jhs@mojatatu.com, xiyou.wangcong@gmail.com, davem@davemloft.net, ast@kernel.org, daniel@iogearbox.net Subject: Re: [PATCH net-next v3 01/16] net: sched: protect block state with mutex Message-ID: <20190204152339.GD2118@nanopsycho> References: <20190204123301.4223-1-vladbu@mellanox.com> <20190204123301.4223-2-vladbu@mellanox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190204123301.4223-2-vladbu@mellanox.com> User-Agent: Mutt/1.10.1 (2018-07-13) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org Mon, Feb 04, 2019 at 01:32:46PM CET, vladbu@mellanox.com wrote: >Currently, tcf_block doesn't use any synchronization mechanisms to protect >critical sections that manage lifetime of its chains. block->chain_list and >multiple variables in tcf_chain that control its lifetime assume external >synchronization provided by global rtnl lock. Converting chain reference >counting to atomic reference counters is not possible because cls API uses >multiple counters and flags to control chain lifetime, so all of them must >be synchronized in chain get/put code. > >Use single per-block lock to protect block data and manage lifetime of all >chains on the block. Always take block->lock when accessing chain_list. >Chain get and put modify chain lifetime-management data and parent block's >chain_list, so take the lock in these functions. Verify block->lock state >with assertions in functions that expect to be called with the lock taken >and are called from multiple places. Take block->lock when accessing >filter_chain_list. > >In order to allow parallel update of rules on single block, move all calls >to classifiers outside of critical sections protected by new block->lock. >Rearrange chain get and put functions code to only access protected chain >data while holding block lock: >- Check if chain was explicitly created inside put function while holding > block lock. Add additional argument to __tcf_chain_put() to only put > explicitly created chain. >- Rearrange code to only access chain reference counter and chain action > reference counter while holding block lock. >- Extract code that requires block->lock from tcf_chain_destroy() into > standalone tcf_chain_destroy() function that is called by > __tcf_chain_put() in same critical section that changes chain reference > counters. > >Signed-off-by: Vlad Buslov >--- > >Changes from V2 to V3: > - Change block->lock type to mutex. > - Implement tcf_block_destroy() helper function that destroys > block->lock mutex before deallocating the block. > - Revert GFP_KERNEL->GFP_ATOMIC memory allocation flags of tcf_chain > which is no longer needed after block->lock type change. > > include/net/sch_generic.h | 5 +++ > net/sched/cls_api.c | 102 ++++++++++++++++++++++++++++++++++++++-------- > 2 files changed, 89 insertions(+), 18 deletions(-) > >diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h >index 7a4957599874..31b8ea66a47d 100644 >--- a/include/net/sch_generic.h >+++ b/include/net/sch_generic.h >@@ -12,6 +12,7 @@ > #include > #include > #include >+#include > #include > #include > >@@ -352,6 +353,10 @@ struct tcf_chain { > }; > > struct tcf_block { >+ /* Lock protects tcf_block and lifetime-management data of chains >+ * attached to the block (refcnt, action_refcnt, explicitly_created). >+ */ >+ struct mutex lock; > struct list_head chain_list; > u32 index; /* block index for shared blocks */ > refcount_t refcnt; >diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c >index e2b5cb2eb34e..cc416b6a3aa2 100644 >--- a/net/sched/cls_api.c >+++ b/net/sched/cls_api.c >@@ -193,6 +193,9 @@ static void tcf_proto_destroy(struct tcf_proto *tp, > kfree_rcu(tp, rcu); > } > >+#define ASSERT_BLOCK_LOCKED(block) \ >+ lockdep_assert_held(&(block)->lock) >+ > struct tcf_filter_chain_list_item { > struct list_head list; > tcf_chain_head_change_t *chain_head_change; >@@ -204,6 +207,8 @@ static struct tcf_chain *tcf_chain_create(struct tcf_block *block, > { > struct tcf_chain *chain; > >+ ASSERT_BLOCK_LOCKED(block); >+ > chain = kzalloc(sizeof(*chain), GFP_KERNEL); > if (!chain) > return NULL; >@@ -235,25 +240,51 @@ static void tcf_chain0_head_change(struct tcf_chain *chain, > tcf_chain_head_change_item(item, tp_head); > } > >-static void tcf_chain_destroy(struct tcf_chain *chain) >+/* Returns true if block can be safely freed. */ >+ >+static bool tcf_chain_detach(struct tcf_chain *chain) > { > struct tcf_block *block = chain->block; > >+ ASSERT_BLOCK_LOCKED(block); >+ > list_del(&chain->list); > if (!chain->index) > block->chain0.chain = NULL; >+ >+ if (list_empty(&block->chain_list) && >+ refcount_read(&block->refcnt) == 0) >+ return true; >+ >+ return false; >+} >+ >+static void tcf_block_destroy(struct tcf_block *block) >+{ >+ mutex_destroy(&block->lock); >+ kfree_rcu(block, rcu); >+} >+ >+static void tcf_chain_destroy(struct tcf_chain *chain, bool free_block) >+{ >+ struct tcf_block *block = chain->block; >+ > kfree(chain); >- if (list_empty(&block->chain_list) && !refcount_read(&block->refcnt)) >- kfree_rcu(block, rcu); >+ if (free_block) >+ tcf_block_destroy(block); > } > > static void tcf_chain_hold(struct tcf_chain *chain) > { >+ ASSERT_BLOCK_LOCKED(chain->block); >+ > ++chain->refcnt; > } > > static bool tcf_chain_held_by_acts_only(struct tcf_chain *chain) > { >+ ASSERT_BLOCK_LOCKED(chain->block); >+ > /* In case all the references are action references, this > * chain should not be shown to the user. > */ >@@ -265,6 +296,8 @@ static struct tcf_chain *tcf_chain_lookup(struct tcf_block *block, > { > struct tcf_chain *chain; > >+ ASSERT_BLOCK_LOCKED(block); >+ > list_for_each_entry(chain, &block->chain_list, list) { > if (chain->index == chain_index) > return chain; >@@ -279,31 +312,40 @@ static struct tcf_chain *__tcf_chain_get(struct tcf_block *block, > u32 chain_index, bool create, > bool by_act) > { >- struct tcf_chain *chain = tcf_chain_lookup(block, chain_index); >+ struct tcf_chain *chain = NULL; >+ bool is_first_reference; > >+ mutex_lock(&block->lock); >+ chain = tcf_chain_lookup(block, chain_index); > if (chain) { > tcf_chain_hold(chain); > } else { > if (!create) >- return NULL; >+ goto errout; > chain = tcf_chain_create(block, chain_index); > if (!chain) >- return NULL; >+ goto errout; > } > > if (by_act) > ++chain->action_refcnt; >+ is_first_reference = chain->refcnt - chain->action_refcnt == 1; >+ mutex_unlock(&block->lock); > > /* Send notification only in case we got the first > * non-action reference. Until then, the chain acts only as > * a placeholder for actions pointing to it and user ought > * not know about them. > */ >- if (chain->refcnt - chain->action_refcnt == 1 && !by_act) >+ if (is_first_reference && !by_act) > tc_chain_notify(chain, NULL, 0, NLM_F_CREATE | NLM_F_EXCL, > RTM_NEWCHAIN, false); > > return chain; >+ >+errout: >+ mutex_unlock(&block->lock); >+ return chain; > } > > static struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index, >@@ -320,37 +362,59 @@ EXPORT_SYMBOL(tcf_chain_get_by_act); > > static void tc_chain_tmplt_del(struct tcf_chain *chain); > >-static void __tcf_chain_put(struct tcf_chain *chain, bool by_act) >+static void __tcf_chain_put(struct tcf_chain *chain, bool by_act, >+ bool explicitly_created) > { >+ struct tcf_block *block = chain->block; >+ bool is_last, free_block = false; >+ unsigned int refcnt; >+ >+ mutex_lock(&block->lock); >+ if (explicitly_created) { >+ if (!chain->explicitly_created) { >+ mutex_unlock(&block->lock); >+ return; >+ } >+ chain->explicitly_created = false; Hmm, I think that you left "chain->explicitly_created = false" at the original location (tc_ctl_chain()). I think it would be better to do the chain->explicitly_created management move in a separate patch. >+ } >+ > if (by_act) > chain->action_refcnt--; >- chain->refcnt--; >+ >+ /* tc_chain_notify_delete can't be called while holding block lock. >+ * However, when block is unlocked chain can be changed concurrently, so >+ * save these to temporary variables. >+ */ >+ refcnt = --chain->refcnt; >+ is_last = refcnt - chain->action_refcnt == 0; >+ if (refcnt == 0) >+ free_block = tcf_chain_detach(chain); >+ mutex_unlock(&block->lock); > > /* The last dropped non-action reference will trigger notification. */ >- if (chain->refcnt - chain->action_refcnt == 0 && !by_act) >+ if (is_last && !by_act) > tc_chain_notify(chain, NULL, 0, 0, RTM_DELCHAIN, false); > >- if (chain->refcnt == 0) { >+ if (refcnt == 0) { > tc_chain_tmplt_del(chain); >- tcf_chain_destroy(chain); >+ tcf_chain_destroy(chain, free_block); > } > } > > static void tcf_chain_put(struct tcf_chain *chain) > { >- __tcf_chain_put(chain, false); >+ __tcf_chain_put(chain, false, false); > } > > void tcf_chain_put_by_act(struct tcf_chain *chain) > { >- __tcf_chain_put(chain, true); >+ __tcf_chain_put(chain, true, false); > } > EXPORT_SYMBOL(tcf_chain_put_by_act); > > static void tcf_chain_put_explicitly_created(struct tcf_chain *chain) > { >- if (chain->explicitly_created) >- tcf_chain_put(chain); >+ __tcf_chain_put(chain, false, true); > } > > static void tcf_chain_flush(struct tcf_chain *chain) >@@ -764,6 +828,7 @@ static struct tcf_block *tcf_block_create(struct net *net, struct Qdisc *q, > NL_SET_ERR_MSG(extack, "Memory allocation for block failed"); > return ERR_PTR(-ENOMEM); > } >+ mutex_init(&block->lock); > INIT_LIST_HEAD(&block->chain_list); > INIT_LIST_HEAD(&block->cb_list); > INIT_LIST_HEAD(&block->owner_list); >@@ -827,7 +892,7 @@ static void tcf_block_put_all_chains(struct tcf_block *block) > static void __tcf_block_put(struct tcf_block *block, struct Qdisc *q, > struct tcf_block_ext_info *ei) > { >- if (refcount_dec_and_test(&block->refcnt)) { >+ if (refcount_dec_and_mutex_lock(&block->refcnt, &block->lock)) { > /* Flushing/putting all chains will cause the block to be > * deallocated when last chain is freed. However, if chain_list > * is empty, block has to be manually deallocated. After block >@@ -836,6 +901,7 @@ static void __tcf_block_put(struct tcf_block *block, struct Qdisc *q, > */ > bool free_block = list_empty(&block->chain_list); > >+ mutex_unlock(&block->lock); > if (tcf_block_shared(block)) > tcf_block_remove(block, block->net); > if (!free_block) >@@ -845,7 +911,7 @@ static void __tcf_block_put(struct tcf_block *block, struct Qdisc *q, > tcf_block_offload_unbind(block, q, ei); > > if (free_block) >- kfree_rcu(block, rcu); >+ tcf_block_destroy(block); > else > tcf_block_put_all_chains(block); > } else if (q) { >-- >2.13.6 >