diff mbox series

[net-next,v3,4/5] net/sched: act_api: conditional notification of events

Message ID 20231206164416.543503-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

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1117 this patch: 1117
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang success Errors and warnings before: 1143 this patch: 1143
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1144 this patch: 1144
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 167 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Pedro Tammela Dec. 6, 2023, 4:44 p.m. UTC 
As of today tc-action events are unconditionally built and sent to
RTNLGRP_TC. As with the introduction of rtnl_notify_needed 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(-)

Comments

Simon Horman Dec. 7, 2023, 8:56 p.m. UTC | #1 
On Wed, Dec 06, 2023 at 01:44:15PM -0300, Pedro Tammela wrote:
> As of today tc-action events are unconditionally built and sent to
> RTNLGRP_TC. As with the introduction of rtnl_notify_needed we can check
> before-hand if they are really needed.
> 
> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>

Hi Pedro,

a nice optimisation :)

...

> +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;

Is there a reason (performance?) to use a goto here
rather than putting the tcf_reoffload_del_notify_msg() call inside
an if condition?

Completely untested!

	if (!rtnl_notify_needed(net, 0, RTNLGRP_TC)) {
		skb = NULL;
	} else {
		skb = tcf_reoffload_del_notify_msg(net, action);
		if (IS_ERR(skb))
			return PTR_ERR(skb);
	}

Or perhaps a helper, as this pattern seems to also appear in tcf_add_notify()


> +
> +	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);
>  	}

...

> +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,
> -- 
> 2.40.1
>
Jiri Pirko Dec. 8, 2023, 10:09 a.m. UTC | #2 
Wed, Dec 06, 2023 at 05:44:15PM CET, pctammela@mojatatu.com wrote:
>As of today tc-action events are unconditionally built and sent to
>RTNLGRP_TC. As with the introduction of rtnl_notify_needed 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 abec5c45b5a4..c15c2083ac84 100644
>--- a/net/sched/act_api.c
>+++ b/net/sched/act_api.c
>@@ -1785,31 +1785,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);

I don't follow. I think you promissed to have this unrelated change as a
separate patch, right? Could you? Unrelated to this patch.


> 	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);
> 	}
>@@ -1875,25 +1889,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) {
>@@ -1902,9 +1933,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
>@@ -1955,26 +1985,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,
>-- 
>2.40.1
>
Pedro Tammela Dec. 8, 2023, 2:21 p.m. UTC | #3 
On 07/12/2023 17:56, Simon Horman wrote:
> On Wed, Dec 06, 2023 at 01:44:15PM -0300, Pedro Tammela wrote:
>> As of today tc-action events are unconditionally built and sent to
>> RTNLGRP_TC. As with the introduction of rtnl_notify_needed we can check
>> before-hand if they are really needed.
>>
>> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
> 
> Hi Pedro,
> 
> a nice optimisation :)
> 
> ...
> 
>> +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;
> 
> Is there a reason (performance?) to use a goto here
> rather than putting the tcf_reoffload_del_notify_msg() call inside
> an if condition?

Not really, seems like the goto mosquito bit me on this one.
I will move to your suggestion and do the change Jiri asked which I 
totally forgot in v3.

> 
> Completely untested!
> 
> 	if (!rtnl_notify_needed(net, 0, RTNLGRP_TC)) {
> 		skb = NULL;
> 	} else {
> 		skb = tcf_reoffload_del_notify_msg(net, action);
> 		if (IS_ERR(skb))
> 			return PTR_ERR(skb);
> 	}
> 
> Or perhaps a helper, as this pattern seems to also appear in tcf_add_notify() >
> 
>> +
>> +	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);
>>   	}
> 
> ...
> 
>> +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,
>> -- 
>> 2.40.1
>>
diff mbox series

Patch

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index abec5c45b5a4..c15c2083ac84 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -1785,31 +1785,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);
 	}
@@ -1875,25 +1889,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) {
@@ -1902,9 +1933,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
@@ -1955,26 +1985,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,