Re: [PATCH net-next RFC 01/20] net/sched: act_api: change act_base into an IDR

[Vlad Buslov <vladbu@nvidia.com>]

On Tue 24 Jan 2023 at 12:04, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> Convert act_base from a list to an IDR.
>
> With the introduction of P4TC action templates, we introduce the concept of
> dynamically creating actions on the fly. Dynamic action IDs are not statically
> defined (as was the case previously) and are therefore harder to manage within
> existing linked list approach. We convert to IDR because it has built in ID
> management which we would have to re-invent with linked lists.
>
> Co-developed-by: Victor Nogueira <victor@mojatatu.com>
> Signed-off-by: Victor Nogueira <victor@mojatatu.com>
> Co-developed-by: Pedro Tammela <pctammela@mojatatu.com>
> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
> ---
>  include/uapi/linux/pkt_cls.h |  1 +
>  net/sched/act_api.c          | 39 +++++++++++++++++++++---------------
>  2 files changed, 24 insertions(+), 16 deletions(-)
>
> diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
> index 648a82f32..4d716841c 100644
> --- a/include/uapi/linux/pkt_cls.h
> +++ b/include/uapi/linux/pkt_cls.h
> @@ -139,6 +139,7 @@ enum tca_id {
>  	TCA_ID_MPLS,
>  	TCA_ID_CT,
>  	TCA_ID_GATE,
> +	TCA_ID_DYN,
>  	/* other actions go here */
>  	__TCA_ID_MAX = 255
>  };
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index cd09ef49d..811dddc3b 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -890,7 +890,7 @@ void tcf_idrinfo_destroy(const struct tc_action_ops *ops,
>  }
>  EXPORT_SYMBOL(tcf_idrinfo_destroy);
>  
> -static LIST_HEAD(act_base);
> +static DEFINE_IDR(act_base);
>  static DEFINE_RWLOCK(act_mod_lock);
>  /* since act ops id is stored in pernet subsystem list,
>   * then there is no way to walk through only all the action
> @@ -949,7 +949,6 @@ static void tcf_pernet_del_id_list(unsigned int id)
>  int tcf_register_action(struct tc_action_ops *act,
>  			struct pernet_operations *ops)
>  {
> -	struct tc_action_ops *a;
>  	int ret;
>  
>  	if (!act->act || !act->dump || !act->init)
> @@ -970,13 +969,24 @@ int tcf_register_action(struct tc_action_ops *act,
>  	}
>  
>  	write_lock(&act_mod_lock);
> -	list_for_each_entry(a, &act_base, head) {
> -		if (act->id == a->id || (strcmp(act->kind, a->kind) == 0)) {
> +	if (act->id) {
> +		if (idr_find(&act_base, act->id)) {
>  			ret = -EEXIST;
>  			goto err_out;
>  		}
> +		ret = idr_alloc_u32(&act_base, act, &act->id, act->id,
> +				    GFP_ATOMIC);
> +		if (ret < 0)
> +			goto err_out;
> +	} else {
> +		/* Only dynamic actions will require ID generation */
> +		act->id = TCA_ID_DYN;

Hi Jamal,

Since TCA_ID_DYN is exposed to userspace and this code expects to use
the whole range of [TCA_ID_DYN, TCA_ID_MAX] for dynamic actions any new
action added after that will have two choices:

- Insert future TCA_ID_*NEW_ACTION* before TCA_ID_DYN in the enum tca_id
in order for this code to continue to work (probably breaking userspace
code compiled for previous kernels).

- Modify this code to allocate dynamic action id from empty range
following new action enum value, which is not ideal.

Maybe consider defining TCA_ID_DYN=128 in order to leave some space for
new actions to be added before it without affecting the userspace?

> +
> +		ret = idr_alloc_u32(&act_base, act, &act->id, TCA_ID_MAX,
> +				    GFP_ATOMIC);
> +		if (ret < 0)
> +			goto err_out;
>  	}
> -	list_add_tail(&act->head, &act_base);
>  	write_unlock(&act_mod_lock);
>  
>  	return 0;
> @@ -994,17 +1004,12 @@ EXPORT_SYMBOL(tcf_register_action);
>  int tcf_unregister_action(struct tc_action_ops *act,
>  			  struct pernet_operations *ops)
>  {
> -	struct tc_action_ops *a;
> -	int err = -ENOENT;
> +	int err = 0;
>  
>  	write_lock(&act_mod_lock);
> -	list_for_each_entry(a, &act_base, head) {
> -		if (a == act) {
> -			list_del(&act->head);
> -			err = 0;
> -			break;
> -		}
> -	}
> +	if (!idr_remove(&act_base, act->id))
> +		err = -EINVAL;
> +
>  	write_unlock(&act_mod_lock);
>  	if (!err) {
>  		unregister_pernet_subsys(ops);
> @@ -1019,10 +1024,11 @@ EXPORT_SYMBOL(tcf_unregister_action);
>  static struct tc_action_ops *tc_lookup_action_n(char *kind)
>  {
>  	struct tc_action_ops *a, *res = NULL;
> +	unsigned long tmp, id;
>  
>  	if (kind) {
>  		read_lock(&act_mod_lock);
> -		list_for_each_entry(a, &act_base, head) {
> +		idr_for_each_entry_ul(&act_base, a, tmp, id) {
>  			if (strcmp(kind, a->kind) == 0) {
>  				if (try_module_get(a->owner))
>  					res = a;
> @@ -1038,10 +1044,11 @@ static struct tc_action_ops *tc_lookup_action_n(char *kind)
>  static struct tc_action_ops *tc_lookup_action(struct nlattr *kind)
>  {
>  	struct tc_action_ops *a, *res = NULL;
> +	unsigned long tmp, id;
>  
>  	if (kind) {
>  		read_lock(&act_mod_lock);
> -		list_for_each_entry(a, &act_base, head) {
> +		idr_for_each_entry_ul(&act_base, a, tmp, id) {
>  			if (nla_strcmp(kind, a->kind) == 0) {
>  				if (try_module_get(a->owner))
>  					res = a;