Re: [PATCH net-next v12 01/15] net: sched: act_api: Introduce P4 actions list

[Paolo Abeni <pabeni@redhat.com>]

On Sun, 2024-02-25 at 11:54 -0500, Jamal Hadi Salim wrote:
> In P4 we require to generate new actions "on the fly" based on the
> specified P4 action definition. P4 action kinds, like the pipeline
> they are attached to, must be per net namespace, as opposed to native
> action kinds which are global. For that reason, we chose to create a
> separate structure to store P4 actions.
> 
> 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>
> Reviewed-by: Vlad Buslov <vladbu@nvidia.com>
> Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> ---
>  include/net/act_api.h |   8 ++-
>  net/sched/act_api.c   | 123 +++++++++++++++++++++++++++++++++++++-----
>  net/sched/cls_api.c   |   2 +-
>  3 files changed, 116 insertions(+), 17 deletions(-)
> 
> diff --git a/include/net/act_api.h b/include/net/act_api.h
> index 77ee0c657..f22be14bb 100644
> --- a/include/net/act_api.h
> +++ b/include/net/act_api.h
> @@ -105,6 +105,7 @@ typedef void (*tc_action_priv_destructor)(void *priv);
>  
>  struct tc_action_ops {
>  	struct list_head head;
> +	struct list_head p4_head;
>  	char    kind[IFNAMSIZ];
>  	enum tca_id  id; /* identifier should match kind */
>  	unsigned int	net_id;
> @@ -199,10 +200,12 @@ int tcf_idr_check_alloc(struct tc_action_net *tn, u32 *index,
>  int tcf_idr_release(struct tc_action *a, bool bind);
>  
>  int tcf_register_action(struct tc_action_ops *a, struct pernet_operations *ops);
> +int tcf_register_p4_action(struct net *net, struct tc_action_ops *act);
>  int tcf_unregister_action(struct tc_action_ops *a,
>  			  struct pernet_operations *ops);
>  #define NET_ACT_ALIAS_PREFIX "net-act-"
>  #define MODULE_ALIAS_NET_ACT(kind)	MODULE_ALIAS(NET_ACT_ALIAS_PREFIX kind)
> +void tcf_unregister_p4_action(struct net *net, struct tc_action_ops *act);
>  int tcf_action_destroy(struct tc_action *actions[], int bind);
>  int tcf_action_exec(struct sk_buff *skb, struct tc_action **actions,
>  		    int nr_actions, struct tcf_result *res);
> @@ -210,8 +213,9 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
>  		    struct nlattr *est,
>  		    struct tc_action *actions[], int init_res[], size_t *attr_size,
>  		    u32 flags, u32 fl_flags, struct netlink_ext_ack *extack);
> -struct tc_action_ops *tc_action_load_ops(struct nlattr *nla, u32 flags,
> -					 struct netlink_ext_ack *extack);
> +struct tc_action_ops *
> +tc_action_load_ops(struct net *net, struct nlattr *nla,
> +		   u32 flags, struct netlink_ext_ack *extack);
>  struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
>  				    struct nlattr *nla, struct nlattr *est,
>  				    struct tc_action_ops *a_o, int *init_res,
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 9ee622fb1..23ef394f2 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -57,6 +57,40 @@ static void tcf_free_cookie_rcu(struct rcu_head *p)
>  	kfree(cookie);
>  }
>  
> +static unsigned int p4_act_net_id;
> +
> +struct tcf_p4_act_net {
> +	struct list_head act_base;
> +	rwlock_t act_mod_lock;

Note that rwlock in networking code is discouraged, as they have to be
unfair, see commit 0daf07e527095e64ee8927ce297ab626643e9f51.

In this specific case I think there should be no problems, as is
extremely hard/impossible to have serious contention on the write
side,. Also there is already an existing rwlock nearby, no not a
blocker but IMHO worthy to be noted.

Cheers,

Paolo