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

[Jamal Hadi Salim <jhs@mojatatu.com>]

On Fri, Mar 1, 2024 at 2:30 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On Thu, 2024-02-29 at 13:21 -0500, Jamal Hadi Salim wrote:
> > On Thu, Feb 29, 2024 at 10:05 AM Paolo Abeni <pabeni@redhat.com> 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@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.
> > >
> >
> > Sure - we can replace it. What's the preference? Spinlock?
>
> Plain spinlock will work. Using spinlock + RCU should be quite straight
> forward and will provide faster lookup.
>

rcu + spinlock sounds like a bit of overkill but we'll look into it.

cheers,
jamal

> Cheers,
>
> Paolo
>