| Message ID | 20240225165447.156954-4-jhs@mojatatu.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Introducing P4TC (series 1) | expand |
On Sun, 2024-02-25 at 11:54 -0500, Jamal Hadi Salim wrote: > The initialisation of P4TC action instances require access to a struct > p4tc_act (which appears in later patches) to help us to retrieve > information like the P4 action parameters etc. In order to retrieve > struct p4tc_act we need the pipeline name or id and the action name or id. > Also recall that P4TC action IDs are P4 and are net namespace specific and > not global like standard tc actions. > The init callback from tc_action_ops parameters had no way of > supplying us that information. To solve this issue, we decided to create a > new tc_action_ops callback (init_ops), that provies us with the > tc_action_ops struct which then provides us with the pipeline and action > name. The new init ops looks a bit unfortunate. I *think* it would be better adding the new argument to the existing init op > In addition we add a new refcount to struct tc_action_ops called > dyn_ref, which accounts for how many action instances we have of a specific > action. > > 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 | 6 ++++++ > net/sched/act_api.c | 14 +++++++++++--- > 2 files changed, 17 insertions(+), 3 deletions(-) > > diff --git a/include/net/act_api.h b/include/net/act_api.h > index c839ff57c..69be5ed83 100644 > --- a/include/net/act_api.h > +++ b/include/net/act_api.h > @@ -109,6 +109,7 @@ struct tc_action_ops { > char kind[ACTNAMSIZ]; > enum tca_id id; /* identifier should match kind */ > unsigned int net_id; > + refcount_t p4_ref; > size_t size; > struct module *owner; > int (*act)(struct sk_buff *, const struct tc_action *, > @@ -120,6 +121,11 @@ struct tc_action_ops { > struct nlattr *est, struct tc_action **act, > struct tcf_proto *tp, > u32 flags, struct netlink_ext_ack *extack); > + /* This should be merged with the original init action */ > + int (*init_ops)(struct net *net, struct nlattr *nla, > + struct nlattr *est, struct tc_action **act, > + struct tcf_proto *tp, struct tc_action_ops *ops, shouldn't the 'ops' argument be 'const'? Thanks, Paolo
On Thu, Feb 29, 2024 at 11:19 AM Paolo Abeni <pabeni@redhat.com> wrote: > > On Sun, 2024-02-25 at 11:54 -0500, Jamal Hadi Salim wrote: > > The initialisation of P4TC action instances require access to a struct > > p4tc_act (which appears in later patches) to help us to retrieve > > information like the P4 action parameters etc. In order to retrieve > > struct p4tc_act we need the pipeline name or id and the action name or id. > > Also recall that P4TC action IDs are P4 and are net namespace specific and > > not global like standard tc actions. > > The init callback from tc_action_ops parameters had no way of > > supplying us that information. To solve this issue, we decided to create a > > new tc_action_ops callback (init_ops), that provies us with the > > tc_action_ops struct which then provides us with the pipeline and action > > name. > > The new init ops looks a bit unfortunate. I *think* it would be better > adding the new argument to the existing init op > Our initial goal was to avoid creating a much larger patch by changing any other action's code and we observe that ->init() already has 8 params already ;-> And only dynamic actions need this extra extension. If you still feel the change is needed, sure we can make that change. > > In addition we add a new refcount to struct tc_action_ops called > > dyn_ref, which accounts for how many action instances we have of a specific > > action. > > > > 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 | 6 ++++++ > > net/sched/act_api.c | 14 +++++++++++--- > > 2 files changed, 17 insertions(+), 3 deletions(-) > > > > diff --git a/include/net/act_api.h b/include/net/act_api.h > > index c839ff57c..69be5ed83 100644 > > --- a/include/net/act_api.h > > +++ b/include/net/act_api.h > > @@ -109,6 +109,7 @@ struct tc_action_ops { > > char kind[ACTNAMSIZ]; > > enum tca_id id; /* identifier should match kind */ > > unsigned int net_id; > > + refcount_t p4_ref; > > size_t size; > > struct module *owner; > > int (*act)(struct sk_buff *, const struct tc_action *, > > @@ -120,6 +121,11 @@ struct tc_action_ops { > > struct nlattr *est, struct tc_action **act, > > struct tcf_proto *tp, > > u32 flags, struct netlink_ext_ack *extack); > > + /* This should be merged with the original init action */ > > + int (*init_ops)(struct net *net, struct nlattr *nla, > > + struct nlattr *est, struct tc_action **act, > > + struct tcf_proto *tp, struct tc_action_ops *ops, > > shouldn't the 'ops' argument be 'const'? > As it is right now this would be hard to do because we carry around a refcnt in that struct. We will think about it.. cheers, jamal > Thanks, > > Paolo >
diff --git a/include/net/act_api.h b/include/net/act_api.h index c839ff57c..69be5ed83 100644 --- a/include/net/act_api.h +++ b/include/net/act_api.h @@ -109,6 +109,7 @@ struct tc_action_ops { char kind[ACTNAMSIZ]; enum tca_id id; /* identifier should match kind */ unsigned int net_id; + refcount_t p4_ref; size_t size; struct module *owner; int (*act)(struct sk_buff *, const struct tc_action *, @@ -120,6 +121,11 @@ struct tc_action_ops { struct nlattr *est, struct tc_action **act, struct tcf_proto *tp, u32 flags, struct netlink_ext_ack *extack); + /* This should be merged with the original init action */ + int (*init_ops)(struct net *net, struct nlattr *nla, + struct nlattr *est, struct tc_action **act, + struct tcf_proto *tp, struct tc_action_ops *ops, + u32 flags, struct netlink_ext_ack *extack); int (*walk)(struct net *, struct sk_buff *, struct netlink_callback *, int, const struct tc_action_ops *, diff --git a/net/sched/act_api.c b/net/sched/act_api.c index ce10d2c6e..3d1fb8da1 100644 --- a/net/sched/act_api.c +++ b/net/sched/act_api.c @@ -1044,7 +1044,7 @@ int tcf_register_action(struct tc_action_ops *act, struct tc_action_ops *a; int ret; - if (!act->act || !act->dump || !act->init) + if (!act->act || !act->dump || (!act->init && !act->init_ops)) return -EINVAL; /* We have to register pernet ops before making the action ops visible, @@ -1517,8 +1517,16 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp, } } - err = a_o->init(net, tb[TCA_ACT_OPTIONS], est, &a, tp, - userflags.value | flags, extack); + /* When we arrive here we guarantee that a_o->init or + * a_o->init_ops exist. + */ + if (a_o->init) + err = a_o->init(net, tb[TCA_ACT_OPTIONS], est, &a, tp, + userflags.value | flags, extack); + else + err = a_o->init_ops(net, tb[TCA_ACT_OPTIONS], est, &a, + tp, a_o, userflags.value | flags, + extack); } else { err = a_o->init(net, nla, est, &a, tp, userflags.value | flags, extack);