From patchwork Tue Dec 5 15:30:11 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Tammela X-Patchwork-Id: 13480331 X-Patchwork-Delegate: kuba@kernel.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=mojatatu-com.20230601.gappssmtp.com header.i=@mojatatu-com.20230601.gappssmtp.com header.b="oLMD5nEf" Received: from mail-pl1-x62c.google.com (mail-pl1-x62c.google.com [IPv6:2607:f8b0:4864:20::62c]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 36A3DA9 for ; Tue, 5 Dec 2023 07:30:31 -0800 (PST) Received: by mail-pl1-x62c.google.com with SMTP id d9443c01a7336-1cf7a8ab047so23265645ad.1 for ; Tue, 05 Dec 2023 07:30:31 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=mojatatu-com.20230601.gappssmtp.com; s=20230601; t=1701790230; x=1702395030; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=OBeO4uD5WmpB1aJU//2ZqVMWJ1KMTm3aSrEbBbQl+GQ=; b=oLMD5nEf9t+LAxTDU3OYK+iloExtYxgA4KRp08bPiHC9XLrNuxJ2eu/cY9+I2RLgTG awrVkCcDvb3Pf69u2QP+T3fQDXVa6U/G/s7Kvn8Atn/huY/SiK6CWyW+yAwNxvFMRfJT wJztwAmafI+2jciFNNSqUX6yRRvt+Q3XiJtRkAJ1iY57BEH48ZKXFn8SAZo3VmyfxtNF ZqzeskgcTIPxXBo9YS62H+GsGiUHnA2Jy1hGfzTzsNUssCvH6k7cguosswhKQr1788mg Wmt6j7NsVwIRNUWF2LIIkO1TqWyaVMf6pwcM9KKuBFnkFAVmTFBm/JL+IKTCC8bkTlmU Rmhg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1701790230; x=1702395030; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=OBeO4uD5WmpB1aJU//2ZqVMWJ1KMTm3aSrEbBbQl+GQ=; b=HyIdQLOJfXTsQ9LwyLVuleCCKI+tUBFWsQ3BbKeymKShC777U1oK7tKLQgdJXcs5vT mwJVF19L0qxoVjbvCQPaBNKGudRFhgEOysBgdDt5t19Fx7WmihrGydG700RHKGS8slLn GMwFN6mUqRav6+pYRqGyQs/HxNiJPjbDHdS3An4RI+qDbm1rQsJoxuGjneFrXlLjFIPG hEQHGl1JwwidMWgbsidGtR0Vs3MzLslDcQoLSBPDecSoauFsixhwgW/0bnbeHL19m7/e BJUaSUV0bhKVKqcBRbSbXfa09I3ksrMFhC4uXQKciImISfxmr8K+tDZ67dQNKMEIcJwa CU7A== X-Gm-Message-State: AOJu0YzJZmZcflUPQhikkkBNC6zpY+7G9XM1tYntDzBJ2dS1p1g9OuvM 7jNTvTOF+D4+gqC6CGyTuXWFbHBGTQgtOKSUAIs= X-Google-Smtp-Source: AGHT+IF9rv3X8VaxDOvqKs+349wzROnnmG726xgEil+piqg37VPmDRzOgvuzzu6IcYWKaE1tpgqzbw== X-Received: by 2002:a17:902:bd49:b0:1d0:b4cc:43ef with SMTP id b9-20020a170902bd4900b001d0b4cc43efmr2110852plx.24.1701790230489; Tue, 05 Dec 2023 07:30:30 -0800 (PST) Received: from rogue-one.tail33bf8.ts.net ([201.17.86.134]) by smtp.gmail.com with ESMTPSA id w3-20020a170902a70300b001cfc34965aesm10384427plq.50.2023.12.05.07.30.27 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 05 Dec 2023 07:30:30 -0800 (PST) From: Pedro Tammela To: netdev@vger.kernel.org Cc: davem@davemloft.net, edumazet@google.com, kuba@kernel.org, pabeni@redhat.com, jhs@mojatatu.com, xiyou.wangcong@gmail.com, jiri@resnulli.us, marcelo.leitner@gmail.com, vladbu@nvidia.com, Pedro Tammela Subject: [PATCH net-next 1/2] net/sched: act_api: rely on rcu in tcf_idr_check_alloc Date: Tue, 5 Dec 2023 12:30:11 -0300 Message-Id: <20231205153012.484687-2-pctammela@mojatatu.com> X-Mailer: git-send-email 2.40.1 In-Reply-To: <20231205153012.484687-1-pctammela@mojatatu.com> References: <20231205153012.484687-1-pctammela@mojatatu.com> Precedence: bulk X-Mailing-List: netdev@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 X-Patchwork-Delegate: kuba@kernel.org Instead of relying only on the idrinfo->lock mutex for bind/alloc logic, rely on a combination of rcu + mutex + atomics to better scale the case where multiple rtnl-less filters are binding to the same action object. Action binding happens when an action index is specified explicitly and an action exists which such index exists. Example: tc actions add action drop index 1 tc filter add ... matchall action drop index 1 tc filter add ... matchall action drop index 1 tc filter add ... matchall action drop index 1 tc filter ls ... filter protocol all pref 49150 matchall chain 0 filter protocol all pref 49150 matchall chain 0 handle 0x1 not_in_hw action order 1: gact action drop random type none pass val 0 index 1 ref 4 bind 3 filter protocol all pref 49151 matchall chain 0 filter protocol all pref 49151 matchall chain 0 handle 0x1 not_in_hw action order 1: gact action drop random type none pass val 0 index 1 ref 4 bind 3 filter protocol all pref 49152 matchall chain 0 filter protocol all pref 49152 matchall chain 0 handle 0x1 not_in_hw action order 1: gact action drop random type none pass val 0 index 1 ref 4 bind 3 When no index is specified, as before, grab the mutex and allocate in the idr the next available id. In this version, as opposed to before, it's simplified to store the -EBUSY pointer instead of the previous alloc + replace combination. When an index is specified, rely on rcu to find if there's an object in such index. If there's none, fallback to the above, serializing on the mutex and reserving the specified id. If there's one, it can be an -EBUSY pointer, in which case we just try again until it's an action, or an action. Given the rcu guarantees, the action found could be dead and therefore we need to bump the refcount if it's not 0, handling the case it's in fact 0. As bind and the action refcount are already atomics, these increments can happen without the mutex protection while many tcf_idr_check_alloc race to bind to the same action instance. Signed-off-by: Pedro Tammela --- net/sched/act_api.c | 56 +++++++++++++++++++++++++++------------------ 1 file changed, 34 insertions(+), 22 deletions(-) diff --git a/net/sched/act_api.c b/net/sched/act_api.c index abec5c45b5a4..79a044d2ae02 100644 --- a/net/sched/act_api.c +++ b/net/sched/act_api.c @@ -824,43 +824,55 @@ int tcf_idr_check_alloc(struct tc_action_net *tn, u32 *index, struct tcf_idrinfo *idrinfo = tn->idrinfo; struct tc_action *p; int ret; + u32 max; -again: - mutex_lock(&idrinfo->lock); if (*index) { +again: + rcu_read_lock(); p = idr_find(&idrinfo->action_idr, *index); + if (IS_ERR(p)) { /* This means that another process allocated * index but did not assign the pointer yet. */ - mutex_unlock(&idrinfo->lock); + rcu_read_unlock(); goto again; } - if (p) { - refcount_inc(&p->tcfa_refcnt); - if (bind) - atomic_inc(&p->tcfa_bindcnt); - *a = p; - ret = 1; - } else { - *a = NULL; - ret = idr_alloc_u32(&idrinfo->action_idr, NULL, index, - *index, GFP_KERNEL); - if (!ret) - idr_replace(&idrinfo->action_idr, - ERR_PTR(-EBUSY), *index); + if (!p) { + /* Empty slot, try to allocate it */ + max = *index; + rcu_read_unlock(); + goto new; + } + + if (!refcount_inc_not_zero(&p->tcfa_refcnt)) { + /* Action was deleted in parallel */ + rcu_read_unlock(); + return -ENOENT; } + + if (bind) + atomic_inc(&p->tcfa_bindcnt); + *a = p; + + rcu_read_unlock(); + + return 1; } else { + /* Find a slot */ *index = 1; - *a = NULL; - ret = idr_alloc_u32(&idrinfo->action_idr, NULL, index, - UINT_MAX, GFP_KERNEL); - if (!ret) - idr_replace(&idrinfo->action_idr, ERR_PTR(-EBUSY), - *index); + max = UINT_MAX; } + +new: + *a = NULL; + + mutex_lock(&idrinfo->lock); + ret = idr_alloc_u32(&idrinfo->action_idr, ERR_PTR(-EBUSY), index, max, + GFP_KERNEL); mutex_unlock(&idrinfo->lock); + return ret; } EXPORT_SYMBOL(tcf_idr_check_alloc); From patchwork Tue Dec 5 15:30:12 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Tammela X-Patchwork-Id: 13480332 X-Patchwork-Delegate: kuba@kernel.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=mojatatu-com.20230601.gappssmtp.com header.i=@mojatatu-com.20230601.gappssmtp.com header.b="k81CwbeE" Received: from mail-pj1-x1029.google.com (mail-pj1-x1029.google.com [IPv6:2607:f8b0:4864:20::1029]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 995A383 for ; Tue, 5 Dec 2023 07:30:34 -0800 (PST) Received: by mail-pj1-x1029.google.com with SMTP id 98e67ed59e1d1-28657040cdcso3413782a91.3 for ; Tue, 05 Dec 2023 07:30:34 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=mojatatu-com.20230601.gappssmtp.com; s=20230601; t=1701790234; x=1702395034; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=EnrPR0dVCE+kR/RtDKyal/cQSVLNTf8sBi7uitToHqA=; b=k81CwbeEeRPAAjVK2JP8UxQYAhC8oRPuhNaySHsQ34KGrR+oEtWjXtzHEB0AQeNoUj 7jEBCTLP66rOv6DIu5GIUuKtwkmqgn31G/ng9T3CyC47XCp3PxO7QznpjlT1wA/bddsy Tfx1l7659KN72VAVTSaVrNJ5B0E4mVIMSlLVSjFzZVdALRihafi5uW3RyHTpTv9OJ7kf tBS44gQDDACVrmLKnxuVyZh74RR3HamvjZMCcSYbruJY3d4nllAwZ1UjHr8dpT/pJIvH B69pC88sIbe6w3ABbuuMEOahIsQL4vr7VxW6t5n+Al3yd+0gCFgjb/gPl4qIBYYzxG5i O3Ig== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1701790234; x=1702395034; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=EnrPR0dVCE+kR/RtDKyal/cQSVLNTf8sBi7uitToHqA=; b=VxulJMH+9VgFKY1Rki7DLZAef/g0vmJawrjD31CMmrP1/Uc///nO1we7Azib59YEiB pkN1zzWWDnpq5z5tbQaRpE2emTzo8gUipUrV9rvoZRCxPkBSMhkxAuIQ8V5fnFBRIuw2 hXYqzf89SdtbQeVm3GGcp8EJ8IJ0kIo/uuihK3zyKKqWqiyTXX3So8b+StHqAZ1zLK5z hjk1SP+yeaEewIR2JlntP4yj8qC91dPCQM8H616FqwnMd9s/wi64Ag0ztXp+FsOgW52i XnO8CDZ4OaKc7ZU2N1/j4B9B4jYdThhLVyWOKvahN2Wr0Eo0cg173I0RnY+G9C1llopp H3kA== X-Gm-Message-State: AOJu0YxhoUcGtSwi5QVsGLvY0WDVeMx0BBZqXJj9zT/PkLnFM82h1+3/ br7ekTAytxOIZ+hUox/+CAh5/rZA0CbdwO9eJC4= X-Google-Smtp-Source: AGHT+IEOz9EpSTovJC5vVsGmf0c5kWp5zYZQs/KwlgzprJ+j5WwzpIg6pKffh7PTnKnK8tYWOXnmDQ== X-Received: by 2002:a17:903:2616:b0:1d0:60ce:be09 with SMTP id jd22-20020a170903261600b001d060cebe09mr5035987plb.61.1701790233844; Tue, 05 Dec 2023 07:30:33 -0800 (PST) Received: from rogue-one.tail33bf8.ts.net ([201.17.86.134]) by smtp.gmail.com with ESMTPSA id w3-20020a170902a70300b001cfc34965aesm10384427plq.50.2023.12.05.07.30.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 05 Dec 2023 07:30:33 -0800 (PST) From: Pedro Tammela To: netdev@vger.kernel.org Cc: davem@davemloft.net, edumazet@google.com, kuba@kernel.org, pabeni@redhat.com, jhs@mojatatu.com, xiyou.wangcong@gmail.com, jiri@resnulli.us, marcelo.leitner@gmail.com, vladbu@nvidia.com, Pedro Tammela Subject: [PATCH net-next 2/2] net/sched: act_api: skip idr replace on bound actions Date: Tue, 5 Dec 2023 12:30:12 -0300 Message-Id: <20231205153012.484687-3-pctammela@mojatatu.com> X-Mailer: git-send-email 2.40.1 In-Reply-To: <20231205153012.484687-1-pctammela@mojatatu.com> References: <20231205153012.484687-1-pctammela@mojatatu.com> Precedence: bulk X-Mailing-List: netdev@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 X-Patchwork-Delegate: kuba@kernel.org tcf_idr_insert_many will replace the allocated -EBUSY pointer in tcf_idr_check_alloc with the real action pointer, exposing it to all operations. This operation is only needed when the action pointer is created (ACT_P_CREATED). For actions which are bound to (returned 0), the pointer already resides in the idr making such operation a nop. Even though it's a nop, it's still not a cheap operation as internally the idr code walks the idr and then does a replace on the appropriate slot. So if the action was bound, better skip the idr replace entirely. Signed-off-by: Pedro Tammela --- include/net/act_api.h | 2 +- net/sched/act_api.c | 11 ++++++----- net/sched/cls_api.c | 2 +- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/include/net/act_api.h b/include/net/act_api.h index 4ae0580b63ca..ea13e1e4a7c2 100644 --- a/include/net/act_api.h +++ b/include/net/act_api.h @@ -191,7 +191,7 @@ int tcf_idr_create_from_flags(struct tc_action_net *tn, u32 index, struct nlattr *est, struct tc_action **a, const struct tc_action_ops *ops, int bind, u32 flags); -void tcf_idr_insert_many(struct tc_action *actions[]); +void tcf_idr_insert_many(struct tc_action *actions[], int init_res[]); void tcf_idr_cleanup(struct tc_action_net *tn, u32 index); int tcf_idr_check_alloc(struct tc_action_net *tn, u32 *index, struct tc_action **a, int bind); diff --git a/net/sched/act_api.c b/net/sched/act_api.c index 79a044d2ae02..64d35ab46993 100644 --- a/net/sched/act_api.c +++ b/net/sched/act_api.c @@ -1295,7 +1295,7 @@ static const struct nla_policy tcf_action_policy[TCA_ACT_MAX + 1] = { [TCA_ACT_HW_STATS] = NLA_POLICY_BITFIELD32(TCA_ACT_HW_STATS_ANY), }; -void tcf_idr_insert_many(struct tc_action *actions[]) +void tcf_idr_insert_many(struct tc_action *actions[], int init_res[]) { struct tc_action *a; int i; @@ -1303,11 +1303,12 @@ void tcf_idr_insert_many(struct tc_action *actions[]) tcf_act_for_each_action(i, a, actions) { struct tcf_idrinfo *idrinfo; + if (init_res[i] == 0) /* Bound */ + continue; + idrinfo = a->idrinfo; mutex_lock(&idrinfo->lock); - /* Replace ERR_PTR(-EBUSY) allocated by tcf_idr_check_alloc if - * it is just created, otherwise this is just a nop. - */ + /* Replace ERR_PTR(-EBUSY) allocated by tcf_idr_check_alloc */ idr_replace(&idrinfo->action_idr, a, a->tcfa_index); mutex_unlock(&idrinfo->lock); } @@ -1507,7 +1508,7 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla, /* We have to commit them all together, because if any error happened in * between, we could not handle the failure gracefully. */ - tcf_idr_insert_many(actions); + tcf_idr_insert_many(actions, init_res); *attr_size = tcf_action_full_attrs_size(sz); err = i - 1; diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c index 1976bd163986..6391b341284e 100644 --- a/net/sched/cls_api.c +++ b/net/sched/cls_api.c @@ -3309,7 +3309,7 @@ int tcf_exts_validate_ex(struct net *net, struct tcf_proto *tp, struct nlattr ** act->type = exts->type = TCA_OLD_COMPAT; exts->actions[0] = act; exts->nr_actions = 1; - tcf_idr_insert_many(exts->actions); + tcf_idr_insert_many(exts->actions, init_res); } else if (exts->action && tb[exts->action]) { int err;