| Message ID | 20231204203907.413435-5-pctammela@mojatatu.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net/sched: conditional notification of events for cls and act | expand |
Mon, Dec 04, 2023 at 09:39:06PM CET, pctammela@mojatatu.com wrote: >As of today tc-action events are unconditionally built and sent to >RTNLGRP_TC. As with the introduction of tc_should_notify we can check >before-hand if they are really needed. > >Signed-off-by: Pedro Tammela <pctammela@mojatatu.com> >--- > net/sched/act_api.c | 105 ++++++++++++++++++++++++++++++++------------ > 1 file changed, 76 insertions(+), 29 deletions(-) > >diff --git a/net/sched/act_api.c b/net/sched/act_api.c >index c39252d61ebb..55c62a8e8803 100644 >--- a/net/sched/act_api.c >+++ b/net/sched/act_api.c >@@ -1780,31 +1780,45 @@ static int tcf_action_delete(struct net *net, struct tc_action *actions[]) > return 0; > } > >-static int >-tcf_reoffload_del_notify(struct net *net, struct tc_action *action) >+static struct sk_buff *tcf_reoffload_del_notify_msg(struct net *net, I wonder, why this new function is needed? If I'm reading things correctly, tcf_reoffload_del_notify() with added check would be just ok, woundn't it? Same for others. >+ struct tc_action *action) > { > size_t attr_size = tcf_action_fill_size(action); > struct tc_action *actions[TCA_ACT_MAX_PRIO] = { > [0] = action, > }; >- const struct tc_action_ops *ops = action->ops; > struct sk_buff *skb; >- int ret; > >- skb = alloc_skb(attr_size <= NLMSG_GOODSIZE ? NLMSG_GOODSIZE : attr_size, >- GFP_KERNEL); >+ skb = alloc_skb(max(attr_size, NLMSG_GOODSIZE), GFP_KERNEL); I don't see how this is related to this patch. Can't you do it in separate patch? Same for others. > if (!skb) >- return -ENOBUFS; >+ return ERR_PTR(-ENOBUFS); > > if (tca_get_fill(skb, actions, 0, 0, 0, RTM_DELACTION, 0, 1, NULL) <= 0) { > kfree_skb(skb); >- return -EINVAL; >+ return ERR_PTR(-EINVAL); > } > >+ return skb; >+} >+ [...]
On 05/12/2023 08:32, Jiri Pirko wrote: > Mon, Dec 04, 2023 at 09:39:06PM CET, pctammela@mojatatu.com wrote: >> As of today tc-action events are unconditionally built and sent to >> RTNLGRP_TC. As with the introduction of tc_should_notify we can check >> before-hand if they are really needed. >> >> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com> >> --- >> net/sched/act_api.c | 105 ++++++++++++++++++++++++++++++++------------ >> 1 file changed, 76 insertions(+), 29 deletions(-) >> >> diff --git a/net/sched/act_api.c b/net/sched/act_api.c >> index c39252d61ebb..55c62a8e8803 100644 >> --- a/net/sched/act_api.c >> +++ b/net/sched/act_api.c >> @@ -1780,31 +1780,45 @@ static int tcf_action_delete(struct net *net, struct tc_action *actions[]) >> return 0; >> } >> >> -static int >> -tcf_reoffload_del_notify(struct net *net, struct tc_action *action) >> +static struct sk_buff *tcf_reoffload_del_notify_msg(struct net *net, > > I wonder, why this new function is needed? If I'm reading things > correctly, tcf_reoffload_del_notify() with added check would be just ok, > woundn't it? > > Same for others. In V1 we had it like what you are suggesting[1]. Jakub suggested to refactor the functions a bit more. The main argument was the code duplication introduced for the delete routines. Note that for the case that no notification is needed, we still need to do the action delete etc... I agree that code duplication is bad in the long term, so I did the changes, but I don't have a strong opinion here (either way is fine for me). Let's see what he has to say, perhaps I overdid what he was suggesting :) [1] https://lore.kernel.org/all/20231201204314.220543-4-pctammela@mojatatu.com/ > >> + struct tc_action *action) >> { >> size_t attr_size = tcf_action_fill_size(action); >> struct tc_action *actions[TCA_ACT_MAX_PRIO] = { >> [0] = action, >> }; >> - const struct tc_action_ops *ops = action->ops; >> struct sk_buff *skb; >> - int ret; >> >> - skb = alloc_skb(attr_size <= NLMSG_GOODSIZE ? NLMSG_GOODSIZE : attr_size, >> - GFP_KERNEL); >> + skb = alloc_skb(max(attr_size, NLMSG_GOODSIZE), GFP_KERNEL); > > I don't see how this is related to this patch. Can't you do it in separate > patch? > > Same for others. Sure, will split it out. > >> if (!skb) >> - return -ENOBUFS; >> + return ERR_PTR(-ENOBUFS); >> >> if (tca_get_fill(skb, actions, 0, 0, 0, RTM_DELACTION, 0, 1, NULL) <= 0) { >> kfree_skb(skb); >> - return -EINVAL; >> + return ERR_PTR(-EINVAL); >> } >> >> + return skb; >> +} >> + > > [...]
Tue, Dec 05, 2023 at 03:45:52PM CET, pctammela@mojatatu.com wrote: >On 05/12/2023 08:32, Jiri Pirko wrote: >> Mon, Dec 04, 2023 at 09:39:06PM CET, pctammela@mojatatu.com wrote: >> > As of today tc-action events are unconditionally built and sent to >> > RTNLGRP_TC. As with the introduction of tc_should_notify we can check >> > before-hand if they are really needed. >> > >> > Signed-off-by: Pedro Tammela <pctammela@mojatatu.com> >> > --- >> > net/sched/act_api.c | 105 ++++++++++++++++++++++++++++++++------------ >> > 1 file changed, 76 insertions(+), 29 deletions(-) >> > >> > diff --git a/net/sched/act_api.c b/net/sched/act_api.c >> > index c39252d61ebb..55c62a8e8803 100644 >> > --- a/net/sched/act_api.c >> > +++ b/net/sched/act_api.c >> > @@ -1780,31 +1780,45 @@ static int tcf_action_delete(struct net *net, struct tc_action *actions[]) >> > return 0; >> > } >> > >> > -static int >> > -tcf_reoffload_del_notify(struct net *net, struct tc_action *action) >> > +static struct sk_buff *tcf_reoffload_del_notify_msg(struct net *net, >> >> I wonder, why this new function is needed? If I'm reading things >> correctly, tcf_reoffload_del_notify() with added check would be just ok, >> woundn't it? >> >> Same for others. > >In V1 we had it like what you are suggesting[1]. >Jakub suggested to refactor the functions a bit more. The main argument was >the code duplication introduced for the delete routines. Okay. >Note that for the case that no notification is needed, we still need to do >the action delete etc... >I agree that code duplication is bad in the long term, so I did the changes, >but I don't have a strong opinion here (either way is fine for me). >Let's see what he has to say, perhaps I overdid what he was suggesting :) > >[1] >https://lore.kernel.org/all/20231201204314.220543-4-pctammela@mojatatu.com/ > >> >> > + struct tc_action *action) >> > { >> > size_t attr_size = tcf_action_fill_size(action); >> > struct tc_action *actions[TCA_ACT_MAX_PRIO] = { >> > [0] = action, >> > }; >> > - const struct tc_action_ops *ops = action->ops; >> > struct sk_buff *skb; >> > - int ret; >> > >> > - skb = alloc_skb(attr_size <= NLMSG_GOODSIZE ? NLMSG_GOODSIZE : attr_size, >> > - GFP_KERNEL); >> > + skb = alloc_skb(max(attr_size, NLMSG_GOODSIZE), GFP_KERNEL); >> >> I don't see how this is related to this patch. Can't you do it in separate >> patch? >> >> Same for others. > >Sure, will split it out. > >> >> > if (!skb) >> > - return -ENOBUFS; >> > + return ERR_PTR(-ENOBUFS); >> > >> > if (tca_get_fill(skb, actions, 0, 0, 0, RTM_DELACTION, 0, 1, NULL) <= 0) { >> > kfree_skb(skb); >> > - return -EINVAL; >> > + return ERR_PTR(-EINVAL); >> > } >> > >> > + return skb; >> > +} >> > + >> >> [...] >
diff --git a/net/sched/act_api.c b/net/sched/act_api.c index c39252d61ebb..55c62a8e8803 100644 --- a/net/sched/act_api.c +++ b/net/sched/act_api.c @@ -1780,31 +1780,45 @@ static int tcf_action_delete(struct net *net, struct tc_action *actions[]) return 0; } -static int -tcf_reoffload_del_notify(struct net *net, struct tc_action *action) +static struct sk_buff *tcf_reoffload_del_notify_msg(struct net *net, + struct tc_action *action) { size_t attr_size = tcf_action_fill_size(action); struct tc_action *actions[TCA_ACT_MAX_PRIO] = { [0] = action, }; - const struct tc_action_ops *ops = action->ops; struct sk_buff *skb; - int ret; - skb = alloc_skb(attr_size <= NLMSG_GOODSIZE ? NLMSG_GOODSIZE : attr_size, - GFP_KERNEL); + skb = alloc_skb(max(attr_size, NLMSG_GOODSIZE), GFP_KERNEL); if (!skb) - return -ENOBUFS; + return ERR_PTR(-ENOBUFS); if (tca_get_fill(skb, actions, 0, 0, 0, RTM_DELACTION, 0, 1, NULL) <= 0) { kfree_skb(skb); - return -EINVAL; + return ERR_PTR(-EINVAL); } + return skb; +} + +static int tcf_reoffload_del_notify(struct net *net, struct tc_action *action) +{ + const struct tc_action_ops *ops = action->ops; + struct sk_buff *skb = NULL; + int ret; + + if (!rtnl_notify_needed(net, 0, RTNLGRP_TC)) + goto skip_msg; + + skb = tcf_reoffload_del_notify_msg(net, action); + if (IS_ERR(skb)) + return PTR_ERR(skb); + +skip_msg: ret = tcf_idr_release_unsafe(action); if (ret == ACT_P_DELETED) { module_put(ops->owner); - ret = rtnetlink_send(skb, net, 0, RTNLGRP_TC, 0); + ret = rtnetlink_maybe_send(skb, net, 0, RTNLGRP_TC, 0); } else { kfree_skb(skb); } @@ -1870,25 +1884,42 @@ int tcf_action_reoffload_cb(flow_indr_block_bind_cb_t *cb, return 0; } -static int -tcf_del_notify(struct net *net, struct nlmsghdr *n, struct tc_action *actions[], - u32 portid, size_t attr_size, struct netlink_ext_ack *extack) +static struct sk_buff *tcf_del_notify_msg(struct net *net, struct nlmsghdr *n, + struct tc_action *actions[], + u32 portid, size_t attr_size, + struct netlink_ext_ack *extack) { - int ret; struct sk_buff *skb; - skb = alloc_skb(attr_size <= NLMSG_GOODSIZE ? NLMSG_GOODSIZE : attr_size, - GFP_KERNEL); + skb = alloc_skb(max(attr_size, NLMSG_GOODSIZE), GFP_KERNEL); if (!skb) - return -ENOBUFS; + return ERR_PTR(-ENOBUFS); if (tca_get_fill(skb, actions, portid, n->nlmsg_seq, 0, RTM_DELACTION, 0, 2, extack) <= 0) { NL_SET_ERR_MSG(extack, "Failed to fill netlink TC action attributes"); kfree_skb(skb); - return -EINVAL; + return ERR_PTR(-EINVAL); } + return skb; +} + +static int tcf_del_notify(struct net *net, struct nlmsghdr *n, + struct tc_action *actions[], u32 portid, + size_t attr_size, struct netlink_ext_ack *extack) +{ + struct sk_buff *skb = NULL; + int ret; + + if (!rtnl_notify_needed(net, n->nlmsg_flags, RTNLGRP_TC)) + goto skip_msg; + + skb = tcf_del_notify_msg(net, n, actions, portid, attr_size, extack); + if (IS_ERR(skb)) + return PTR_ERR(skb); + +skip_msg: /* now do the delete */ ret = tcf_action_delete(net, actions); if (ret < 0) { @@ -1897,9 +1928,8 @@ tcf_del_notify(struct net *net, struct nlmsghdr *n, struct tc_action *actions[], return ret; } - ret = rtnetlink_send(skb, net, portid, RTNLGRP_TC, - n->nlmsg_flags & NLM_F_ECHO); - return ret; + return rtnetlink_maybe_send(skb, net, portid, RTNLGRP_TC, + n->nlmsg_flags & NLM_F_ECHO); } static int @@ -1950,26 +1980,43 @@ tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n, return ret; } -static int -tcf_add_notify(struct net *net, struct nlmsghdr *n, struct tc_action *actions[], - u32 portid, size_t attr_size, struct netlink_ext_ack *extack) +static struct sk_buff *tcf_add_notify_msg(struct net *net, struct nlmsghdr *n, + struct tc_action *actions[], + u32 portid, size_t attr_size, + struct netlink_ext_ack *extack) { struct sk_buff *skb; - skb = alloc_skb(attr_size <= NLMSG_GOODSIZE ? NLMSG_GOODSIZE : attr_size, - GFP_KERNEL); + skb = alloc_skb(max(attr_size, NLMSG_GOODSIZE), GFP_KERNEL); if (!skb) - return -ENOBUFS; + return ERR_PTR(-ENOBUFS); if (tca_get_fill(skb, actions, portid, n->nlmsg_seq, n->nlmsg_flags, RTM_NEWACTION, 0, 0, extack) <= 0) { NL_SET_ERR_MSG(extack, "Failed to fill netlink attributes while adding TC action"); kfree_skb(skb); - return -EINVAL; + return ERR_PTR(-EINVAL); } - return rtnetlink_send(skb, net, portid, RTNLGRP_TC, - n->nlmsg_flags & NLM_F_ECHO); + return skb; +} + +static int tcf_add_notify(struct net *net, struct nlmsghdr *n, + struct tc_action *actions[], u32 portid, + size_t attr_size, struct netlink_ext_ack *extack) +{ + struct sk_buff *skb = NULL; + + if (!rtnl_notify_needed(net, n->nlmsg_flags, RTNLGRP_TC)) + goto skip_msg; + + skb = tcf_add_notify_msg(net, n, actions, portid, attr_size, extack); + if (IS_ERR(skb)) + return PTR_ERR(skb); + +skip_msg: + return rtnetlink_maybe_send(skb, net, portid, RTNLGRP_TC, + n->nlmsg_flags & NLM_F_ECHO); } static int tcf_action_add(struct net *net, struct nlattr *nla,
As of today tc-action events are unconditionally built and sent to RTNLGRP_TC. As with the introduction of tc_should_notify we can check before-hand if they are really needed. Signed-off-by: Pedro Tammela <pctammela@mojatatu.com> --- net/sched/act_api.c | 105 ++++++++++++++++++++++++++++++++------------ 1 file changed, 76 insertions(+), 29 deletions(-)