[PATCH (repost) net-next] sched: add extack for tfilter_notify

[PATCH (repost) net-next] sched: add extack for tfilter_notify

[Hangbin Liu]

In commit 81c7288b170a ("sched: cls: enable verbose logging") Marcelo
made cls could log verbose info for offloading failures, which helps
improving Open vSwitch debuggability when using flower offloading.

It would also be helpful if "tc monitor" could log this message, as it
doesn't require vswitchd log level adjusment. Let's add the extack message
in tfilter_notify so the monitor program could receive the failures.
e.g.

  # tc monitor
  added chain dev enp3s0f1np1 parent ffff: chain 0
  added filter dev enp3s0f1np1 ingress protocol all pref 49152 flower chain 0 handle 0x1
    ct_state +trk+new
    not_in_hw
          action order 1: gact action drop
           random type none pass val 0
           index 1 ref 1 bind 1

  Warning: mlx5_core: matching on ct_state +new isn't supported.

Suggested-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---

Rebase the patch to latest net-next as the previous could not
apply to net-next.

---
 net/sched/cls_api.c | 45 ++++++++++++++++++++++++++++++---------------
 1 file changed, 30 insertions(+), 15 deletions(-)

diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 50566db45949..7994dfdbd312 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -1816,13 +1816,15 @@ static int tcf_fill_node(struct net *net, struct sk_buff *skb,
 			 struct tcf_proto *tp, struct tcf_block *block,
 			 struct Qdisc *q, u32 parent, void *fh,
 			 u32 portid, u32 seq, u16 flags, int event,
-			 bool terse_dump, bool rtnl_held)
+			 bool terse_dump, bool rtnl_held,
+			 struct netlink_ext_ack *extack)
 {
 	struct tcmsg *tcm;
 	struct nlmsghdr  *nlh;
 	unsigned char *b = skb_tail_pointer(skb);
 
-	nlh = nlmsg_put(skb, portid, seq, event, sizeof(*tcm), flags);
+	nlh = nlmsg_put(skb, portid, seq, event, sizeof(*tcm),
+			(extack && extack->_msg) ? flags | NLM_F_MULTI : flags);
 	if (!nlh)
 		goto out_nlmsg_trim;
 	tcm = nlmsg_data(nlh);
@@ -1857,6 +1859,18 @@ static int tcf_fill_node(struct net *net, struct sk_buff *skb,
 			goto nla_put_failure;
 	}
 	nlh->nlmsg_len = skb_tail_pointer(skb) - b;
+
+	if (extack && extack->_msg) {
+		nlh = nlmsg_put(skb, portid, seq, NLMSG_DONE, 0, flags | NLM_F_ACK_TLVS);
+		if (!nlh)
+			goto out_nlmsg_trim;
+
+		if (nla_put_string(skb, NLMSGERR_ATTR_MSG, extack->_msg))
+			goto nla_put_failure;
+
+		nlmsg_end(skb, nlh);
+	}
+
 	return skb->len;
 
 out_nlmsg_trim:
@@ -1870,7 +1884,7 @@ static int tfilter_notify(struct net *net, struct sk_buff *oskb,
 			  struct nlmsghdr *n, struct tcf_proto *tp,
 			  struct tcf_block *block, struct Qdisc *q,
 			  u32 parent, void *fh, int event, bool unicast,
-			  bool rtnl_held)
+			  bool rtnl_held, struct netlink_ext_ack *extack)
 {
 	struct sk_buff *skb;
 	u32 portid = oskb ? NETLINK_CB(oskb).portid : 0;
@@ -1882,7 +1896,7 @@ static int tfilter_notify(struct net *net, struct sk_buff *oskb,
 
 	if (tcf_fill_node(net, skb, tp, block, q, parent, fh, portid,
 			  n->nlmsg_seq, n->nlmsg_flags, event,
-			  false, rtnl_held) <= 0) {
+			  false, rtnl_held, extack) <= 0) {
 		kfree_skb(skb);
 		return -EINVAL;
 	}
@@ -1911,7 +1925,7 @@ static int tfilter_del_notify(struct net *net, struct sk_buff *oskb,
 
 	if (tcf_fill_node(net, skb, tp, block, q, parent, fh, portid,
 			  n->nlmsg_seq, n->nlmsg_flags, RTM_DELTFILTER,
-			  false, rtnl_held) <= 0) {
+			  false, rtnl_held, extack) <= 0) {
 		NL_SET_ERR_MSG(extack, "Failed to build del event notification");
 		kfree_skb(skb);
 		return -EINVAL;
@@ -1937,14 +1951,15 @@ static int tfilter_del_notify(struct net *net, struct sk_buff *oskb,
 static void tfilter_notify_chain(struct net *net, struct sk_buff *oskb,
 				 struct tcf_block *block, struct Qdisc *q,
 				 u32 parent, struct nlmsghdr *n,
-				 struct tcf_chain *chain, int event)
+				 struct tcf_chain *chain, int event,
+				 struct netlink_ext_ack *extack)
 {
 	struct tcf_proto *tp;
 
 	for (tp = tcf_get_next_proto(chain, NULL);
 	     tp; tp = tcf_get_next_proto(chain, tp))
-		tfilter_notify(net, oskb, n, tp, block,
-			       q, parent, NULL, event, false, true);
+		tfilter_notify(net, oskb, n, tp, block, q, parent, NULL,
+			       event, false, true, extack);
 }
 
 static void tfilter_put(struct tcf_proto *tp, void *fh)
@@ -2148,7 +2163,7 @@ static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
 			      flags, extack);
 	if (err == 0) {
 		tfilter_notify(net, skb, n, tp, block, q, parent, fh,
-			       RTM_NEWTFILTER, false, rtnl_held);
+			       RTM_NEWTFILTER, false, rtnl_held, extack);
 		tfilter_put(tp, fh);
 		/* q pointer is NULL for shared blocks */
 		if (q)
@@ -2276,7 +2291,7 @@ static int tc_del_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
 
 	if (prio == 0) {
 		tfilter_notify_chain(net, skb, block, q, parent, n,
-				     chain, RTM_DELTFILTER);
+				     chain, RTM_DELTFILTER, extack);
 		tcf_chain_flush(chain, rtnl_held);
 		err = 0;
 		goto errout;
@@ -2300,7 +2315,7 @@ static int tc_del_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
 
 		tcf_proto_put(tp, rtnl_held, NULL);
 		tfilter_notify(net, skb, n, tp, block, q, parent, fh,
-			       RTM_DELTFILTER, false, rtnl_held);
+			       RTM_DELTFILTER, false, rtnl_held, extack);
 		err = 0;
 		goto errout;
 	}
