Based on previous patches of this patchset, the walk and lookup hooks in
the tc_action_ops structure are no longer used, and redundant code logic
branches should be removed. tcf_generic_walker() and tcf_idr_search() are
also used only in the act_api.c, change them to static.
Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com>
---
include/net/act_api.h | 10 ----------
net/sched/act_api.c | 42 ++++++++++++------------------------------
2 files changed, 12 insertions(+), 40 deletions(-)
diff --git a/include/net/act_api.h b/include/net/act_api.h
index a79d6e58519e..ebd84ef06b5b 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -118,15 +118,10 @@ struct tc_action_ops {
struct tcf_result *); /* called under RCU BH lock*/
int (*dump)(struct sk_buff *, struct tc_action *, int, int);
void (*cleanup)(struct tc_action *);
- int (*lookup)(struct net *net, struct tc_action **a, u32 index);
int (*init)(struct net *net, struct nlattr *nla,
struct nlattr *est, struct tc_action **act,
struct tcf_proto *tp,
u32 flags, struct netlink_ext_ack *extack);
- int (*walk)(struct net *, struct sk_buff *,
- struct netlink_callback *, int,
- const struct tc_action_ops *,
- struct netlink_ext_ack *);
void (*stats_update)(struct tc_action *, u64, u64, u64, u64, bool);
size_t (*get_fill_size)(const struct tc_action *act);
struct net_device *(*get_dev)(const struct tc_action *a,
@@ -178,11 +173,6 @@ static inline void tc_action_net_exit(struct list_head *net_list,
rtnl_unlock();
}
-int tcf_generic_walker(struct tc_action_net *tn, struct sk_buff *skb,
- struct netlink_callback *cb, int type,
- const struct tc_action_ops *ops,
- struct netlink_ext_ack *extack);
-int tcf_idr_search(struct tc_action_net *tn, struct tc_action **a, u32 index);
int tcf_idr_create(struct tc_action_net *tn, u32 index, struct nlattr *est,
struct tc_action **a, const struct tc_action_ops *ops,
int bind, bool cpustats, u32 flags);
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 7063d2004199..2d26aec25e3a 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -636,10 +636,10 @@ static int tcf_del_walker(struct tcf_idrinfo *idrinfo, struct sk_buff *skb,
return ret;
}
-int tcf_generic_walker(struct tc_action_net *tn, struct sk_buff *skb,
- struct netlink_callback *cb, int type,
- const struct tc_action_ops *ops,
- struct netlink_ext_ack *extack)
+static int tcf_generic_walker(struct tc_action_net *tn, struct sk_buff *skb,
+ struct netlink_callback *cb, int type,
+ const struct tc_action_ops *ops,
+ struct netlink_ext_ack *extack)
{
struct tcf_idrinfo *idrinfo = tn->idrinfo;
@@ -653,9 +653,8 @@ int tcf_generic_walker(struct tc_action_net *tn, struct sk_buff *skb,
return -EINVAL;
}
}
-EXPORT_SYMBOL(tcf_generic_walker);
-int tcf_idr_search(struct tc_action_net *tn, struct tc_action **a, u32 index)
+static int tcf_idr_search(struct tc_action_net *tn, struct tc_action **a, u32 index)
{
struct tcf_idrinfo *idrinfo = tn->idrinfo;
struct tc_action *p;
@@ -674,7 +673,6 @@ int tcf_idr_search(struct tc_action_net *tn, struct tc_action **a, u32 index)
}
return false;
}
-EXPORT_SYMBOL(tcf_idr_search);
static int __tcf_generic_walker(struct net *net, struct sk_buff *skb,
struct netlink_callback *cb, int type,
@@ -945,8 +943,7 @@ int tcf_register_action(struct tc_action_ops *act,
struct tc_action_ops *a;
int ret;
- if (!act->act || !act->dump || !act->init ||
- (!act->net_id && (!act->walk || !act->lookup)))
+ if (!act->act || !act->dump || !act->init || !act->net_id)
return -EINVAL;
/* We have to register pernet ops before making the action ops visible,
@@ -1658,16 +1655,10 @@ static struct tc_action *tcf_action_get_1(struct net *net, struct nlattr *nla,
goto err_out;
}
err = -ENOENT;
- if (ops->lookup) {
- if (ops->lookup(net, &a, index) == 0) {
- NL_SET_ERR_MSG(extack, "TC action with specified index not found");
- goto err_mod;
- }
- } else {
- if (__tcf_idr_search(net, ops, &a, index) == 0) {
- NL_SET_ERR_MSG(extack, "TC action with specified index not found");
- goto err_mod;
- }
+
+ if (__tcf_idr_search(net, ops, &a, index) == 0) {
+ NL_SET_ERR_MSG(extack, "TC action with specified index not found");
+ goto err_mod;
}
module_put(ops->owner);
@@ -1730,12 +1721,7 @@ static int tca_action_flush(struct net *net, struct nlattr *nla,
goto out_module_put;
}
- if (ops->walk) {
- err = ops->walk(net, skb, &dcb, RTM_DELACTION, ops, extack);
- } else {
- err = __tcf_generic_walker(net, skb, &dcb, RTM_DELACTION, ops, extack);
- }
-
+ err = __tcf_generic_walker(net, skb, &dcb, RTM_DELACTION, ops, extack);
if (err <= 0) {
nla_nest_cancel(skb, nest);
goto out_module_put;
@@ -2153,11 +2139,7 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
if (nest == NULL)
goto out_module_put;
- if (a_o->walk)
- ret = a_o->walk(net, skb, cb, RTM_GETACTION, a_o, NULL);
- else
- ret = __tcf_generic_walker(net, skb, cb, RTM_GETACTION, a_o, NULL);
-
+ ret = __tcf_generic_walker(net, skb, cb, RTM_GETACTION, a_o, NULL);
if (ret < 0)
goto out_module_put;
--
2.17.1On Fri, Sep 2, 2022 at 7:22 AM Zhengchao Shao <shaozhengchao@huawei.com> wrote:
>
> Based on previous patches of this patchset, the walk and lookup hooks in
> the tc_action_ops structure are no longer used, and redundant code logic
> branches should be removed. tcf_generic_walker() and tcf_idr_search() are
> also used only in the act_api.c, change them to static.
>
All the rest look good.
It is possible to have extra computation for a lookup/walk depending on the
action complexity. That was the point of those APIs; however, we could argue
that if a user shows up with those demands then we'll add them then.
If you want to remove the callbacks, why not do them in the first patch?
cheers,
jamal
> Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com>
> ---
> include/net/act_api.h | 10 ----------
> net/sched/act_api.c | 42 ++++++++++++------------------------------
> 2 files changed, 12 insertions(+), 40 deletions(-)
>
> diff --git a/include/net/act_api.h b/include/net/act_api.h
> index a79d6e58519e..ebd84ef06b5b 100644
> --- a/include/net/act_api.h
> +++ b/include/net/act_api.h
> @@ -118,15 +118,10 @@ struct tc_action_ops {
> struct tcf_result *); /* called under RCU BH lock*/
> int (*dump)(struct sk_buff *, struct tc_action *, int, int);
> void (*cleanup)(struct tc_action *);
> - int (*lookup)(struct net *net, struct tc_action **a, u32 index);
> int (*init)(struct net *net, struct nlattr *nla,
> struct nlattr *est, struct tc_action **act,
> struct tcf_proto *tp,
> u32 flags, struct netlink_ext_ack *extack);
> - int (*walk)(struct net *, struct sk_buff *,
> - struct netlink_callback *, int,
> - const struct tc_action_ops *,
> - struct netlink_ext_ack *);
> void (*stats_update)(struct tc_action *, u64, u64, u64, u64, bool);
> size_t (*get_fill_size)(const struct tc_action *act);
> struct net_device *(*get_dev)(const struct tc_action *a,
> @@ -178,11 +173,6 @@ static inline void tc_action_net_exit(struct list_head *net_list,
> rtnl_unlock();
> }
>
> -int tcf_generic_walker(struct tc_action_net *tn, struct sk_buff *skb,
> - struct netlink_callback *cb, int type,
> - const struct tc_action_ops *ops,
> - struct netlink_ext_ack *extack);
> -int tcf_idr_search(struct tc_action_net *tn, struct tc_action **a, u32 index);
> int tcf_idr_create(struct tc_action_net *tn, u32 index, struct nlattr *est,
> struct tc_action **a, const struct tc_action_ops *ops,
> int bind, bool cpustats, u32 flags);
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 7063d2004199..2d26aec25e3a 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -636,10 +636,10 @@ static int tcf_del_walker(struct tcf_idrinfo *idrinfo, struct sk_buff *skb,
> return ret;
> }
>
> -int tcf_generic_walker(struct tc_action_net *tn, struct sk_buff *skb,
> - struct netlink_callback *cb, int type,
> - const struct tc_action_ops *ops,
> - struct netlink_ext_ack *extack)
> +static int tcf_generic_walker(struct tc_action_net *tn, struct sk_buff *skb,
> + struct netlink_callback *cb, int type,
> + const struct tc_action_ops *ops,
> + struct netlink_ext_ack *extack)
> {
> struct tcf_idrinfo *idrinfo = tn->idrinfo;
>
> @@ -653,9 +653,8 @@ int tcf_generic_walker(struct tc_action_net *tn, struct sk_buff *skb,
> return -EINVAL;
> }
> }
> -EXPORT_SYMBOL(tcf_generic_walker);
>
> -int tcf_idr_search(struct tc_action_net *tn, struct tc_action **a, u32 index)
> +static int tcf_idr_search(struct tc_action_net *tn, struct tc_action **a, u32 index)
> {
> struct tcf_idrinfo *idrinfo = tn->idrinfo;
> struct tc_action *p;
> @@ -674,7 +673,6 @@ int tcf_idr_search(struct tc_action_net *tn, struct tc_action **a, u32 index)
> }
> return false;
> }
> -EXPORT_SYMBOL(tcf_idr_search);
>
> static int __tcf_generic_walker(struct net *net, struct sk_buff *skb,
> struct netlink_callback *cb, int type,
> @@ -945,8 +943,7 @@ int tcf_register_action(struct tc_action_ops *act,
> struct tc_action_ops *a;
> int ret;
>
> - if (!act->act || !act->dump || !act->init ||
> - (!act->net_id && (!act->walk || !act->lookup)))
> + if (!act->act || !act->dump || !act->init || !act->net_id)
> return -EINVAL;
>
> /* We have to register pernet ops before making the action ops visible,
> @@ -1658,16 +1655,10 @@ static struct tc_action *tcf_action_get_1(struct net *net, struct nlattr *nla,
> goto err_out;
> }
> err = -ENOENT;
> - if (ops->lookup) {
> - if (ops->lookup(net, &a, index) == 0) {
> - NL_SET_ERR_MSG(extack, "TC action with specified index not found");
> - goto err_mod;
> - }
> - } else {
> - if (__tcf_idr_search(net, ops, &a, index) == 0) {
> - NL_SET_ERR_MSG(extack, "TC action with specified index not found");
> - goto err_mod;
> - }
> +
> + if (__tcf_idr_search(net, ops, &a, index) == 0) {
> + NL_SET_ERR_MSG(extack, "TC action with specified index not found");
> + goto err_mod;
> }
>
> module_put(ops->owner);
> @@ -1730,12 +1721,7 @@ static int tca_action_flush(struct net *net, struct nlattr *nla,
> goto out_module_put;
> }
>
> - if (ops->walk) {
> - err = ops->walk(net, skb, &dcb, RTM_DELACTION, ops, extack);
> - } else {
> - err = __tcf_generic_walker(net, skb, &dcb, RTM_DELACTION, ops, extack);
> - }
> -
> + err = __tcf_generic_walker(net, skb, &dcb, RTM_DELACTION, ops, extack);
> if (err <= 0) {
> nla_nest_cancel(skb, nest);
> goto out_module_put;
> @@ -2153,11 +2139,7 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
> if (nest == NULL)
> goto out_module_put;
>
> - if (a_o->walk)
> - ret = a_o->walk(net, skb, cb, RTM_GETACTION, a_o, NULL);
> - else
> - ret = __tcf_generic_walker(net, skb, cb, RTM_GETACTION, a_o, NULL);
> -
> + ret = __tcf_generic_walker(net, skb, cb, RTM_GETACTION, a_o, NULL);
> if (ret < 0)
> goto out_module_put;
>
> --
> 2.17.1
>
On 2022/9/2 22:03, Jamal Hadi Salim wrote:
> On Fri, Sep 2, 2022 at 7:22 AM Zhengchao Shao <shaozhengchao@huawei.com> wrote:
>>
>> Based on previous patches of this patchset, the walk and lookup hooks in
>> the tc_action_ops structure are no longer used, and redundant code logic
>> branches should be removed. tcf_generic_walker() and tcf_idr_search() are
>> also used only in the act_api.c, change them to static.
>>
>
> All the rest look good.
> It is possible to have extra computation for a lookup/walk depending on the
> action complexity. That was the point of those APIs; however, we could argue
> that if a user shows up with those demands then we'll add them then.
> If you want to remove the callbacks, why not do them in the first patch?
>
> cheers,
> jamal
>
Hi jamal:
Thank you for your reply. I have test patchset v2 with TDC action
module, and they are all passed. Next, I will add some selftest in other
patchset.
I will reserve the walk and lookup interfaces and delete them only when
they are no longer used.
Zhengchao Shao
>> Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com>
>> ---
>> include/net/act_api.h | 10 ----------
>> net/sched/act_api.c | 42 ++++++++++++------------------------------
>> 2 files changed, 12 insertions(+), 40 deletions(-)
>>
>> diff --git a/include/net/act_api.h b/include/net/act_api.h
>> index a79d6e58519e..ebd84ef06b5b 100644
>> --- a/include/net/act_api.h
>> +++ b/include/net/act_api.h
>> @@ -118,15 +118,10 @@ struct tc_action_ops {
>> struct tcf_result *); /* called under RCU BH lock*/
>> int (*dump)(struct sk_buff *, struct tc_action *, int, int);
>> void (*cleanup)(struct tc_action *);
>> - int (*lookup)(struct net *net, struct tc_action **a, u32 index);
>> int (*init)(struct net *net, struct nlattr *nla,
>> struct nlattr *est, struct tc_action **act,
>> struct tcf_proto *tp,
>> u32 flags, struct netlink_ext_ack *extack);
>> - int (*walk)(struct net *, struct sk_buff *,
>> - struct netlink_callback *, int,
>> - const struct tc_action_ops *,
>> - struct netlink_ext_ack *);
>> void (*stats_update)(struct tc_action *, u64, u64, u64, u64, bool);
>> size_t (*get_fill_size)(const struct tc_action *act);
>> struct net_device *(*get_dev)(const struct tc_action *a,
>> @@ -178,11 +173,6 @@ static inline void tc_action_net_exit(struct list_head *net_list,
>> rtnl_unlock();
>> }
>>
>> -int tcf_generic_walker(struct tc_action_net *tn, struct sk_buff *skb,
>> - struct netlink_callback *cb, int type,
>> - const struct tc_action_ops *ops,
>> - struct netlink_ext_ack *extack);
>> -int tcf_idr_search(struct tc_action_net *tn, struct tc_action **a, u32 index);
>> int tcf_idr_create(struct tc_action_net *tn, u32 index, struct nlattr *est,
>> struct tc_action **a, const struct tc_action_ops *ops,
>> int bind, bool cpustats, u32 flags);
>> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>> index 7063d2004199..2d26aec25e3a 100644
>> --- a/net/sched/act_api.c
>> +++ b/net/sched/act_api.c
>> @@ -636,10 +636,10 @@ static int tcf_del_walker(struct tcf_idrinfo *idrinfo, struct sk_buff *skb,
>> return ret;
>> }
>>
>> -int tcf_generic_walker(struct tc_action_net *tn, struct sk_buff *skb,
>> - struct netlink_callback *cb, int type,
>> - const struct tc_action_ops *ops,
>> - struct netlink_ext_ack *extack)
>> +static int tcf_generic_walker(struct tc_action_net *tn, struct sk_buff *skb,
>> + struct netlink_callback *cb, int type,
>> + const struct tc_action_ops *ops,
>> + struct netlink_ext_ack *extack)
>> {
>> struct tcf_idrinfo *idrinfo = tn->idrinfo;
>>
>> @@ -653,9 +653,8 @@ int tcf_generic_walker(struct tc_action_net *tn, struct sk_buff *skb,
>> return -EINVAL;
>> }
>> }
>> -EXPORT_SYMBOL(tcf_generic_walker);
>>
>> -int tcf_idr_search(struct tc_action_net *tn, struct tc_action **a, u32 index)
>> +static int tcf_idr_search(struct tc_action_net *tn, struct tc_action **a, u32 index)
>> {
>> struct tcf_idrinfo *idrinfo = tn->idrinfo;
>> struct tc_action *p;
>> @@ -674,7 +673,6 @@ int tcf_idr_search(struct tc_action_net *tn, struct tc_action **a, u32 index)
>> }
>> return false;
>> }
>> -EXPORT_SYMBOL(tcf_idr_search);
>>
>> static int __tcf_generic_walker(struct net *net, struct sk_buff *skb,
>> struct netlink_callback *cb, int type,
>> @@ -945,8 +943,7 @@ int tcf_register_action(struct tc_action_ops *act,
>> struct tc_action_ops *a;
>> int ret;
>>
>> - if (!act->act || !act->dump || !act->init ||
>> - (!act->net_id && (!act->walk || !act->lookup)))
>> + if (!act->act || !act->dump || !act->init || !act->net_id)
>> return -EINVAL;
>>
>> /* We have to register pernet ops before making the action ops visible,
>> @@ -1658,16 +1655,10 @@ static struct tc_action *tcf_action_get_1(struct net *net, struct nlattr *nla,
>> goto err_out;
>> }
>> err = -ENOENT;
>> - if (ops->lookup) {
>> - if (ops->lookup(net, &a, index) == 0) {
>> - NL_SET_ERR_MSG(extack, "TC action with specified index not found");
>> - goto err_mod;
>> - }
>> - } else {
>> - if (__tcf_idr_search(net, ops, &a, index) == 0) {
>> - NL_SET_ERR_MSG(extack, "TC action with specified index not found");
>> - goto err_mod;
>> - }
>> +
>> + if (__tcf_idr_search(net, ops, &a, index) == 0) {
>> + NL_SET_ERR_MSG(extack, "TC action with specified index not found");
>> + goto err_mod;
>> }
>>
>> module_put(ops->owner);
>> @@ -1730,12 +1721,7 @@ static int tca_action_flush(struct net *net, struct nlattr *nla,
>> goto out_module_put;
>> }
>>
>> - if (ops->walk) {
>> - err = ops->walk(net, skb, &dcb, RTM_DELACTION, ops, extack);
>> - } else {
>> - err = __tcf_generic_walker(net, skb, &dcb, RTM_DELACTION, ops, extack);
>> - }
>> -
>> + err = __tcf_generic_walker(net, skb, &dcb, RTM_DELACTION, ops, extack);
>> if (err <= 0) {
>> nla_nest_cancel(skb, nest);
>> goto out_module_put;
>> @@ -2153,11 +2139,7 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
>> if (nest == NULL)
>> goto out_module_put;
>>
>> - if (a_o->walk)
>> - ret = a_o->walk(net, skb, cb, RTM_GETACTION, a_o, NULL);
>> - else
>> - ret = __tcf_generic_walker(net, skb, cb, RTM_GETACTION, a_o, NULL);
>> -
>> + ret = __tcf_generic_walker(net, skb, cb, RTM_GETACTION, a_o, NULL);
>> if (ret < 0)
>> goto out_module_put;
>>
>> --
>> 2.17.1
>>
>
© 2016 - 2026 Red Hat, Inc.