[PATCH net-next v3 0/5] net/sched: conditional notification of events for cls and act

[PATCH net-next v3 0/5] net/sched: conditional notification of events for cls and act

[Pedro Tammela]

This is an optimization we have been leveraging on P4TC but we believe
it will benefit rtnl users in general.

It's common to allocate an skb, build a notification message and then
broadcast an event. In the absence of any user space listeners, these
resources (cpu and memory operations) are wasted. In cases where the subsystem
is lockless (such as in tc-flower) this waste is more prominent. For the
scenarios where the rtnl_lock is held it is not as prominent.

The idea is simple. Build and send the notification iif:
   - The user requests via NLM_F_ECHO or
   - Someone is listening to the rtnl group (tc mon)

On a simple test with tc-flower adding 1M entries, using just a single core,
there's already a noticeable difference in the cycles spent in tc_new_tfilter
with this patchset.

before:
   - 43.68% tc_new_tfilter
      + 31.73% fl_change
      + 6.35% tfilter_notify
      + 1.62% nlmsg_notify
        0.66% __tcf_qdisc_find.part.0
        0.64% __tcf_chain_get
        0.54% fl_get
      + 0.53% tcf_proto_lookup_ops

after:
   - 39.20% tc_new_tfilter
      + 34.58% fl_change
        0.69% __tcf_qdisc_find.part.0
        0.67% __tcf_chain_get
      + 0.61% tcf_proto_lookup_ops

Note, the above test is using iproute2:tc which execs a shell.
We expect people using netlink directly to observe even greater
reductions.

The qdisc side needs some refactoring of the notification routines to fit in
this new model, so they will be sent in a later patchset.

v2->v3:
- Collected Jiri review tags
- Fixed commit messages to refer to rtnl_notify_needed

v1->v2:
- Address Jakub comments

Jamal Hadi Salim (1):
  rtnl: add helper to check if rtnl group has listeners

Pedro Tammela (3):
  rtnl: add helper to send if skb is not null
  net/sched: act_api: conditional notification of events
  net/sched: cls_api: conditional notification of events

Victor Nogueira (1):
  rtnl: add helper to check if a notification is needed

 include/linux/rtnetlink.h |  29 +++++++++++
 net/sched/act_api.c       | 105 +++++++++++++++++++++++++++-----------
 net/sched/cls_api.c       |  12 +++++
 3 files changed, 117 insertions(+), 29 deletions(-)

-- 
2.40.1

[PATCH net-next v3 1/5] rtnl: add helper to check if rtnl group has listeners

[Pedro Tammela]

From: Jamal Hadi Salim <jhs@mojatatu.com>

As of today, rtnl code creates a new skb and unconditionally fills and
broadcasts it to the relevant group. For most operations this is okay
and doesn't waste resources in general.

When operations are done without the rtnl_lock, as in tc-flower, such
skb allocation, message fill and no-op broadcasting can happen in all
cores of the system, which contributes to system pressure and wastes
precious cpu cycles when no one will receive the built message.

Introduce this helper so rtnetlink operations can simply check if someone
is listening and then proceed if necessary.

Reviewed-by: Jiri Pirko <jiri@nvidia.com>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Victor Nogueira <victor@mojatatu.com>
Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
---
 include/linux/rtnetlink.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/linux/rtnetlink.h b/include/linux/rtnetlink.h
index 3d6cf306cd55..a7d757e96c55 100644
--- a/include/linux/rtnetlink.h
+++ b/include/linux/rtnetlink.h
@@ -130,4 +130,11 @@ extern int ndo_dflt_bridge_getlink(struct sk_buff *skb, u32 pid, u32 seq,
 
 extern void rtnl_offload_xstats_notify(struct net_device *dev);
 
+static inline int rtnl_has_listeners(const struct net *net, u32 group)
+{
+	struct sock *rtnl = net->rtnl;
+
+	return netlink_has_listeners(rtnl, group);
+}
+
 #endif	/* __LINUX_RTNETLINK_H */
-- 
2.40.1

[PATCH net-next v3 2/5] rtnl: add helper to check if a notification is needed

[Pedro Tammela]

From: Victor Nogueira <victor@mojatatu.com>

Building on the rtnl_has_listeners helper, add the rtnl_notify_needed
helper to check if we can bail out early in the notification routines.

Reviewed-by: Jiri Pirko <jiri@nvidia.com>
Signed-off-by: Victor Nogueira <victor@mojatatu.com>
Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
---
 include/linux/rtnetlink.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/include/linux/rtnetlink.h b/include/linux/rtnetlink.h
index a7d757e96c55..0cbbbded0331 100644
--- a/include/linux/rtnetlink.h
+++ b/include/linux/rtnetlink.h
@@ -137,4 +137,19 @@ static inline int rtnl_has_listeners(const struct net *net, u32 group)
 	return netlink_has_listeners(rtnl, group);
 }
 
