Re: [PATCH net-next v4 1/2] net/sched: transition act_pedit to rcu and percpu stats

[Simon Horman <simon.horman@corigine.com>]

On Mon, Jan 30, 2023 at 01:02:32PM -0300, Pedro Tammela wrote:
> The software pedit action didn't get the same love as some of the
> other actions and it's still using spinlocks and shared stats in the
> datapath.
> Transition the action to rcu and percpu stats as this improves the
> action's performance on multiple cpu deployments.
> 
> Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
> ---
>  include/net/tc_act/tc_pedit.h |  81 +++++++++++++++----
>  net/sched/act_pedit.c         | 144 +++++++++++++++++++++-------------
>  2 files changed, 154 insertions(+), 71 deletions(-)

Hi Pedro,

thanks for the update.

A few questions from my side below.

> diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
> index a0378e9f0121..c8e8748dc258 100644
> --- a/net/sched/act_pedit.c
> +++ b/net/sched/act_pedit.c
> @@ -134,6 +134,17 @@ static int tcf_pedit_key_ex_dump(struct sk_buff *skb,
>  	return -EINVAL;
>  }
>  
> +static void tcf_pedit_cleanup_rcu(struct rcu_head *head)
> +{
> +	struct tcf_pedit_parms *parms =
> +		container_of(head, struct tcf_pedit_parms, rcu);
> +
> +	kfree(parms->tcfp_keys_ex);
> +	kfree(parms->tcfp_keys);
> +
> +	kfree(parms);
> +}
> +
>  static int tcf_pedit_init(struct net *net, struct nlattr *nla,
>  			  struct nlattr *est, struct tc_action **a,
>  			  struct tcf_proto *tp, u32 flags,
> @@ -143,8 +154,7 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
>  	bool bind = flags & TCA_ACT_FLAGS_BIND;
>  	struct nlattr *tb[TCA_PEDIT_MAX + 1];
>  	struct tcf_chain *goto_ch = NULL;
> -	struct tc_pedit_key *keys = NULL;
> -	struct tcf_pedit_key_ex *keys_ex;
> +	struct tcf_pedit_parms *oparms, *nparms;
>  	struct tc_pedit *parm;
>  	struct nlattr *pattr;
>  	struct tcf_pedit *p;
> @@ -181,18 +191,25 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
>  		return -EINVAL;
>  	}
>  
> -	keys_ex = tcf_pedit_keys_ex_parse(tb[TCA_PEDIT_KEYS_EX], parm->nkeys);
> -	if (IS_ERR(keys_ex))
> -		return PTR_ERR(keys_ex);
> +	nparms = kzalloc(sizeof(*nparms), GFP_KERNEL);
> +	if (!nparms)
> +		return -ENOMEM;
> +
> +	nparms->tcfp_keys_ex =
> +		tcf_pedit_keys_ex_parse(tb[TCA_PEDIT_KEYS_EX], parm->nkeys);
> +	if (IS_ERR(nparms->tcfp_keys_ex)) {
> +		ret = PTR_ERR(nparms->tcfp_keys_ex);
> +		goto out_free;
> +	}
>  
>  	index = parm->index;
>  	err = tcf_idr_check_alloc(tn, &index, a, bind);
>  	if (!err) {
> -		ret = tcf_idr_create(tn, index, est, a,
> -				     &act_pedit_ops, bind, false, flags);
> +		ret = tcf_idr_create_from_flags(tn, index, est, a,
> +						&act_pedit_ops, bind, flags);
>  		if (ret) {
>  			tcf_idr_cleanup(tn, index);
> -			goto out_free;
> +			goto out_free_ex;
>  		}
>  		ret = ACT_P_CREATED;
>  	} else if (err > 0) {
> @@ -204,7 +221,7 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
>  		}
>  	} else {
>  		ret = err;
> -		goto out_free;
> +		goto out_free_ex;
>  	}
>  
>  	err = tcf_action_check_ctrlact(parm->action, tp, &goto_ch, extack);
> @@ -212,68 +229,79 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
>  		ret = err;
>  		goto out_release;
>  	}
> +
> +	nparms->tcfp_off_max_hint = 0;
> +	nparms->tcfp_flags = parm->flags;
> +
>  	p = to_pedit(*a);
>  	spin_lock_bh(&p->tcf_lock);
>  
> +	oparms = rcu_dereference_protected(p->parms, 1);
> +
>  	if (ret == ACT_P_CREATED ||
> -	    (p->tcfp_nkeys && p->tcfp_nkeys != parm->nkeys)) {
> -		keys = kmalloc(ksize, GFP_ATOMIC);
> -		if (!keys) {
> +	    (oparms->tcfp_nkeys && oparms->tcfp_nkeys != parm->nkeys)) {
> +		nparms->tcfp_keys = kmalloc(ksize, GFP_ATOMIC);
> +		if (!nparms->tcfp_keys) {
>  			spin_unlock_bh(&p->tcf_lock);
>  			ret = -ENOMEM;
> -			goto put_chain;
> +			goto out_release;

I'm a little unclear on why put_chain is no longer needed.
It seems to me that there can be a reference to goto_ch held here,
as was the case before this patch.

>  		}
> -		kfree(p->tcfp_keys);
> -		p->tcfp_keys = keys;
> -		p->tcfp_nkeys = parm->nkeys;
> +		nparms->tcfp_nkeys = parm->nkeys;
> +	} else {
> +		nparms->tcfp_keys = oparms->tcfp_keys;

I feel that I am missing something obvious:
* Here oparms->tcfp_keys is assigned to nparms->tcfp_keys.
* Later on there is a call to call_rcu(..., tcf_pedit_cleanup_rcu),
  which will free oparms->tcfp_keys some time in the future.
* But the memory bay still be accessed via tcfp_keys.

Is there a life cycle issue here?

> +		nparms->tcfp_nkeys = oparms->tcfp_nkeys;
>  	}
> -	memcpy(p->tcfp_keys, parm->keys, ksize);
> -	p->tcfp_off_max_hint = 0;
> -	for (i = 0; i < p->tcfp_nkeys; ++i) {
> -		u32 cur = p->tcfp_keys[i].off;
> +
> +	memcpy(nparms->tcfp_keys, parm->keys, ksize);
> +
> +	for (i = 0; i < nparms->tcfp_nkeys; ++i) {
> +		u32 cur = nparms->tcfp_keys[i].off;
>  
>  		/* sanitize the shift value for any later use */
> -		p->tcfp_keys[i].shift = min_t(size_t, BITS_PER_TYPE(int) - 1,
> -					      p->tcfp_keys[i].shift);
> +		nparms->tcfp_keys[i].shift = min_t(size_t,
> +						   BITS_PER_TYPE(int) - 1,
> +						   nparms->tcfp_keys[i].shift);
>  
>  		/* The AT option can read a single byte, we can bound the actual
>  		 * value with uchar max.
>  		 */
> -		cur += (0xff & p->tcfp_keys[i].offmask) >> p->tcfp_keys[i].shift;
> +		cur += (0xff & nparms->tcfp_keys[i].offmask) >> nparms->tcfp_keys[i].shift;
>  
>  		/* Each key touches 4 bytes starting from the computed offset */
> -		p->tcfp_off_max_hint = max(p->tcfp_off_max_hint, cur + 4);
> +		nparms->tcfp_off_max_hint =
> +			max(nparms->tcfp_off_max_hint, cur + 4);
>  	}
>  
> -	p->tcfp_flags = parm->flags;
>  	goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
>  
> -	kfree(p->tcfp_keys_ex);
> -	p->tcfp_keys_ex = keys_ex;
> +	rcu_assign_pointer(p->parms, nparms);
>  
>  	spin_unlock_bh(&p->tcf_lock);
> +
> +	if (oparms)
> +		call_rcu(&oparms->rcu, tcf_pedit_cleanup_rcu);

	Here there is a condition on oparms being non-NULL.
	But further above oparms is dereference unconditionally.
	Is there an inconsistency here?

> +
>  	if (goto_ch)
>  		tcf_chain_put_by_act(goto_ch);
> +
>  	return ret;
>  
> -put_chain:
> -	if (goto_ch)
> -		tcf_chain_put_by_act(goto_ch);
>  out_release:
>  	tcf_idr_release(*a, bind);
> +out_free_ex:
> +	kfree(nparms->tcfp_keys_ex);
>  out_free:
> -	kfree(keys_ex);
> +	kfree(nparms);
>  	return ret;
> -
>  }

...