Re: [PATCH net-next 2/3] net/sched: act_connmark: transition to percpu stats and rcu

[Paolo Abeni <pabeni@redhat.com>]

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@mojatatu.com>
> Signed-off-by: Pedro Tammela <pctammela@mojatatu.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