[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <09051dd20251cd521c253ed8d133301b03d90f9e.camel@redhat.com>
Date: Tue, 14 Feb 2023 10:03:05 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: Pedro Tammela <pctammela@...atatu.com>, netdev@...r.kernel.org
Cc: jhs@...atatu.com, xiyou.wangcong@...il.com, jiri@...nulli.us,
davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org
Subject: Re: [PATCH net-next 2/3] net/sched: act_connmark: transition to
percpu stats and rcu
On Fri, 2023-02-10 at 17:27 -0300, Pedro Tammela wrote:
> The tc action act_connmark was using shared stats and taking the per
> action lock in the datapath. Improve it by using percpu stats and rcu.
>
> perf before:
> - 13.55% tcf_connmark_act
> - 81.18% _raw_spin_lock
> 80.46% native_queued_spin_lock_slowpath
>
> perf after:
> - 3.12% tcf_connmark_act
>
> tdc results:
> 1..15
> ok 1 2002 - Add valid connmark action with defaults
> ok 2 56a5 - Add valid connmark action with control pass
> ok 3 7c66 - Add valid connmark action with control drop
> ok 4 a913 - Add valid connmark action with control pipe
> ok 5 bdd8 - Add valid connmark action with control reclassify
> ok 6 b8be - Add valid connmark action with control continue
> ok 7 d8a6 - Add valid connmark action with control jump
> ok 8 aae8 - Add valid connmark action with zone argument
> ok 9 2f0b - Add valid connmark action with invalid zone argument
> ok 10 9305 - Add connmark action with unsupported argument
> ok 11 71ca - Add valid connmark action and replace it
> ok 12 5f8f - Add valid connmark action with cookie
> ok 13 c506 - Replace connmark with invalid goto chain control
> ok 14 6571 - Delete connmark action with valid index
> ok 15 3426 - Delete connmark action with invalid index
>
> Reviewed-by: Jamal Hadi Salim <jhs@...atatu.com>
> Signed-off-by: Pedro Tammela <pctammela@...atatu.com>
> ---
> include/net/tc_act/tc_connmark.h | 9 ++-
> net/sched/act_connmark.c | 109 ++++++++++++++++++++-----------
> 2 files changed, 77 insertions(+), 41 deletions(-)
>
> diff --git a/include/net/tc_act/tc_connmark.h b/include/net/tc_act/tc_connmark.h
> index 1f4cb477b..e8dd77a96 100644
> --- a/include/net/tc_act/tc_connmark.h
> +++ b/include/net/tc_act/tc_connmark.h
> @@ -4,10 +4,15 @@
>
> #include <net/act_api.h>
>
> -struct tcf_connmark_info {
> - struct tc_action common;
> +struct tcf_connmark_parms {
> struct net *net;
> u16 zone;
> + struct rcu_head rcu;
> +};
> +
> +struct tcf_connmark_info {
> + struct tc_action common;
> + struct tcf_connmark_parms __rcu *parms;
> };
>
> #define to_connmark(a) ((struct tcf_connmark_info *)a)
> diff --git a/net/sched/act_connmark.c b/net/sched/act_connmark.c
> index 7e63ff7e3..541e1c556 100644
> --- a/net/sched/act_connmark.c
> +++ b/net/sched/act_connmark.c
> @@ -36,13 +36,15 @@ TC_INDIRECT_SCOPE int tcf_connmark_act(struct sk_buff *skb,
> struct nf_conntrack_tuple tuple;
> enum ip_conntrack_info ctinfo;
> struct tcf_connmark_info *ca = to_connmark(a);
> + struct tcf_connmark_parms *parms;
> struct nf_conntrack_zone zone;
> struct nf_conn *c;
> int proto;
>
> - spin_lock(&ca->tcf_lock);
> tcf_lastuse_update(&ca->tcf_tm);
> - bstats_update(&ca->tcf_bstats, skb);
> + tcf_action_update_bstats(&ca->common, skb);
> +
> + parms = rcu_dereference_bh(ca->parms);
>
> switch (skb_protocol(skb, true)) {
> case htons(ETH_P_IP):
> @@ -64,31 +66,31 @@ TC_INDIRECT_SCOPE int tcf_connmark_act(struct sk_buff *skb,
> c = nf_ct_get(skb, &ctinfo);
> if (c) {
> skb->mark = READ_ONCE(c->mark);
> - /* using overlimits stats to count how many packets marked */
> - ca->tcf_qstats.overlimits++;
> - goto out;
> + goto count;
> }
>
> - if (!nf_ct_get_tuplepr(skb, skb_network_offset(skb),
> - proto, ca->net, &tuple))
> + if (!nf_ct_get_tuplepr(skb, skb_network_offset(skb), proto, parms->net,
> + &tuple))
> goto out;
>
> - zone.id = ca->zone;
> + zone.id = parms->zone;
> zone.dir = NF_CT_DEFAULT_ZONE_DIR;
>
> - thash = nf_conntrack_find_get(ca->net, &zone, &tuple);
> + thash = nf_conntrack_find_get(parms->net, &zone, &tuple);
> if (!thash)
> goto out;
>
> c = nf_ct_tuplehash_to_ctrack(thash);
> - /* using overlimits stats to count how many packets marked */
> - ca->tcf_qstats.overlimits++;
> skb->mark = READ_ONCE(c->mark);
> nf_ct_put(c);
>
> -out:
> +count:
> + /* using overlimits stats to count how many packets marked */
> + spin_lock(&ca->tcf_lock);
> + ca->tcf_qstats.overlimits++;
> spin_unlock(&ca->tcf_lock);
I think above you could use tcf_action_inc_overlimit_qstats() and avoid
acquiring the spin lock in most cases.
Side note: it looks like pedit could use a similar change, too - sorry
for missing that point before.
> - return ca->tcf_action;
> +out:
> + return READ_ONCE(ca->tcf_action);
> }
>
> static const struct nla_policy connmark_policy[TCA_CONNMARK_MAX + 1] = {
> @@ -104,6 +106,7 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla,
> struct nlattr *tb[TCA_CONNMARK_MAX + 1];
> bool bind = flags & TCA_ACT_FLAGS_BIND;
> struct tcf_chain *goto_ch = NULL;
> + struct tcf_connmark_parms *nparms, *oparms;
> struct tcf_connmark_info *ci;
> struct tc_connmark *parm;
> int ret = 0, err;
Please respect the reverse x-mas tree above.
Thanks,
Paolo
Powered by blists - more mailing lists