+/**
+ * rtnl_notify_needed - check if notification is needed
+ * @net: Pointer to the net namespace
+ * @nlflags: netlink ingress message flags
+ * @group: rtnl group
+ *
+ * Based on the ingress message flags and rtnl group, returns true
+ * if a notification is needed, false otherwise.
+ */
+static inline bool
+rtnl_notify_needed(const struct net *net, u16 nlflags, u32 group)
+{
+	return (nlflags & NLM_F_ECHO) || rtnl_has_listeners(net, group);
+}
+
 #endif	/* __LINUX_RTNETLINK_H */
-- 
2.40.1

[PATCH net-next v3 3/5] rtnl: add helper to send if skb is not null

[Pedro Tammela]

This is a convenience helper for routines handling conditional rtnl
events, that is code that might send a notification depending on
rtnl_has_listeners/rtnl_notify_needed.

Instead of:
   if (skb)
      rtnetlink_send(...)

Use:
      rtnetlink_maybe_send(...)

Reviewed-by: Jiri Pirko <jiri@nvidia.com>
Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
---
 include/linux/rtnetlink.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/linux/rtnetlink.h b/include/linux/rtnetlink.h
index 0cbbbded0331..6a8543b34e2c 100644
--- a/include/linux/rtnetlink.h
+++ b/include/linux/rtnetlink.h
@@ -10,6 +10,13 @@
 #include <uapi/linux/rtnetlink.h>
 
 extern int rtnetlink_send(struct sk_buff *skb, struct net *net, u32 pid, u32 group, int echo);
+
+static inline int rtnetlink_maybe_send(struct sk_buff *skb, struct net *net,
+				       u32 pid, u32 group, int echo)
+{
+	return !skb ? 0 : rtnetlink_send(skb, net, pid, group, echo);
+}
+
 extern int rtnl_unicast(struct sk_buff *skb, struct net *net, u32 pid);
 extern void rtnl_notify(struct sk_buff *skb, struct net *net, u32 pid,
 			u32 group, const struct nlmsghdr *nlh, gfp_t flags);
-- 
2.40.1

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

[Pedro Tammela]

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

[PATCH net-next v3 5/5] net/sched: cls_api: conditional notification of events

[Pedro Tammela]

As of today tc-filter/chain 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. This will help to alleviate
system pressure when filters are concurrently added without the rtnl
lock as in tc-flower.