@@ -2444,7 +2459,7 @@ static int tc_get_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
 		err = -ENOENT;
 	} else {
 		err = tfilter_notify(net, skb, n, tp, block, q, parent,
-				     fh, RTM_NEWTFILTER, true, rtnl_held);
+				     fh, RTM_NEWTFILTER, true, rtnl_held, extack);
 		if (err < 0)
 			NL_SET_ERR_MSG(extack, "Failed to send filter notify message");
 	}
@@ -2482,7 +2497,7 @@ static int tcf_node_dump(struct tcf_proto *tp, void *n, struct tcf_walker *arg)
 	return tcf_fill_node(net, a->skb, tp, a->block, a->q, a->parent,
 			     n, NETLINK_CB(a->cb->skb).portid,
 			     a->cb->nlh->nlmsg_seq, NLM_F_MULTI,
-			     RTM_NEWTFILTER, a->terse_dump, true);
+			     RTM_NEWTFILTER, a->terse_dump, true, NULL);
 }
 
 static bool tcf_chain_dump(struct tcf_chain *chain, struct Qdisc *q, u32 parent,
@@ -2516,7 +2531,7 @@ static bool tcf_chain_dump(struct tcf_chain *chain, struct Qdisc *q, u32 parent,
 			if (tcf_fill_node(net, skb, tp, block, q, parent, NULL,
 					  NETLINK_CB(cb->skb).portid,
 					  cb->nlh->nlmsg_seq, NLM_F_MULTI,
-					  RTM_NEWTFILTER, false, true) <= 0)
+					  RTM_NEWTFILTER, false, true, NULL) <= 0)
 				goto errout;
 			cb->args[1] = 1;
 		}
