| Message ID | 20230124170510.316970-1-jhs@mojatatu.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Introducing P4TC | expand |
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;
Good catch Vlad. Maybe we can pick 256 to be safe. cheers, jamal On Wed, Jan 25, 2023 at 11:57 AM Vlad Buslov <vladbu@nvidia.com> wrote: > > > 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; >
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; + + 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;