* [PATCH net-next 1/4] rtnl: add helper to check if rtnl group has listeners
2023-12-01 20:43 [PATCH net-next 0/4] net/sched: conditional notification of events for cls and act Pedro Tammela
@ 2023-12-01 20:43 ` Pedro Tammela
2023-12-01 20:43 ` [PATCH net-next 2/4] net/sched: add helper to check if a notification is needed Pedro Tammela
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: Pedro Tammela @ 2023-12-01 20:43 UTC (permalink / raw)
To: netdev
Cc: davem, edumazet, kuba, pabeni, jhs, xiyou.wangcong, jiri,
marcelo.leitner, vladbu, Victor Nogueira, 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.
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH net-next 2/4] net/sched: add helper to check if a notification is needed
2023-12-01 20:43 [PATCH net-next 0/4] net/sched: conditional notification of events for cls and act Pedro Tammela
2023-12-01 20:43 ` [PATCH net-next 1/4] rtnl: add helper to check if rtnl group has listeners Pedro Tammela
@ 2023-12-01 20:43 ` Pedro Tammela
2023-12-02 19:18 ` Jakub Kicinski
2023-12-01 20:43 ` [PATCH net-next 3/4] net/sched: act_api: conditional notification of events Pedro Tammela
2023-12-01 20:43 ` [PATCH net-next 4/4] net/sched: cls_api: " Pedro Tammela
3 siblings, 1 reply; 9+ messages in thread
From: Pedro Tammela @ 2023-12-01 20:43 UTC (permalink / raw)
To: netdev
Cc: davem, edumazet, kuba, pabeni, jhs, xiyou.wangcong, jiri,
marcelo.leitner, vladbu, Victor Nogueira, Pedro Tammela
From: Victor Nogueira <victor@mojatatu•com>
Building on the rtnl_has_listeners helper, add the tc_should_notify
helper to check if we can bail out early in the tc notification
routines.
Signed-off-by: Victor Nogueira <victor@mojatatu•com>
Signed-off-by: Pedro Tammela <pctammela@mojatatu•com>
---
include/net/pkt_cls.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index a76c9171db0e..39c24cf30984 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -1065,4 +1065,9 @@ static inline void tc_skb_ext_tc_disable(void) { }
#define tc_skb_ext_tc_enabled() false
#endif
+static inline bool tc_should_notify(const struct net *net, u16 nlflags)
+{
+ return (nlflags & NLM_F_ECHO) || rtnl_has_listeners(net, RTNLGRP_TC);
+}
+
#endif
--
2.40.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH net-next 2/4] net/sched: add helper to check if a notification is needed
2023-12-01 20:43 ` [PATCH net-next 2/4] net/sched: add helper to check if a notification is needed Pedro Tammela
@ 2023-12-02 19:18 ` Jakub Kicinski
2023-12-04 14:50 ` Pedro Tammela
0 siblings, 1 reply; 9+ messages in thread
From: Jakub Kicinski @ 2023-12-02 19:18 UTC (permalink / raw)
To: Pedro Tammela
Cc: netdev, davem, edumazet, pabeni, jhs, xiyou.wangcong, jiri,
marcelo.leitner, vladbu, Victor Nogueira
On Fri, 1 Dec 2023 17:43:12 -0300 Pedro Tammela wrote:
> +static inline bool tc_should_notify(const struct net *net, u16 nlflags)
nit: tc_notify_needed() ? doesn't matter
> +{
> + return (nlflags & NLM_F_ECHO) || rtnl_has_listeners(net, RTNLGRP_TC);
I think it'd be nice to have an rtnl_* helper which looks at the flags
as well. With a proper kdoc. Maybe someone will notice that and remember
to implement F_ECHO for their command?
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH net-next 2/4] net/sched: add helper to check if a notification is needed
2023-12-02 19:18 ` Jakub Kicinski
@ 2023-12-04 14:50 ` Pedro Tammela
0 siblings, 0 replies; 9+ messages in thread
From: Pedro Tammela @ 2023-12-04 14:50 UTC (permalink / raw)
To: Jakub Kicinski
Cc: netdev, davem, edumazet, pabeni, jhs, xiyou.wangcong, jiri,
marcelo.leitner, vladbu, Victor Nogueira
On 02/12/2023 16:18, Jakub Kicinski wrote:
>> +{
>> + return (nlflags & NLM_F_ECHO) || rtnl_has_listeners(net, RTNLGRP_TC);
>
> I think it'd be nice to have an rtnl_* helper which looks at the flags
> as well. With a proper kdoc. Maybe someone will notice that and remember
> to implement F_ECHO for their command?
Ack, will change this to a generic rtnl helper
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH net-next 3/4] net/sched: act_api: conditional notification of events
2023-12-01 20:43 [PATCH net-next 0/4] net/sched: conditional notification of events for cls and act Pedro Tammela
2023-12-01 20:43 ` [PATCH net-next 1/4] rtnl: add helper to check if rtnl group has listeners Pedro Tammela
2023-12-01 20:43 ` [PATCH net-next 2/4] net/sched: add helper to check if a notification is needed Pedro Tammela
@ 2023-12-01 20:43 ` Pedro Tammela
2023-12-02 19:21 ` Jakub Kicinski
2023-12-01 20:43 ` [PATCH net-next 4/4] net/sched: cls_api: " Pedro Tammela
3 siblings, 1 reply; 9+ messages in thread
From: Pedro Tammela @ 2023-12-01 20:43 UTC (permalink / raw)
To: netdev
Cc: davem, edumazet, kuba, pabeni, jhs, xiyou.wangcong, jiri,
marcelo.leitner, vladbu, Pedro Tammela
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 | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index c39252d61ebb..2570f9702eeb 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -1791,6 +1791,13 @@ tcf_reoffload_del_notify(struct net *net, struct tc_action *action)
struct sk_buff *skb;
int ret;
+ if (!tc_should_notify(net, 0)) {
+ ret = tcf_idr_release_unsafe(action);
+ if (ret == ACT_P_DELETED)
+ module_put(ops->owner);
+ return ret;
+ }
+
skb = alloc_skb(attr_size <= NLMSG_GOODSIZE ? NLMSG_GOODSIZE : attr_size,
GFP_KERNEL);
if (!skb)
@@ -1877,6 +1884,13 @@ tcf_del_notify(struct net *net, struct nlmsghdr *n, struct tc_action *actions[],
int ret;
struct sk_buff *skb;
+ if (!tc_should_notify(net, n->nlmsg_flags)) {
+ ret = tcf_action_delete(net, actions);
+ if (ret < 0)
+ NL_SET_ERR_MSG(extack, "Failed to delete TC action");
+ return ret;
+ }
+
skb = alloc_skb(attr_size <= NLMSG_GOODSIZE ? NLMSG_GOODSIZE : attr_size,
GFP_KERNEL);
if (!skb)
@@ -1956,6 +1970,9 @@ tcf_add_notify(struct net *net, struct nlmsghdr *n, struct tc_action *actions[],
{
struct sk_buff *skb;
+ if (!tc_should_notify(net, n->nlmsg_flags))
+ return 0;
+
skb = alloc_skb(attr_size <= NLMSG_GOODSIZE ? NLMSG_GOODSIZE : attr_size,
GFP_KERNEL);
if (!skb)
--
2.40.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH net-next 3/4] net/sched: act_api: conditional notification of events
2023-12-01 20:43 ` [PATCH net-next 3/4] net/sched: act_api: conditional notification of events Pedro Tammela
@ 2023-12-02 19:21 ` Jakub Kicinski
2023-12-04 14:52 ` Pedro Tammela
0 siblings, 1 reply; 9+ messages in thread
From: Jakub Kicinski @ 2023-12-02 19:21 UTC (permalink / raw)
To: Pedro Tammela
Cc: netdev, davem, edumazet, pabeni, jhs, xiyou.wangcong, jiri,
marcelo.leitner, vladbu
On Fri, 1 Dec 2023 17:43:13 -0300 Pedro Tammela wrote:
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -1791,6 +1791,13 @@ tcf_reoffload_del_notify(struct net *net, struct tc_action *action)
> struct sk_buff *skb;
> int ret;
>
> + if (!tc_should_notify(net, 0)) {
> + ret = tcf_idr_release_unsafe(action);
> + if (ret == ACT_P_DELETED)
> + module_put(ops->owner);
> + return ret;
> + }
I fell like we can do better than this.. let's refactor this code a bit
harder. Maybe factor out the alloc_skb() and fill()? Then add a wrapper
around rtnetlink_send() which does nothing if skb is NULL?
> skb = alloc_skb(attr_size <= NLMSG_GOODSIZE ? NLMSG_GOODSIZE : attr_size,
> GFP_KERNEL);
> if (!skb)
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH net-next 3/4] net/sched: act_api: conditional notification of events
2023-12-02 19:21 ` Jakub Kicinski
@ 2023-12-04 14:52 ` Pedro Tammela
0 siblings, 0 replies; 9+ messages in thread
From: Pedro Tammela @ 2023-12-04 14:52 UTC (permalink / raw)
To: Jakub Kicinski
Cc: netdev, davem, edumazet, pabeni, jhs, xiyou.wangcong, jiri,
marcelo.leitner, vladbu
On 02/12/2023 16:21, Jakub Kicinski wrote:
> On Fri, 1 Dec 2023 17:43:13 -0300 Pedro Tammela wrote:
>> --- a/net/sched/act_api.c
>> +++ b/net/sched/act_api.c
>> @@ -1791,6 +1791,13 @@ tcf_reoffload_del_notify(struct net *net, struct tc_action *action)
>> struct sk_buff *skb;
>> int ret;
>>
>> + if (!tc_should_notify(net, 0)) {
>> + ret = tcf_idr_release_unsafe(action);
>> + if (ret == ACT_P_DELETED)
>> + module_put(ops->owner);
>> + return ret;
>> + }
>
> I fell like we can do better than this.. let's refactor this code a bit
> harder. Maybe factor out the alloc_skb() and fill()? Then add a wrapper
> around rtnetlink_send() which does nothing if skb is NULL?
Ack
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH net-next 4/4] net/sched: cls_api: conditional notification of events
2023-12-01 20:43 [PATCH net-next 0/4] net/sched: conditional notification of events for cls and act Pedro Tammela
` (2 preceding siblings ...)
2023-12-01 20:43 ` [PATCH net-next 3/4] net/sched: act_api: conditional notification of events Pedro Tammela
@ 2023-12-01 20:43 ` Pedro Tammela
3 siblings, 0 replies; 9+ messages in thread
From: Pedro Tammela @ 2023-12-01 20:43 UTC (permalink / raw)
To: netdev
Cc: davem, edumazet, kuba, pabeni, jhs, xiyou.wangcong, jiri,
marcelo.leitner, vladbu, Pedro Tammela
As of today tc-filter/chain 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. This will help to alleviate
system pressure when filters are concurrently added without the rtnl
lock as in tc-flower.
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..e99df0543c91 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 && !tc_should_notify(net, n->nlmsg_flags))
+ 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 && !tc_should_notify(net, n->nlmsg_flags))
+ 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 && !tc_should_notify(net, flags))
+ 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 && !tc_should_notify(net, flags))
+ return 0;
+
skb = alloc_skb(NLMSG_GOODSIZE, GFP_KERNEL);
if (!skb)
return -ENOBUFS;
--
2.40.1
^ permalink raw reply related [flat|nested] 9+ messages in thread