Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
References: <1526308035-12484-1-git-send-email-vladbu@mellanox.com> <1526308035-12484-11-git-send-email-vladbu@mellanox.com> <20180516075000.GC1972@nanopsycho>
User-agent: mu4e 0.9.16; emacs 25.3.50.2
From:   Vlad Buslov <vladbu@mellanox.com>
To:     Jiri Pirko <jiri@resnulli.us>
Cc:     netdev@vger.kernel.org, davem@davemloft.net, jhs@mojatatu.com,
        xiyou.wangcong@gmail.com, pablo@netfilter.org,
        kadlec@blackhole.kfki.hu, fw@strlen.de, ast@kernel.org,
        daniel@iogearbox.net, edumazet@google.com, keescook@chromium.org,
        linux-kernel@vger.kernel.org, netfilter-devel@vger.kernel.org,
        coreteam@netfilter.org, kliteyn@mellanox.com
Subject: Re: [PATCH 10/14] net: sched: extend act API for lockless actions
In-reply-to: <20180516075000.GC1972@nanopsycho>
Date:   Wed, 16 May 2018 11:16:13 +0300
Message-ID: <vbfbmdg10du.fsf@reg-r-vrt-018-180.mtr.labs.mlnx>
MIME-Version: 1.0
Content-Type: text/plain
Received-SPF: None (protection.outlook.com: mellanox.com does not designate
 permitted sender hosts)
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 May 2018 08:16:19.0341
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 4295ace4-8260-4ba9-d73f-08d5bb054f1b
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: a652971c-7d2e-4d9b-a6a4-d149256f461b
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB6PR05MB4693
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On Wed 16 May 2018 at 07:50, Jiri Pirko <jiri@resnulli.us> wrote:
> Mon, May 14, 2018 at 04:27:11PM CEST, vladbu@mellanox.com wrote:
>>Implement new action API function to atomically delete action with
>>specified index and to atomically insert unique action. These functions are
>>required to implement init and delete functions for specific actions that
>>do not rely on rtnl lock.
>>
>>Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
>>---
>> include/net/act_api.h |  2 ++
>> net/sched/act_api.c   | 45 +++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 47 insertions(+)
>>
>>diff --git a/include/net/act_api.h b/include/net/act_api.h
>>index a8c8570..bce0cf1 100644
>>--- a/include/net/act_api.h
>>+++ b/include/net/act_api.h
>>@@ -153,7 +153,9 @@ int tcf_idr_create(struct tc_action_net *tn, u32 index, struct nlattr *est,
>> 		   struct tc_action **a, const struct tc_action_ops *ops,
>> 		   int bind, bool cpustats);
>> void tcf_idr_insert(struct tc_action_net *tn, struct tc_action *a);
>>+void tcf_idr_insert_unique(struct tc_action_net *tn, struct tc_action *a);
>> 
>>+int tcf_idr_find_delete(struct tc_action_net *tn, u32 index);
>> int __tcf_idr_release(struct tc_action *a, bool bind, bool strict);
>> 
>> static inline int tcf_idr_release(struct tc_action *a, bool bind)
>>diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>>index 2772276e..a5193dc 100644
>>--- a/net/sched/act_api.c
>>+++ b/net/sched/act_api.c
>>@@ -330,6 +330,41 @@ bool tcf_idr_check(struct tc_action_net *tn, u32 index, struct tc_action **a,
>> }
>> EXPORT_SYMBOL(tcf_idr_check);
>> 
>>+int tcf_idr_find_delete(struct tc_action_net *tn, u32 index)
>>+{
>>+	struct tcf_idrinfo *idrinfo = tn->idrinfo;
>>+	struct tc_action *p;
>>+	int ret = 0;
>>+
>>+	spin_lock_bh(&idrinfo->lock);
>
> Why "_bh" is needed here?

Original idr remove function used _bh version so I used it here as well.
As I already replied to your previous question about idrinfo lock usage,
I don't see any particular reason for locking with _bh at this point.
I've contacted the author(Chris Mi) and he said that he just preserved
locking the same way as it was before he changed hash table to idr for
action lookup.

You want me to do standalone patch that cleans up idrinfo locking?

>
>
>>+	p = idr_find(&idrinfo->action_idr, index);
>>+	if (!p) {
>>+		spin_unlock(&idrinfo->lock);
>>+		return -ENOENT;
>>+	}
>>+
>>+	if (!atomic_read(&p->tcfa_bindcnt)) {
>>+		if (refcount_dec_and_test(&p->tcfa_refcnt)) {
>>+			struct module *owner = p->ops->owner;
>>+
>>+			WARN_ON(p != idr_remove(&idrinfo->action_idr,
>>+						p->tcfa_index));
>>+			spin_unlock_bh(&idrinfo->lock);
>>+
>>+			tcf_action_cleanup(p);
>>+			module_put(owner);
>>+			return 0;
>>+		}
>>+		ret = 0;
>>+	} else {
>>+		ret = -EPERM;
>
> I wonder if "-EPERM" is the best error code for this...

This is what original code returned so I decided to preserve
compatibility.

>
>
>>+	}
>>+
>>+	spin_unlock_bh(&idrinfo->lock);
>>+	return ret;
>>+}
>>+EXPORT_SYMBOL(tcf_idr_find_delete);
>>+
>> int tcf_idr_create(struct tc_action_net *tn, u32 index, struct nlattr *est,
>> 		   struct tc_action **a, const struct tc_action_ops *ops,
>> 		   int bind, bool cpustats)
>>@@ -407,6 +442,16 @@ void tcf_idr_insert(struct tc_action_net *tn, struct tc_action *a)
>> }
>> EXPORT_SYMBOL(tcf_idr_insert);
>> 
>>+void tcf_idr_insert_unique(struct tc_action_net *tn, struct tc_action *a)
>>+{
>>+	struct tcf_idrinfo *idrinfo = tn->idrinfo;
>>+
>>+	spin_lock_bh(&idrinfo->lock);
>>+	WARN_ON(idr_replace(&idrinfo->action_idr, a, a->tcfa_index));
>
> Under which condition this WARN_ON is hit?

When idr replace returns non-NULL pointer, which means that somehow
concurrent insertion of action with same index has happened and we are
leaking memory.

By the way I'm still not sure if having this insert unique function is
warranted or I should just add WARN to regular idr insert. What is your
opinion on this?

>
>
>>+	spin_unlock_bh(&idrinfo->lock);
>>+}
>>+EXPORT_SYMBOL(tcf_idr_insert_unique);
>>+
>> void tcf_idrinfo_destroy(const struct tc_action_ops *ops,
>> 			 struct tcf_idrinfo *idrinfo)
>> {
>>-- 
>>2.7.5
>>