@@ -2904,7 +2919,7 @@ static int tc_ctl_chain(struct sk_buff *skb, struct nlmsghdr *n,
 		break;
 	case RTM_DELCHAIN:
 		tfilter_notify_chain(net, skb, block, q, parent, n,
-				     chain, RTM_DELTFILTER);
+				     chain, RTM_DELTFILTER, extack);
 		/* Flush the chain first as the user requested chain removal. */
 		tcf_chain_flush(chain, true);
 		/* In case the chain was successfully deleted, put a reference
-- 
2.37.2

Re: [PATCH (repost) net-next] sched: add extack for tfilter_notify

[Jakub Kicinski]

On Thu, 29 Sep 2022 11:35:05 +0800 Hangbin Liu wrote:
> In commit 81c7288b170a ("sched: cls: enable verbose logging") Marcelo
> made cls could log verbose info for offloading failures, which helps
> improving Open vSwitch debuggability when using flower offloading.
> 
> It would also be helpful if "tc monitor" could log this message, as it
> doesn't require vswitchd log level adjusment. Let's add the extack message
> in tfilter_notify so the monitor program could receive the failures.
> e.g.

The title read as "just another extack addition" but this is much 
more than that :S 

Jamal, you may want to take a look.

>   # tc monitor
>   added chain dev enp3s0f1np1 parent ffff: chain 0
>   added filter dev enp3s0f1np1 ingress protocol all pref 49152 flower chain 0 handle 0x1
>     ct_state +trk+new
>     not_in_hw
>           action order 1: gact action drop
>            random type none pass val 0
>            index 1 ref 1 bind 1
> 
>   Warning: mlx5_core: matching on ct_state +new isn't supported.
> 
> Suggested-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
> 
> Rebase the patch to latest net-next as the previous could not
> apply to net-next.

> +	nlh = nlmsg_put(skb, portid, seq, event, sizeof(*tcm),
> +			(extack && extack->_msg) ? flags | NLM_F_MULTI : flags);

> +
> +	if (extack && extack->_msg) {
> +		nlh = nlmsg_put(skb, portid, seq, NLMSG_DONE, 0, flags | NLM_F_ACK_TLVS);
> +		if (!nlh)
> +			goto out_nlmsg_trim;
> +
> +		if (nla_put_string(skb, NLMSGERR_ATTR_MSG, extack->_msg))
> +			goto nla_put_failure;
> +
> +		nlmsg_end(skb, nlh);
> +	}
> +

So you're adding a fake* _F_MULTI on the notification just so you
can queue a NLMSG_DONE after and not break the "NLMSG_DONE terminates 
a _F_MUTLI" sequence rule?

* fake as in there's only one message, there's no multi-ness here.

I don't think _F_MULTI should be treated lightly and I don't think
NLMSG_DONE as part of notification sequences is a good idea either :(

(1) does the tracepoint not give you want you need?
    (netlink:netlink_extack), failing that -
(2) why not wrap the extack msg in an attribute

Re: [PATCH (repost) net-next] sched: add extack for tfilter_notify

[Cong Wang]

On Thu, Sep 29, 2022 at 11:35:05AM +0800, Hangbin Liu wrote:
> In commit 81c7288b170a ("sched: cls: enable verbose logging") Marcelo
> made cls could log verbose info for offloading failures, which helps
> improving Open vSwitch debuggability when using flower offloading.
> 
> It would also be helpful if "tc monitor" could log this message, as it
> doesn't require vswitchd log level adjusment. Let's add the extack message
> in tfilter_notify so the monitor program could receive the failures.
> e.g.
> 

I don't think tc monitor is supposed to carry any error messages, it
only serves the purpose for monitoring control path events.

Thanks.

Re: [PATCH (repost) net-next] sched: add extack for tfilter_notify

[Marcelo Ricardo Leitner]

On Sat, Oct 01, 2022 at 11:39:07AM -0700, Cong Wang wrote:
> On Thu, Sep 29, 2022 at 11:35:05AM +0800, Hangbin Liu wrote:
> > In commit 81c7288b170a ("sched: cls: enable verbose logging") Marcelo
> > made cls could log verbose info for offloading failures, which helps
> > improving Open vSwitch debuggability when using flower offloading.
> > 
> > It would also be helpful if "tc monitor" could log this message, as it
> > doesn't require vswitchd log level adjusment. Let's add the extack message
> > in tfilter_notify so the monitor program could receive the failures.
> > e.g.
> > 
> 
> I don't think tc monitor is supposed to carry any error messages, it
> only serves the purpose for monitoring control path events.

But, precisely. In the example Hangbin gave, it is showing why the
entry is not_in_hw. That's still data that belongs to the event that
happened and that can't be queried afterwards even if the user/app
monitoring it want to. Had it failed entirely, I agree, as the control
path never changed.

tc monitor is easier to use than perf probes in some systems. It's not
uncommon to have tc installed but not perf. It's also easier to ask a
customer to run it than explain how to enable the tracepoint and print
ftrace buffer via /sys files, and the output is more meaningful for us
as well: we know exactly which filter triggered the message. The only
other place that we can correlate the filter and the warning, is on
vswitchd log. Which is not easy to read either.

Thanks,
Marcelo

Re: [PATCH (repost) net-next] sched: add extack for tfilter_notify

[Jamal Hadi Salim]

On Sat, Oct 1, 2022 at 4:39 PM Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
>
> On Sat, Oct 01, 2022 at 11:39:07AM -0700, Cong Wang wrote:
> > On Thu, Sep 29, 2022 at 11:35:05AM +0800, Hangbin Liu wrote:
> > > In commit 81c7288b170a ("sched: cls: enable verbose logging") Marcelo
> > > made cls could log verbose info for offloading failures, which helps
> > > improving Open vSwitch debuggability when using flower offloading.
> > >
> > > It would also be helpful if "tc monitor" could log this message, as it
> > > doesn't require vswitchd log level adjusment. Let's add the extack message
> > > in tfilter_notify so the monitor program could receive the failures.
> > > e.g.
> > >
> >
> > I don't think tc monitor is supposed to carry any error messages, it
> > only serves the purpose for monitoring control path events.
>
> But, precisely. In the example Hangbin gave, it is showing why the
> entry is not_in_hw. That's still data that belongs to the event that
> happened and that can't be queried afterwards even if the user/app
> monitoring it want to. Had it failed entirely, I agree, as the control
> path never changed.
>
> tc monitor is easier to use than perf probes in some systems. It's not
> uncommon to have tc installed but not perf. It's also easier to ask a
> customer to run it than explain how to enable the tracepoint and print
> ftrace buffer via /sys files, and the output is more meaningful for us
> as well: we know exactly which filter triggered the message. The only
> other place that we can correlate the filter and the warning, is on
> vswitchd log. Which is not easy to read either.

To Jakub's point: I think one of those NLMSGERR TLVs is the right place
and while traces look attractive I see the value of having a unified
collection point via the tc monitor.
Since you cant really batch events - it seems the NLMSG_DONE/MULTI
hack is done just to please iproute2::tc?
IMO:
I think if you need to do this, then you have to teach iproute2
new ways of interpreting the message (which is nice because you
dont have to worry about backward compat). Some of that code
should be centralized and reused by netlink generically
instead of just cls_api, example the whole NLM_F_ACK_TLVS dance.

Also - i guess it will depend on the underlying driver?
This seems very related to a specific driver:
"Warning: mlx5_core: matching on ct_state +new isn't supported."
Debuggability is always great but so is backwards compat.
What happens when you run old userspace tc? There are tons
of punting systems that process these events out there and
depend on the current event messages as is.

cheers,
jamal

Re: [PATCH (repost) net-next] sched: add extack for tfilter_notify

[Hangbin Liu]

On Sun, Oct 02, 2022 at 11:27:08AM -0400, Jamal Hadi Salim wrote:
> > But, precisely. In the example Hangbin gave, it is showing why the
> > entry is not_in_hw. That's still data that belongs to the event that
> > happened and that can't be queried afterwards even if the user/app
> > monitoring it want to. Had it failed entirely, I agree, as the control
> > path never changed.
> >
> > tc monitor is easier to use than perf probes in some systems. It's not
> > uncommon to have tc installed but not perf. It's also easier to ask a
> > customer to run it than explain how to enable the tracepoint and print
> > ftrace buffer via /sys files, and the output is more meaningful for us
> > as well: we know exactly which filter triggered the message. The only
> > other place that we can correlate the filter and the warning, is on
> > vswitchd log. Which is not easy to read either.
> 
> To Jakub's point: I think one of those NLMSGERR TLVs is the right place
> and while traces look attractive I see the value of having a unified
> collection point via the tc monitor.

Hi Jamal,

Sorry for the late response. I just came back form vacation. For this issue,
I saw netlink_dump_done() also put NLMSGERR_ATTR_MSG in NLMSG_DONE.
So why can't we do the same here?

In https://www.kernel.org/doc/html//next/userspace-api/netlink/intro.html,
The "optionally extended ACK" in NLMSG_DONE is OK.

> Since you cant really batch events - it seems the NLMSG_DONE/MULTI
> hack is done just to please iproute2::tc?

Yes.

> IMO:
> I think if you need to do this, then you have to teach iproute2
> new ways of interpreting the message (which is nice because you
> dont have to worry about backward compat). Some of that code
> should be centralized and reused by netlink generically
> instead of just cls_api, example the whole NLM_F_ACK_TLVS dance.

Would you please help explain more about this?

> 
> Also - i guess it will depend on the underlying driver?
> This seems very related to a specific driver:
> "Warning: mlx5_core: matching on ct_state +new isn't supported."
> Debuggability is always great but so is backwards compat.
> What happens when you run old userspace tc? There are tons
> of punting systems that process these events out there and
> depend on the current event messages as is.

I think old tc should just ignore this NLMSGERR_ATTR_MSG?

Thanks
Hangbin

Re: [PATCH (repost) net-next] sched: add extack for tfilter_notify

[Hangbin Liu]

Hi Jamal,

Any comments?

Thanks
Hangbin
On Wed, Oct 26, 2022 at 05:58:14PM +0800, Hangbin Liu wrote:
> On Sun, Oct 02, 2022 at 11:27:08AM -0400, Jamal Hadi Salim wrote:
> > > But, precisely. In the example Hangbin gave, it is showing why the
> > > entry is not_in_hw. That's still data that belongs to the event that
> > > happened and that can't be queried afterwards even if the user/app
> > > monitoring it want to. Had it failed entirely, I agree, as the control
> > > path never changed.
> > >
> > > tc monitor is easier to use than perf probes in some systems. It's not
> > > uncommon to have tc installed but not perf. It's also easier to ask a
> > > customer to run it than explain how to enable the tracepoint and print
> > > ftrace buffer via /sys files, and the output is more meaningful for us
> > > as well: we know exactly which filter triggered the message. The only
> > > other place that we can correlate the filter and the warning, is on
> > > vswitchd log. Which is not easy to read either.
> > 
> > To Jakub's point: I think one of those NLMSGERR TLVs is the right place
> > and while traces look attractive I see the value of having a unified
> > collection point via the tc monitor.
> 
> Hi Jamal,
> 
> Sorry for the late response. I just came back form vacation. For this issue,
> I saw netlink_dump_done() also put NLMSGERR_ATTR_MSG in NLMSG_DONE.
> So why can't we do the same here?
> 
> In https://www.kernel.org/doc/html//next/userspace-api/netlink/intro.html,
> The "optionally extended ACK" in NLMSG_DONE is OK.
> 
> > Since you cant really batch events - it seems the NLMSG_DONE/MULTI
> > hack is done just to please iproute2::tc?
> 
> Yes.
> 
> > IMO:
> > I think if you need to do this, then you have to teach iproute2
> > new ways of interpreting the message (which is nice because you
> > dont have to worry about backward compat). Some of that code
> > should be centralized and reused by netlink generically
> > instead of just cls_api, example the whole NLM_F_ACK_TLVS dance.
> 
> Would you please help explain more about this?
> 
> > 
> > Also - i guess it will depend on the underlying driver?
> > This seems very related to a specific driver:
> > "Warning: mlx5_core: matching on ct_state +new isn't supported."
> > Debuggability is always great but so is backwards compat.
> > What happens when you run old userspace tc? There are tons
> > of punting systems that process these events out there and
> > depend on the current event messages as is.
> 
> I think old tc should just ignore this NLMSGERR_ATTR_MSG?
> 
> Thanks
> Hangbin

Re: [PATCH (repost) net-next] sched: add extack for tfilter_notify

[Jamal Hadi Salim]

On Wed, Oct 26, 2022 at 5:58 AM Hangbin Liu <liuhangbin@gmail.com> wrote:
>
> On Sun, Oct 02, 2022 at 11:27:08AM -0400, Jamal Hadi Salim wrote:
> > > But, precisely. In the example Hangbin gave, it is showing why the
> > > entry is not_in_hw. That's still data that belongs to the event that
> > > happened and that can't be queried afterwards even if the user/app
> > > monitoring it want to. Had it failed entirely, I agree, as the control
> > > path never changed.
> > >
> > > tc monitor is easier to use than perf probes in some systems. It's not
> > > uncommon to have tc installed but not perf. It's also easier to ask a
> > > customer to run it than explain how to enable the tracepoint and print
> > > ftrace buffer via /sys files, and the output is more meaningful for us
> > > as well: we know exactly which filter triggered the message. The only
> > > other place that we can correlate the filter and the warning, is on
> > > vswitchd log. Which is not easy to read either.
> >
> > To Jakub's point: I think one of those NLMSGERR TLVs is the right place
> > and while traces look attractive I see the value of having a unified
> > collection point via the tc monitor.
>

Sorry for the latency - was at conference and still in travel mode...

> Hi Jamal,
>
> Sorry for the late response. I just came back form vacation. For this issue,
> I saw netlink_dump_done() also put NLMSGERR_ATTR_MSG in NLMSG_DONE.
> So why can't we do the same here?
>
> In https://www.kernel.org/doc/html//next/userspace-api/netlink/intro.html,
> The "optionally extended ACK" in NLMSG_DONE is OK.
>

Ok.
[That seemd to  be a nice doc - need to find time to look at it]

> > IMO:
> > I think if you need to do this, then you have to teach iproute2
> > new ways of interpreting the message (which is nice because you
> > dont have to worry about backward compat). Some of that code
> > should be centralized and reused by netlink generically
> > instead of just cls_api, example the whole NLM_F_ACK_TLVS dance.
>
> Would you please help explain more about this?
>

I meant you only added it for filter notification - but such a feature would
be useful also for other tc pieces (like actions and qdiscs). Is there a better
way to do it such that the other tc parts may benefit (instead of just
filter_notify?).

> >
> > Also - i guess it will depend on the underlying driver?
> > This seems very related to a specific driver:
> > "Warning: mlx5_core: matching on ct_state +new isn't supported."
> > Debuggability is always great but so is backwards compat.
> > What happens when you run old userspace tc? There are tons
> > of punting systems that process these events out there and
> > depend on the current event messages as is.
>
> I think old tc should just ignore this NLMSGERR_ATTR_MSG?

Yes.
So looks good to me then.

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal

Re: [PATCH (repost) net-next] sched: add extack for tfilter_notify

[Jakub Kicinski]

On Wed, 2 Nov 2022 11:33:18 -0400 Jamal Hadi Salim wrote:
> > Sorry for the late response. I just came back form vacation. For this issue,
> > I saw netlink_dump_done() also put NLMSGERR_ATTR_MSG in NLMSG_DONE.
> > So why can't we do the same here?
> >
> > In https://www.kernel.org/doc/html//next/userspace-api/netlink/intro.html,
> > The "optionally extended ACK" in NLMSG_DONE is OK.
> 
> Ok.
> [That seemd to  be a nice doc - need to find time to look at it]

Thanks.

> > > Also - i guess it will depend on the underlying driver?
> > > This seems very related to a specific driver:
> > > "Warning: mlx5_core: matching on ct_state +new isn't supported."
> > > Debuggability is always great but so is backwards compat.
> > > What happens when you run old userspace tc? There are tons
> > > of punting systems that process these events out there and
> > > depend on the current event messages as is.  
> >
> > I think old tc should just ignore this NLMSGERR_ATTR_MSG?  
> 
> Yes.
> So looks good to me then.
> 
> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

Eish.

Hangbin, I'm still against this. Please go back to my suggestions /
questions. A tracepoint or an attribute should do. Multi-part messages
are very hard to map to normal programming constructs, and I don't
think there is any precedent for mutli-part notifications.

Re: [PATCH (repost) net-next] sched: add extack for tfilter_notify

[Hangbin Liu]

On Wed, Nov 02, 2022 at 04:36:46PM -0700, Jakub Kicinski wrote:
> > I meant you only added it for filter notification - but such a feature would
> > be useful also for other tc pieces (like actions and qdiscs). Is there a better
> > way to do it such that the other tc parts may benefit (instead of just
> > filter_notify?).
> > 
> > Yes.
> > So looks good to me then.
> > 
> > Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

Thanks Jamal for the comments.

> 
> Eish.
> 
> Hangbin, I'm still against this. Please go back to my suggestions /
> questions. A tracepoint or an attribute should do. Multi-part messages
> are very hard to map to normal programming constructs, and I don't
> think there is any precedent for mutli-part notifications.

OK, I will re-consider about how to implement it via tracepoint or an attribute.

Thanks
Hangbin

Re: [PATCH (repost) net-next] sched: add extack for tfilter_notify

[Hangbin Liu]

On Wed, Nov 02, 2022 at 04:36:46PM -0700, Jakub Kicinski wrote:
> Eish.
> 
> Hangbin, I'm still against this. Please go back to my suggestions /
> questions. A tracepoint or an attribute should do. Multi-part messages
> are very hard to map to normal programming constructs, and I don't
> think there is any precedent for mutli-part notifications.

Hi Jakub,

I checked the doc[1], the NLMSGERR_ATTR_MSG could only be in NLMSG_ERROR and
NLMSG_DONE messages. But the tfilter_notify() set the nlmsg type to
RTM_NEWTFILTER. Would you like to help explain what you mean of using
attribute? Should I send a NLMSG_ERROR/NLMSG_DONE message separately after the
tfilter_notify()?

[1] https://www.kernel.org/doc/html//next/userspace-api/netlink/intro.html#ext-ack

Thanks
Hangbin

Re: [PATCH (repost) net-next] sched: add extack for tfilter_notify

[Jakub Kicinski]

On Tue, 8 Nov 2022 17:11:22 +0800 Hangbin Liu wrote:
> On Wed, Nov 02, 2022 at 04:36:46PM -0700, Jakub Kicinski wrote:
> > Eish.
> > 
> > Hangbin, I'm still against this. Please go back to my suggestions /
> > questions. A tracepoint or an attribute should do. Multi-part messages
> > are very hard to map to normal programming constructs, and I don't
> > think there is any precedent for mutli-part notifications.  
> 
> Hi Jakub,
> 
> I checked the doc[1], the NLMSGERR_ATTR_MSG could only be in NLMSG_ERROR and
> NLMSG_DONE messages. But the tfilter_notify() set the nlmsg type to
> RTM_NEWTFILTER. Would you like to help explain what you mean of using
> attribute? Should I send a NLMSG_ERROR/NLMSG_DONE message separately after the
> tfilter_notify()?
> 
> [1] https://www.kernel.org/doc/html//next/userspace-api/netlink/intro.html#ext-ack

My initial thought was to add an attribute type completely independent
of the attribute space defined in enum nlmsgerr_attrs, add it in the
TCA_* space. So for example add a TCA_NTF_WARN_MSG which will carry the
string message.

We can also create a nest to carry the full nlmsgerr_attrs attributes
with their existing types (TCA_NTF_EXT_ACK?). Each nest gets
to choose what attribute set it carries.

That said, most of the ext_ack attributes refer to an input attribute by
specifying the offset within the request. The notification recipient
will not be able to resolve those in any meaningful way. So since only
the string message will be of interest I reckon adding a full nest is
an unnecessary complication?

Re: [PATCH (repost) net-next] sched: add extack for tfilter_notify

[Hangbin Liu]

On Tue, Nov 08, 2022 at 10:55:44AM -0800, Jakub Kicinski wrote:
> My initial thought was to add an attribute type completely independent
> of the attribute space defined in enum nlmsgerr_attrs, add it in the
> TCA_* space. So for example add a TCA_NTF_WARN_MSG which will carry the
> string message.
> 
> We can also create a nest to carry the full nlmsgerr_attrs attributes
> with their existing types (TCA_NTF_EXT_ACK?). Each nest gets
> to choose what attribute set it carries.
> 
> That said, most of the ext_ack attributes refer to an input attribute by
> specifying the offset within the request. The notification recipient
> will not be able to resolve those in any meaningful way. So since only
> the string message will be of interest I reckon adding a full nest is
> an unnecessary complication?

Thanks for the explanation. I will try add the TCA_NTF_WARN_MSG to TCA
space.

Hangbin

Re: [PATCH (repost) net-next] sched: add extack for tfilter_notify

[Jamal Hadi Salim]

TCA_XXX are local whereas NLMSGERR_ATTR_MSG global to the
netlink message. Does this mean to replicate TCA_NTF_EXT_ACK
for all objects when needed? (qdiscs, actions, etc).

cheers,
jamal

On Wed, Nov 9, 2022 at 6:53 AM Hangbin Liu <liuhangbin@gmail.com> wrote:
>
> On Tue, Nov 08, 2022 at 10:55:44AM -0800, Jakub Kicinski wrote:
> > My initial thought was to add an attribute type completely independent
> > of the attribute space defined in enum nlmsgerr_attrs, add it in the
> > TCA_* space. So for example add a TCA_NTF_WARN_MSG which will carry the
> > string message.
> >
> > We can also create a nest to carry the full nlmsgerr_attrs attributes
> > with their existing types (TCA_NTF_EXT_ACK?). Each nest gets
> > to choose what attribute set it carries.
> >
> > That said, most of the ext_ack attributes refer to an input attribute by
> > specifying the offset within the request. The notification recipient
> > will not be able to resolve those in any meaningful way. So since only
> > the string message will be of interest I reckon adding a full nest is
> > an unnecessary complication?
>
> Thanks for the explanation. I will try add the TCA_NTF_WARN_MSG to TCA
> space.
>
> Hangbin

Re: [PATCH (repost) net-next] sched: add extack for tfilter_notify

[Jakub Kicinski]

On Wed, 9 Nov 2022 20:52:37 -0500 Jamal Hadi Salim wrote:
> TCA_XXX are local whereas NLMSGERR_ATTR_MSG global to the
> netlink message. 

"Global", but they necessitate complicating the entire protocol 
to use directly.

Unless we want to create a separate netlink multicast channel for 
just ext acks of a family. That's fine by me, I guess. I'm mostly
objecting to pretending notifications are multi-msg just to reuse
NLMSG_DONE, and forcing all notification listeners to deal with it.

> Does this mean to replicate TCA_NTF_EXT_ACK
> for all objects when needed? (qdiscs, actions, etc).

The more time we spend discussing this the more I'm inclined to say
"this is a typical tracing use case, just use the tracepoint" :(

Re: [PATCH (repost) net-next] sched: add extack for tfilter_notify

[Hangbin Liu]

On Wed, Nov 09, 2022 at 06:20:53PM -0800, Jakub Kicinski wrote:
> Unless we want to create a separate netlink multicast channel for 
> just ext acks of a family. That's fine by me, I guess. I'm mostly
> objecting to pretending notifications are multi-msg just to reuse
> NLMSG_DONE, and forcing all notification listeners to deal with it.

Hi Jakub,

Actually I'm a little curious about how should we use NLMSG_DONE.
Does a normal nlmsg(with NLM_F_MULTI flag) + a NLMSG_DONE msg illegal?
Should we need at least  2 nlmsgs + a NLMSG_DONE message.

Because when I wrote this patch, I saw some functions, like
team_nl_send_options_get(), team_nl_send_port_list_get() in team driver,
devlink_dpipe_tables_fill() in netlink.c, even netlink_dump_done(), could
*possible* only have 1 nlmsg + 1 NLMSG_DONE message.

In my understand, we can send only 1 nlmsg without NLM_F_MULTI flag. But if
there is 1 nlmsg + 1 NLMSG_DONE message. It should be considered as multi
message, and the first nlmsg need to add NLM_F_MULTI flag. Maybe there is
a little abuse of using NLMSG_DONE, but should be legal.

What do you think? Did I miss something?

Thanks
Hangbin

Re: [PATCH (repost) net-next] sched: add extack for tfilter_notify

[Jakub Kicinski]

On Thu, 10 Nov 2022 14:29:29 +0800 Hangbin Liu wrote:
> On Wed, Nov 09, 2022 at 06:20:53PM -0800, Jakub Kicinski wrote:
> > Unless we want to create a separate netlink multicast channel for 
> > just ext acks of a family. That's fine by me, I guess. I'm mostly
> > objecting to pretending notifications are multi-msg just to reuse
> > NLMSG_DONE, and forcing all notification listeners to deal with it.  
> 
> Actually I'm a little curious about how should we use NLMSG_DONE.
> Does a normal nlmsg(with NLM_F_MULTI flag) + a NLMSG_DONE msg illegal?
> Should we need at least  2 nlmsgs + a NLMSG_DONE message.
> 
> Because when I wrote this patch, I saw some functions, like
> team_nl_send_options_get(), team_nl_send_port_list_get() in team driver,
> devlink_dpipe_tables_fill() in netlink.c, even netlink_dump_done(), could
> *possible* only have 1 nlmsg + 1 NLMSG_DONE message.
> 
> In my understand, we can send only 1 nlmsg without NLM_F_MULTI flag. But if
> there is 1 nlmsg + 1 NLMSG_DONE message. It should be considered as multi
> message, and the first nlmsg need to add NLM_F_MULTI flag. Maybe there is
> a little abuse of using NLMSG_DONE, but should be legal.
> 
> What do you think? Did I miss something?

No, I mean, it's perfectly legal to send a single message with MULTI
and NLMSG_DONE, but it's hard to deserialize that. You can hand parse
anything, but how would you describe that in terms of abstract objects
that a normal high level language can consume directly?

Re: [PATCH (repost) net-next] sched: add extack for tfilter_notify

[Jamal Hadi Salim]

On Wed, Nov 9, 2022 at 9:20 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 9 Nov 2022 20:52:37 -0500 Jamal Hadi Salim wrote:
> > TCA_XXX are local whereas NLMSGERR_ATTR_MSG global to the
> > netlink message.
>
> "Global", but they necessitate complicating the entire protocol
> to use directly.
>
> Unless we want to create a separate netlink multicast channel for
> just ext acks of a family. That's fine by me, I guess. I'm mostly
> objecting to pretending notifications are multi-msg just to reuse
> NLMSG_DONE, and forcing all notification listeners to deal with it.
>

TBH, I am struggling as well. NLMSG_DONE is really for multi-message
(with kernel state) like dumps. Could we just extend nlmsg_notify()
callers to take extack and pass it through and then have  nlmsg_notify()
do the NLM_F_ACK_TLVS dance without MULTI flag? It would have to
be backward compat and require user space changes which Hangbin's
patch avoids but will be more general.

> > Does this mean to replicate TCA_NTF_EXT_ACK
> > for all objects when needed? (qdiscs, actions, etc).
>
> The more time we spend discussing this the more I'm inclined to say
> "this is a typical tracing use case, just use the tracepoint" :(

I understand your frustration but from an operational pov it is better to deal
with one tool than two (Marcelo's point). The way i look at these uapi
discussions
is it is ok to discuss the color of the bike shed(within reason) because any
decisions made here will have a long term effect.

cheers,
jamal

Re: [PATCH (repost) net-next] sched: add extack for tfilter_notify

[Jakub Kicinski]

On Thu, 10 Nov 2022 09:27:40 -0500 Jamal Hadi Salim wrote:
> > "Global", but they necessitate complicating the entire protocol
> > to use directly.
> >
> > Unless we want to create a separate netlink multicast channel for
> > just ext acks of a family. That's fine by me, I guess. I'm mostly
> > objecting to pretending notifications are multi-msg just to reuse
> > NLMSG_DONE, and forcing all notification listeners to deal with it.
> 
> TBH, I am struggling as well. NLMSG_DONE is really for multi-message
> (with kernel state) like dumps. Could we just extend nlmsg_notify()
> callers to take extack and pass it through and then have  nlmsg_notify()
> do the NLM_F_ACK_TLVS dance without MULTI flag? It would have to
> be backward compat and require user space changes which Hangbin's
> patch avoids but will be more general.

I think we'd need some sort of "internal / netlink level attributes"
to do that. We have only one attribute "space" inside any message,
defined by the family itself. So attribute type 1 for a TCA notification
is TCA_KIND, not NLMSGERR_ATTR_MSG.

We'd need changes to struct nlmsghdr to allow the nlmsghdr to have its
own set of attributes. Would be cool, but major surgery at this point.
I guess we could assume the families don't use high attr types, and say
that attr types > 0x400 are reserved for netlink. Put NLMSG_ATTRs there.
Seems risky, tho.

> > The more time we spend discussing this the more I'm inclined to say
> > "this is a typical tracing use case, just use the tracepoint" :(  
> 
> I understand your frustration but from an operational pov it is
> better to deal with one tool than two (Marcelo's point).

IDK, we can have a kernel hook into the trace point and generate 
the output over netlink, like we do with drop monitor and skb_free().
But I really doubt that its worth it. Also you can put a USDT into OvS
if you don't want to restart it. There are many options, not everything
is a nail :S

> The way i look at these uapi discussions is it is ok to discuss the
> color of the bike shed(within reason) because any decisions made here
> will have a long term effect.

To stretch the analogy - in my mind we have way too many one-off, 
odd looking bike sheds and not enough bikes (users) with netlink.

So anything that reads to me like "ooh, look at this neat trick 
I can do with netlink that I can totally hand parse in iproute2" 
rises the hair on my back :(

Re: [PATCH (repost) net-next] sched: add extack for tfilter_notify

[Hangbin Liu]

On Thu, Nov 10, 2022 at 09:27:09AM -0800, Jakub Kicinski wrote:
> > I understand your frustration but from an operational pov it is
> > better to deal with one tool than two (Marcelo's point).
> 
> IDK, we can have a kernel hook into the trace point and generate 
> the output over netlink, like we do with drop monitor and skb_free().
> But I really doubt that its worth it. Also you can put a USDT into OvS
> if you don't want to restart it. There are many options, not everything
> is a nail :S

I have finished a patch with TCA_NTF_WARN_MSG to carry the string message.
But looks our discussion goes to a way that this feature is not valuable?

So maybe I should stop on here?

Anyway, thanks a lot for your time and comments.

Hangbin

Re: [PATCH (repost) net-next] sched: add extack for tfilter_notify

[Jakub Kicinski]

On Tue, 15 Nov 2022 11:07:21 +0800 Hangbin Liu wrote:
> > IDK, we can have a kernel hook into the trace point and generate 
> > the output over netlink, like we do with drop monitor and skb_free().
> > But I really doubt that its worth it. Also you can put a USDT into OvS
> > if you don't want to restart it. There are many options, not everything
> > is a nail :S  
> 
> I have finished a patch with TCA_NTF_WARN_MSG to carry the string message.
> But looks our discussion goes to a way that this feature is not valuable?
> 
> So maybe I should stop on here?

It's a bit of a catch 22 - I don't mind the TCA_NTF_WARN_MSG itself 
but I would prefer for the extack via notification to spread to other
notifications.

If you have the code ready - post it, let's see how folks feel after
sleeping on it.

Re: [PATCH (repost) net-next] sched: add extack for tfilter_notify

[Jamal Hadi Salim]

On Mon, Nov 14, 2022 at 11:51 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 15 Nov 2022 11:07:21 +0800 Hangbin Liu wrote:

[..]
> > I have finished a patch with TCA_NTF_WARN_MSG to carry the string message.
> > But looks our discussion goes to a way that this feature is not valuable?
> >
> > So maybe I should stop on here?
>
> It's a bit of a catch 22 - I don't mind the TCA_NTF_WARN_MSG itself
> but I would prefer for the extack via notification to spread to other
> notifications.
>

+1.
I got distracted but was thinking of  sending a sample patch. I think
we can do it for all if we pass extack to nlmsg_notify() from all callers
and then check if extack is present and do the NLM_F_ACK_TLVS
dance in there.
Hangbin - maybe consider this approach?

cheers,
jamal

> If you have the code ready - post it, let's see how folks feel after
> sleeping on it.

Re: [PATCH (repost) net-next] sched: add extack for tfilter_notify

[Hangbin Liu]

On Mon, Nov 14, 2022 at 08:51:43PM -0800, Jakub Kicinski wrote:
> > So maybe I should stop on here?
> 
> It's a bit of a catch 22 - I don't mind the TCA_NTF_WARN_MSG itself 
> but I would prefer for the extack via notification to spread to other
> notifications.

Not sure if we could find a way to pass the GROUP ID to netlink_ack(),
and use nlmsg_notify() instead of nlmsg_unicast() in it. Then the tc monitor
could handle the NLMSG_ERROR directly.

> 
> If you have the code ready - post it, let's see how folks feel after
> sleeping on it.

I just add a new TCA_NTF_WARN_MSG enum and put the extack message here.
Is this what you want?

diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index eb2747d58a81..6952573e7054 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h
@@ -635,6 +635,7 @@ enum {
 	TCA_INGRESS_BLOCK,
 	TCA_EGRESS_BLOCK,
 	TCA_DUMP_FLAGS,
+	TCA_NTF_WARN_MSG,
 	__TCA_MAX
 };
 
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 23d1cfa4f58c..204181cec215 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -1816,7 +1816,8 @@ static int tcf_fill_node(struct net *net, struct sk_buff *skb,
 			 struct tcf_proto *tp, struct tcf_block *block,
 			 struct Qdisc *q, u32 parent, void *fh,
 			 u32 portid, u32 seq, u16 flags, int event,
-			 bool terse_dump, bool rtnl_held)
+			 bool terse_dump, bool rtnl_held,
+			 struct netlink_ext_ack *extack)
 {
 	struct tcmsg *tcm;
 	struct nlmsghdr  *nlh;
@@ -1856,6 +1857,11 @@ static int tcf_fill_node(struct net *net, struct sk_buff *skb,
 		    tp->ops->dump(net, tp, fh, skb, tcm, rtnl_held) < 0)
 			goto nla_put_failure;
 	}
+
+	if (extack && extack->_msg &&
+	    nla_put_string(skb, TCA_NTF_WARN_MSG, extack->_msg))
+			goto nla_put_failure;
+
 	nlh->nlmsg_len = skb_tail_pointer(skb) - b;
 	return skb->len;

Thanks
Hangbin

Re: [PATCH (repost) net-next] sched: add extack for tfilter_notify

[Jamal Hadi Salim]

On Tue, Nov 15, 2022 at 7:44 AM Hangbin Liu <liuhangbin@gmail.com> wrote:
>
> On Mon, Nov 14, 2022 at 08:51:43PM -0800, Jakub Kicinski wrote:
> > > So maybe I should stop on here?
> >
> > It's a bit of a catch 22 - I don't mind the TCA_NTF_WARN_MSG itself
> > but I would prefer for the extack via notification to spread to other
> > notifications.
>
> Not sure if we could find a way to pass the GROUP ID to netlink_ack(),
> and use nlmsg_notify() instead of nlmsg_unicast() in it. Then the tc monitor
> could handle the NLMSG_ERROR directly.
>

That's what I meant. Do you have time to try this? Otherwise i will make time.
Your patch is still very specific to cls. If you only look at h/w
offload, actions can also
be added independently and fail independently as well. But in general this would
be useful for all notifications.

cheers,
jamal

Re: [PATCH (repost) net-next] sched: add extack for tfilter_notify

[Hangbin Liu]

On Tue, Nov 15, 2022 at 08:13:15AM -0500, Jamal Hadi Salim wrote:
> > > but I would prefer for the extack via notification to spread to other
> > > notifications.
> >
> > Not sure if we could find a way to pass the GROUP ID to netlink_ack(),
> > and use nlmsg_notify() instead of nlmsg_unicast() in it. Then the tc monitor
> > could handle the NLMSG_ERROR directly.
> >
> 
> That's what I meant. Do you have time to try this? Otherwise i will make time.

Yes, I can have a try.

> Your patch is still very specific to cls. If you only look at h/w
> offload, actions can also
> be added independently and fail independently as well. But in general this would
> be useful for all notifications.

I saw your last mail mean to pass extack to nlmsg_notify() and do
the NLM_F_ACK_TLVS there. This is more agile but need to re-write what
netlink_ack() does. If we pass GROUP ID to netlink_ack() directly, we can
save this work. But from the call path like netlink_rcv_skb() ->
netlink_ack(). I don't have a good way to get the GROUP ID.

Should we write a new helper to convert the GROUP ID from nlmsg_type?
On the other hand, if we add this helper, not suer if we should only
add RTNLGRP_TC first in case other field do not want to multicast the ack
message?

Thanks
Hangbin

Re: [PATCH (repost) net-next] sched: add extack for tfilter_notify

[Jamal Hadi Salim]

On Tue, Nov 15, 2022 at 8:57 AM Hangbin Liu <liuhangbin@gmail.com> wrote:
>
> On Tue, Nov 15, 2022 at 08:13:15AM -0500, Jamal Hadi Salim wrote:

> > That's what I meant. Do you have time to try this? Otherwise i will make time.
>
> Yes, I can have a try.
>
> > Your patch is still very specific to cls. If you only look at h/w
> > offload, actions can also
> > be added independently and fail independently as well. But in general this would
> > be useful for all notifications.
>
> I saw your last mail mean to pass extack to nlmsg_notify() and do
> the NLM_F_ACK_TLVS there. This is more agile but need to re-write what
> netlink_ack() does. If we pass GROUP ID to netlink_ack() directly, we can
> save this work. But from the call path like netlink_rcv_skb() ->
> netlink_ack(). I don't have a good way to get the GROUP ID.
>
> Should we write a new helper to convert the GROUP ID from nlmsg_type?
> On the other hand, if we add this helper, not suer if we should only
> add RTNLGRP_TC first in case other field do not want to multicast the ack
> message?
>

Ok, I think i get what Jakub meant by "internal / netlink level attributes" now.
i.e the problem is there is a namespace collision for attributes between
the global NLMSG_ERROR and service local. It is more tricky than i thought.
How were you thinking of storing the extack info into the message?
Note: I wouldnt touch netlink_ack() - maybe write a separate helper for events.

cheers,
jamal

Re: [PATCH (repost) net-next] sched: add extack for tfilter_notify

[Hangbin Liu]

On Tue, Nov 15, 2022 at 11:26:54AM -0500, Jamal Hadi Salim wrote:
> Ok, I think i get what Jakub meant by "internal / netlink level attributes" now.
> i.e the problem is there is a namespace collision for attributes between
> the global NLMSG_ERROR and service local. It is more tricky than i thought.
> How were you thinking of storing the extack info into the message?

Not sure, I will have a try first. FYI, Next week I will be in vacation.
So there would be some delay on this...

> Note: I wouldnt touch netlink_ack() - maybe write a separate helper for events.

Hmm, OK, I will try not touch netlink_ack().

Thanks
Hangbin

Re: [PATCH (repost) net-next] sched: add extack for tfilter_notify

[Hangbin Liu]

Hi Jamal,

On Tue, Nov 15, 2022 at 11:26:54AM -0500, Jamal Hadi Salim wrote:
> How were you thinking of storing the extack info into the message?
> Note: I wouldnt touch netlink_ack() - maybe write a separate helper for events.

It looks a separate helper is just duplicated codes. Send the notify from
the netlink_ack() is much easy. e.g.

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index a662e8a5ff84..79a1586e8eb0 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -2463,6 +2463,33 @@ netlink_ack_tlv_fill(struct sk_buff *in_skb, struct sk_buff *skb,
 				    (u8 *)extack->miss_nest - (u8 *)nlh));
 }
 
+/**
+ * get_nlgroup - get netlink group id based on nlmsg_type
+ * @nlmsg_type: netlink message type
+ *
+ * Right now, we only support net tc sched. Please add other types if the
+ * sub maintainers feel needed.
+ */
+unsigned int get_nlgroup(__u16 nlmsg_type)
+{
+	switch (nlmsg_type) {
+	case RTM_NEWTFILTER:
+	case RTM_DELTFILTER:
+	case RTM_NEWCHAIN:
+	case RTM_DELCHAIN:
+	case RTM_NEWTCLASS:
+	case RTM_DELTCLASS:
+	case RTM_NEWQDISC:
+	case RTM_DELQDISC:
+	case RTM_GETACTION:
+	case RTM_NEWACTION:
+	case RTM_DELACTION:
+		return RTNLGRP_TC;
+	}
+
+	return 0;
+}
+
 void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err,
 		 const struct netlink_ext_ack *extack)
 {
@@ -2471,7 +2498,7 @@ void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err,
 	struct nlmsgerr *errmsg;
 	size_t payload = sizeof(*errmsg);
 	struct netlink_sock *nlk = nlk_sk(NETLINK_CB(in_skb).sk);
-	unsigned int flags = 0;
+	unsigned int group, flags = 0;
 	size_t tlvlen;
 
 	/* Error messages get the original request appened, unless the user
@@ -2507,7 +2534,9 @@ void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err,
 
 	nlmsg_end(skb, rep);
 
-	nlmsg_unicast(in_skb->sk, skb, NETLINK_CB(in_skb).portid);
+	group = get_nlgroup(nlh->nlmsg_type);
+	/* Multicast the error message if we can get group if from nlmsg_type */
+	nlmsg_notify(in_skb->sk, skb, NETLINK_CB(in_skb).portid, group, 1, GFP_KERNEL);
 }
 EXPORT_SYMBOL(netlink_ack);
 

