On Fri, Sep 2, 2022 at 7:22 AM Zhengchao Shao <shaozhengchao@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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
>
