On Thu, Feb 29, 2024 at 10:05 AM Paolo Abeni <pabeni@xxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxxxx>
> > Signed-off-by: Victor Nogueira <victor@xxxxxxxxxxxx>
> > Co-developed-by: Pedro Tammela <pctammela@xxxxxxxxxxxx>
> > Signed-off-by: Pedro Tammela <pctammela@xxxxxxxxxxxx>
> > Signed-off-by: Jamal Hadi Salim <jhs@xxxxxxxxxxxx>
> > Reviewed-by: Vlad Buslov <vladbu@xxxxxxxxxx>
> > Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@xxxxxxxxx>
> > ---
> > 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.
>
Sure - we can replace it. What's the preference? Spinlock?
cheers,
jamal
> Cheers,
>
> Paolo
>