Reviewed-by: Jiri Pirko <jiri@nvidia.com>
Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
---
 net/sched/cls_api.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 1976bd163986..123185907ebd 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -2053,6 +2053,9 @@ static int tfilter_notify(struct net *net, struct sk_buff *oskb,
 	u32 portid = oskb ? NETLINK_CB(oskb).portid : 0;
 	int err = 0;
 
+	if (!unicast && !rtnl_notify_needed(net, n->nlmsg_flags, RTNLGRP_TC))
+		return 0;
+
 	skb = alloc_skb(NLMSG_GOODSIZE, GFP_KERNEL);
 	if (!skb)
 		return -ENOBUFS;
@@ -2082,6 +2085,9 @@ static int tfilter_del_notify(struct net *net, struct sk_buff *oskb,
 	u32 portid = oskb ? NETLINK_CB(oskb).portid : 0;
 	int err;
 
+	if (!unicast && !rtnl_notify_needed(net, n->nlmsg_flags, RTNLGRP_TC))
+		return tp->ops->delete(tp, fh, last, rtnl_held, extack);
+
 	skb = alloc_skb(NLMSG_GOODSIZE, GFP_KERNEL);
 	if (!skb)
 		return -ENOBUFS;
@@ -2906,6 +2912,9 @@ static int tc_chain_notify(struct tcf_chain *chain, struct sk_buff *oskb,
 	struct sk_buff *skb;
 	int err = 0;
 
+	if (!unicast && !rtnl_notify_needed(net, flags, RTNLGRP_TC))
+		return 0;
+
 	skb = alloc_skb(NLMSG_GOODSIZE, GFP_KERNEL);
 	if (!skb)
 		return -ENOBUFS;
@@ -2935,6 +2944,9 @@ static int tc_chain_notify_delete(const struct tcf_proto_ops *tmplt_ops,
 	struct net *net = block->net;
 	struct sk_buff *skb;
 
+	if (!unicast && !rtnl_notify_needed(net, flags, RTNLGRP_TC))
+		return 0;
+
 	skb = alloc_skb(NLMSG_GOODSIZE, GFP_KERNEL);
 	if (!skb)
 		return -ENOBUFS;
-- 
2.40.1

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

[Simon Horman]

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
> 

Re: [PATCH net-next v3 5/5] net/sched: cls_api: conditional notification of events

[Simon Horman]

On Wed, Dec 06, 2023 at 01:44:16PM -0300, Pedro Tammela wrote:
> As of today tc-filter/chain 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. This will help to alleviate
> system pressure when filters are concurrently added without the rtnl
> lock as in tc-flower.
> 
> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>

Thanks Pedro,

my comment below notwithstanding, this looks good to me.

Reviewed-by: Simon Horman <horms@kernel.org>

> ---
>  net/sched/cls_api.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index 1976bd163986..123185907ebd 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -2053,6 +2053,9 @@ static int tfilter_notify(struct net *net, struct sk_buff *oskb,
>  	u32 portid = oskb ? NETLINK_CB(oskb).portid : 0;
>  	int err = 0;
>  
> +	if (!unicast && !rtnl_notify_needed(net, n->nlmsg_flags, RTNLGRP_TC))

I don't feel strongly about this, but
as the above condition appears 3 times in this patch,
perhaps it could be a helper?

> +		return 0;
> +
>  	skb = alloc_skb(NLMSG_GOODSIZE, GFP_KERNEL);
>  	if (!skb)
>  		return -ENOBUFS;
> @@ -2082,6 +2085,9 @@ static int tfilter_del_notify(struct net *net, struct sk_buff *oskb,
>  	u32 portid = oskb ? NETLINK_CB(oskb).portid : 0;
>  	int err;
>  
> +	if (!unicast && !rtnl_notify_needed(net, n->nlmsg_flags, RTNLGRP_TC))
> +		return tp->ops->delete(tp, fh, last, rtnl_held, extack);
> +
>  	skb = alloc_skb(NLMSG_GOODSIZE, GFP_KERNEL);
>  	if (!skb)
>  		return -ENOBUFS;
> @@ -2906,6 +2912,9 @@ static int tc_chain_notify(struct tcf_chain *chain, struct sk_buff *oskb,
>  	struct sk_buff *skb;
>  	int err = 0;
>  
> +	if (!unicast && !rtnl_notify_needed(net, flags, RTNLGRP_TC))
> +		return 0;
> +
>  	skb = alloc_skb(NLMSG_GOODSIZE, GFP_KERNEL);
>  	if (!skb)
>  		return -ENOBUFS;
> @@ -2935,6 +2944,9 @@ static int tc_chain_notify_delete(const struct tcf_proto_ops *tmplt_ops,
>  	struct net *net = block->net;
>  	struct sk_buff *skb;
>  
> +	if (!unicast && !rtnl_notify_needed(net, flags, RTNLGRP_TC))
> +		return 0;
> +
>  	skb = alloc_skb(NLMSG_GOODSIZE, GFP_KERNEL);
>  	if (!skb)
>  		return -ENOBUFS;
> -- 
> 2.40.1
> 

Re: [PATCH net-next v3 1/5] rtnl: add helper to check if rtnl group has listeners

[Simon Horman]

On Wed, Dec 06, 2023 at 01:44:12PM -0300, Pedro Tammela wrote:
> From: Jamal Hadi Salim <jhs@mojatatu.com>
> 
> As of today, rtnl code creates a new skb and unconditionally fills and
> broadcasts it to the relevant group. For most operations this is okay
> and doesn't waste resources in general.
> 
> When operations are done without the rtnl_lock, as in tc-flower, such
> skb allocation, message fill and no-op broadcasting can happen in all
> cores of the system, which contributes to system pressure and wastes
> precious cpu cycles when no one will receive the built message.
> 
> Introduce this helper so rtnetlink operations can simply check if someone
> is listening and then proceed if necessary.
> 
> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
> Signed-off-by: Victor Nogueira <victor@mojatatu.com>
> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next v3 2/5] rtnl: add helper to check if a notification is needed

[Simon Horman]

On Wed, Dec 06, 2023 at 01:44:13PM -0300, Pedro Tammela wrote:
> From: Victor Nogueira <victor@mojatatu.com>
> 
> Building on the rtnl_has_listeners helper, add the rtnl_notify_needed
> helper to check if we can bail out early in the notification routines.
> 
> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
> Signed-off-by: Victor Nogueira <victor@mojatatu.com>
> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next v3 3/5] rtnl: add helper to send if skb is not null

[Simon Horman]

On Wed, Dec 06, 2023 at 01:44:14PM -0300, Pedro Tammela wrote:
> This is a convenience helper for routines handling conditional rtnl
> events, that is code that might send a notification depending on
> rtnl_has_listeners/rtnl_notify_needed.
> 
> Instead of:
>    if (skb)
>       rtnetlink_send(...)
> 
> Use:
>       rtnetlink_maybe_send(...)
> 
> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>

Reviewed-by: Simon Horman <horms@kernel.org>

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

[Jiri Pirko]

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
>

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

[Pedro Tammela]

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

Re: [PATCH net-next v3 5/5] net/sched: cls_api: conditional notification of events

[Pedro Tammela]

On 07/12/2023 17:59, Simon Horman wrote:
> On Wed, Dec 06, 2023 at 01:44:16PM -0300, Pedro Tammela wrote:
>> As of today tc-filter/chain 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. This will help to alleviate
>> system pressure when filters are concurrently added without the rtnl
>> lock as in tc-flower.
>>
>> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
>> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
> 
> Thanks Pedro,
> 
> my comment below notwithstanding, this looks good to me.
> 
> Reviewed-by: Simon Horman <horms@kernel.org>
> 
>> ---
>>   net/sched/cls_api.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>> index 1976bd163986..123185907ebd 100644
>> --- a/net/sched/cls_api.c
>> +++ b/net/sched/cls_api.c
>> @@ -2053,6 +2053,9 @@ static int tfilter_notify(struct net *net, struct sk_buff *oskb,
>>   	u32 portid = oskb ? NETLINK_CB(oskb).portid : 0;
>>   	int err = 0;
>>   
>> +	if (!unicast && !rtnl_notify_needed(net, n->nlmsg_flags, RTNLGRP_TC))
> 
> I don't feel strongly about this, but
> as the above condition appears 3 times in this patch,
> perhaps it could be a helper?

I noticed that some of these functions will never call with unicast set 
to true (delete routines). So I will append a patch that removes it.
So this pattern will hopefully bet not repeating enough for a macro :)

> 
>> +		return 0;
>> +
>>   	skb = alloc_skb(NLMSG_GOODSIZE, GFP_KERNEL);
>>   	if (!skb)
>>   		return -ENOBUFS;
>> @@ -2082,6 +2085,9 @@ static int tfilter_del_notify(struct net *net, struct sk_buff *oskb,
>>   	u32 portid = oskb ? NETLINK_CB(oskb).portid : 0;
>>   	int err;
>>   
>> +	if (!unicast && !rtnl_notify_needed(net, n->nlmsg_flags, RTNLGRP_TC))
>> +		return tp->ops->delete(tp, fh, last, rtnl_held, extack);
>> +
>>   	skb = alloc_skb(NLMSG_GOODSIZE, GFP_KERNEL);
>>   	if (!skb)
>>   		return -ENOBUFS;
>> @@ -2906,6 +2912,9 @@ static int tc_chain_notify(struct tcf_chain *chain, struct sk_buff *oskb,
>>   	struct sk_buff *skb;
>>   	int err = 0;
>>   
>> +	if (!unicast && !rtnl_notify_needed(net, flags, RTNLGRP_TC))
>> +		return 0;
>> +
>>   	skb = alloc_skb(NLMSG_GOODSIZE, GFP_KERNEL);
>>   	if (!skb)
>>   		return -ENOBUFS;
>> @@ -2935,6 +2944,9 @@ static int tc_chain_notify_delete(const struct tcf_proto_ops *tmplt_ops,
>>   	struct net *net = block->net;
>>   	struct sk_buff *skb;
>>   
>> +	if (!unicast && !rtnl_notify_needed(net, flags, RTNLGRP_TC))
>> +		return 0;
>> +
>>   	skb = alloc_skb(NLMSG_GOODSIZE, GFP_KERNEL);
>>   	if (!skb)
>>   		return -ENOBUFS;
>> -- 
>> 2.40.1
>>