What do you think?

Thanks
Hangbin

Re: [PATCH (repost) net-next] sched: add extack for tfilter_notify

[Jakub Kicinski]

On Tue, 29 Nov 2022 16:07:04 +0800 Hangbin Liu wrote:
> +unsigned int get_nlgroup(__u16 nlmsg_type)
> +{
> +	switch (nlmsg_type) {
> +	case RTM_NEWTFILTER:
> +	case RTM_DELTFILTER:
> +	case RTM_NEWCHAIN:
> +	case RTM_DELCHAIN:
> +	case RTM_NEWTCLASS:
> +	case RTM_DELTCLASS:
> +	case RTM_NEWQDISC:
> +	case RTM_DELQDISC:
> +	case RTM_GETACTION:
> +	case RTM_NEWACTION:
> +	case RTM_DELACTION:
> +		return RTNLGRP_TC;
> +	}

These are rtnl message ids, they don't belong in af_netlink.

Re: [PATCH (repost) net-next] sched: add extack for tfilter_notify

[Hangbin Liu]

On Tue, Nov 29, 2022 at 07:43:20AM -0800, Jakub Kicinski wrote:
> On Tue, 29 Nov 2022 16:07:04 +0800 Hangbin Liu wrote:
> > +unsigned int get_nlgroup(__u16 nlmsg_type)
> > +{
> > +	switch (nlmsg_type) {
> > +	case RTM_NEWTFILTER:
> > +	case RTM_DELTFILTER:
> > +	case RTM_NEWCHAIN:
> > +	case RTM_DELCHAIN:
> > +	case RTM_NEWTCLASS:
> > +	case RTM_DELTCLASS:
> > +	case RTM_NEWQDISC:
> > +	case RTM_DELQDISC:
> > +	case RTM_GETACTION:
> > +	case RTM_NEWACTION:
> > +	case RTM_DELACTION:
> > +		return RTNLGRP_TC;
> > +	}
> 
> These are rtnl message ids, they don't belong in af_netlink.

Oh, thanks very much! Now I got what Jamal said the
"namespace collision for attributes". I will see where we should
put the new helper.

Hangbin