netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCHv2 net-next] sched: multicast sched extack messages
@ 2022-12-21  9:39 Hangbin Liu
  2022-12-22  1:28 ` Jakub Kicinski
  0 siblings, 1 reply; 10+ messages in thread
From: Hangbin Liu @ 2022-12-21  9:39 UTC (permalink / raw)
  To: netdev
  Cc: Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, David Ahern,
	Hangbin Liu, Marcelo Ricardo Leitner

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 a new function
to report the extack message 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>
---
v2: use NLMSG_ERROR instad of NLMSG_DONE to report the extack message
---
 net/sched/cls_api.c | 61 ++++++++++++++++++++++++++++++++++-----------
 1 file changed, 47 insertions(+), 14 deletions(-)

diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 668130f08903..a63262f0dc2c 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -1813,11 +1813,39 @@ static struct tcf_proto *tcf_chain_tp_find(struct tcf_chain *chain,
 	return tp;
 }
 
+static int tfilter_set_nl_ext(struct sk_buff *skb, const struct nlmsghdr *n,
+			      struct netlink_ext_ack *extack, u32 portid)
+{
+	struct nlmsgerr *errmsg;
+	struct nlmsghdr *nlh;
+
+	if (!extack || !extack->_msg)
+		return 0;
+
+	nlh = nlmsg_put(skb, portid, n->nlmsg_seq, NLMSG_ERROR, sizeof(*errmsg),
+			NLM_F_ACK_TLVS | NLM_F_CAPPED);
+	if (!nlh)
+		return -1;
+
+	errmsg = (struct nlmsgerr *)nlmsg_data(nlh);
+	errmsg->error = 0;
+	errmsg->msg = *n;
+
+	if (nla_put_string(skb, NLMSGERR_ATTR_MSG, extack->_msg))
+		return -1;
+
+	nlmsg_end(skb, nlh);
+
+	return 0;
+}
+
 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,
+			 const struct nlmsghdr *n,
+			 struct netlink_ext_ack *extack)
 {
 	struct tcmsg *tcm;
 	struct nlmsghdr  *nlh;
@@ -1858,6 +1886,10 @@ 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 ((flags & NLM_F_ACK) && tfilter_set_nl_ext(skb, n, extack, portid))
+		goto out_nlmsg_trim;
+
 	return skb->len;
 
 out_nlmsg_trim:
@@ -1871,7 +1903,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;
@@ -1883,7 +1915,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, n, extack) <= 0) {
 		kfree_skb(skb);
 		return -EINVAL;
 	}
@@ -1912,7 +1944,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, n, extack) <= 0) {
 		NL_SET_ERR_MSG(extack, "Failed to build del event notification");
 		kfree_skb(skb);
 		return -EINVAL;
@@ -1938,14 +1970,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)
@@ -2156,7 +2189,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)
@@ -2284,7 +2317,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;
@@ -2308,7 +2341,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;
 	}
@@ -2452,7 +2485,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");
 	}
@@ -2490,7 +2523,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, a->cb->nlh, NULL);
 }
 
 static bool tcf_chain_dump(struct tcf_chain *chain, struct Qdisc *q, u32 parent,
@@ -2524,7 +2557,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, cb->nlh, NULL) <= 0)
 				goto errout;
 			cb->args[1] = 1;
 		}
@@ -2912,7 +2945,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.38.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCHv2 net-next] sched: multicast sched extack messages
  2022-12-21  9:39 [PATCHv2 net-next] sched: multicast sched extack messages Hangbin Liu
@ 2022-12-22  1:28 ` Jakub Kicinski
  2022-12-22  7:48   ` Hangbin Liu
  0 siblings, 1 reply; 10+ messages in thread
From: Jakub Kicinski @ 2022-12-22  1:28 UTC (permalink / raw)
  To: Hangbin Liu
  Cc: netdev, Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S. Miller,
	Eric Dumazet, Paolo Abeni, David Ahern, Marcelo Ricardo Leitner

On Wed, 21 Dec 2022 17:39:40 +0800 Hangbin Liu wrote:
> +	nlh = nlmsg_put(skb, portid, n->nlmsg_seq, NLMSG_ERROR, sizeof(*errmsg),
> +			NLM_F_ACK_TLVS | NLM_F_CAPPED);
> +	if (!nlh)
> +		return -1;
> +
> +	errmsg = (struct nlmsgerr *)nlmsg_data(nlh);
> +	errmsg->error = 0;
> +	errmsg->msg = *n;
> +
> +	if (nla_put_string(skb, NLMSGERR_ATTR_MSG, extack->_msg))
> +		return -1;
> +
> +	nlmsg_end(skb, nlh);

I vote "no", notifications should not generate NLMSG_ERRORs.
(BTW setting pid and seq on notifications is odd, no?)

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCHv2 net-next] sched: multicast sched extack messages
  2022-12-22  1:28 ` Jakub Kicinski
@ 2022-12-22  7:48   ` Hangbin Liu
  2022-12-22 16:26     ` David Ahern
  0 siblings, 1 reply; 10+ messages in thread
From: Hangbin Liu @ 2022-12-22  7:48 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: netdev, Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S. Miller,
	Eric Dumazet, Paolo Abeni, David Ahern, Marcelo Ricardo Leitner

On Wed, Dec 21, 2022 at 05:28:17PM -0800, Jakub Kicinski wrote:
> On Wed, 21 Dec 2022 17:39:40 +0800 Hangbin Liu wrote:
> > +	nlh = nlmsg_put(skb, portid, n->nlmsg_seq, NLMSG_ERROR, sizeof(*errmsg),
> > +			NLM_F_ACK_TLVS | NLM_F_CAPPED);
> > +	if (!nlh)
> > +		return -1;
> > +
> > +	errmsg = (struct nlmsgerr *)nlmsg_data(nlh);
> > +	errmsg->error = 0;
> > +	errmsg->msg = *n;
> > +
> > +	if (nla_put_string(skb, NLMSGERR_ATTR_MSG, extack->_msg))
> > +		return -1;
> > +
> > +	nlmsg_end(skb, nlh);
> 
> I vote "no", notifications should not generate NLMSG_ERRORs.
> (BTW setting pid and seq on notifications is odd, no?)

I'm not sure if this error message should be counted to notifications generation.
The error message is generated as there is extack message, which is from
qdisc/filter adding/deleting.

Can't we multicast error message?

If we can't multicast the extack message via NLMSG_ERROR or NLMSG_DONE. I
think there is no other way to do it via netlink.

Thanks
Hangbin

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCHv2 net-next] sched: multicast sched extack messages
  2022-12-22  7:48   ` Hangbin Liu
@ 2022-12-22 16:26     ` David Ahern
  2022-12-23  2:35       ` Hangbin Liu
  0 siblings, 1 reply; 10+ messages in thread
From: David Ahern @ 2022-12-22 16:26 UTC (permalink / raw)
  To: Hangbin Liu, Jakub Kicinski
  Cc: netdev, Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S. Miller,
	Eric Dumazet, Paolo Abeni, Marcelo Ricardo Leitner

On 12/22/22 12:48 AM, Hangbin Liu wrote:
> On Wed, Dec 21, 2022 at 05:28:17PM -0800, Jakub Kicinski wrote:
>> On Wed, 21 Dec 2022 17:39:40 +0800 Hangbin Liu wrote:
>>> +	nlh = nlmsg_put(skb, portid, n->nlmsg_seq, NLMSG_ERROR, sizeof(*errmsg),
>>> +			NLM_F_ACK_TLVS | NLM_F_CAPPED);
>>> +	if (!nlh)
>>> +		return -1;
>>> +
>>> +	errmsg = (struct nlmsgerr *)nlmsg_data(nlh);
>>> +	errmsg->error = 0;
>>> +	errmsg->msg = *n;
>>> +
>>> +	if (nla_put_string(skb, NLMSGERR_ATTR_MSG, extack->_msg))
>>> +		return -1;
>>> +
>>> +	nlmsg_end(skb, nlh);
>>
>> I vote "no", notifications should not generate NLMSG_ERRORs.
>> (BTW setting pid and seq on notifications is odd, no?)
> 
> I'm not sure if this error message should be counted to notifications generation.
> The error message is generated as there is extack message, which is from
> qdisc/filter adding/deleting.
> 
> Can't we multicast error message?
> 
> If we can't multicast the extack message via NLMSG_ERROR or NLMSG_DONE. I
> think there is no other way to do it via netlink.
> 

it is confusing as an API to send back information or debugging strings
marked as an "error message."


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCHv2 net-next] sched: multicast sched extack messages
  2022-12-22 16:26     ` David Ahern
@ 2022-12-23  2:35       ` Hangbin Liu
  2022-12-26 16:31         ` Jamal Hadi Salim
  0 siblings, 1 reply; 10+ messages in thread
From: Hangbin Liu @ 2022-12-23  2:35 UTC (permalink / raw)
  To: David Ahern
  Cc: Jakub Kicinski, netdev, Jamal Hadi Salim, Cong Wang, Jiri Pirko,
	David S. Miller, Eric Dumazet, Paolo Abeni,
	Marcelo Ricardo Leitner

On Thu, Dec 22, 2022 at 09:26:14AM -0700, David Ahern wrote:
> On 12/22/22 12:48 AM, Hangbin Liu wrote:
> > On Wed, Dec 21, 2022 at 05:28:17PM -0800, Jakub Kicinski wrote:
> >> On Wed, 21 Dec 2022 17:39:40 +0800 Hangbin Liu wrote:
> >>> +	nlh = nlmsg_put(skb, portid, n->nlmsg_seq, NLMSG_ERROR, sizeof(*errmsg),
> >>> +			NLM_F_ACK_TLVS | NLM_F_CAPPED);
> >>> +	if (!nlh)
> >>> +		return -1;
> >>> +
> >>> +	errmsg = (struct nlmsgerr *)nlmsg_data(nlh);
> >>> +	errmsg->error = 0;
> >>> +	errmsg->msg = *n;
> >>> +
> >>> +	if (nla_put_string(skb, NLMSGERR_ATTR_MSG, extack->_msg))
> >>> +		return -1;
> >>> +
> >>> +	nlmsg_end(skb, nlh);
> >>
> >> I vote "no", notifications should not generate NLMSG_ERRORs.
> >> (BTW setting pid and seq on notifications is odd, no?)
> > 
> > I'm not sure if this error message should be counted to notifications generation.
> > The error message is generated as there is extack message, which is from
> > qdisc/filter adding/deleting.
> > 
> > Can't we multicast error message?
> > 
> > If we can't multicast the extack message via NLMSG_ERROR or NLMSG_DONE. I
> > think there is no other way to do it via netlink.
> > 
> 
> it is confusing as an API to send back information or debugging strings
> marked as an "error message."
> 

I think it's OK to send back information with error message. Based on rfc3549,

   An error code of zero indicates that the message is an ACK response.
   An ACK response message contains the original Netlink message header,
   which can be used to compare against (sent sequence numbers, etc).

I tried to do it on netlink_ack[1]. But Jakub pointed that the message ids
are not same families. So I moved it to net/sched.

I think the argue point is, can we multicast the error message?

[1] https://lore.kernel.org/all/Y4W9qEHzg5h9n%2Fod@Laptop-X1/

Thanks
Hangbin

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCHv2 net-next] sched: multicast sched extack messages
  2022-12-23  2:35       ` Hangbin Liu
@ 2022-12-26 16:31         ` Jamal Hadi Salim
  2022-12-27  3:58           ` Hangbin Liu
  0 siblings, 1 reply; 10+ messages in thread
From: Jamal Hadi Salim @ 2022-12-26 16:31 UTC (permalink / raw)
  To: Hangbin Liu
  Cc: David Ahern, Jakub Kicinski, netdev, Cong Wang, Jiri Pirko,
	David S. Miller, Eric Dumazet, Paolo Abeni,
	Marcelo Ricardo Leitner, Vlad Buslov

My only concern is this is not generic enough i.e can other objects
outside of filters do this?
You are still doing it only for the filter (in tfilter_set_nl_ext() -
sitting in cls_api)
As i mentioned earlier, actions can also be offloaded independently;
would this work with actions extack?
If it wont work then perhaps we should go the avenue of using
per-object(in this case filter) specific attributes
to carry the extack as suggested by Jakub earlier.

David, extacks are passed to user space today via NLMSG_ERROR  and in
this case the error is being returned by
the driver - so it makes sense to stick to that attribute so user
space can interpret it as such.

Jakub, I would argue this is an event given the original intent: Some
human (or daemon) tried to add an entry to
hardware and s/w (neither skip_sw nor skip_hw set in user space
request) and it failed because the hardware does
not support the request, and therefore the entry got added as to the
kernel only.
The event in this case is to tell whoever is listening that this
happened i.e half the request worked.

The secondary argument from Marcelo and Hangbin is: there is a
practical use for this, you reducing the amount
of operational tooling needed. Alternative is to get the extack
equivalent in syslog and the other from from events and
then do the heuristics.

cheers,
jamal

On Thu, Dec 22, 2022 at 9:35 PM Hangbin Liu <liuhangbin@gmail.com> wrote:
>
> On Thu, Dec 22, 2022 at 09:26:14AM -0700, David Ahern wrote:
> > On 12/22/22 12:48 AM, Hangbin Liu wrote:
> > > On Wed, Dec 21, 2022 at 05:28:17PM -0800, Jakub Kicinski wrote:
> > >> On Wed, 21 Dec 2022 17:39:40 +0800 Hangbin Liu wrote:
> > >>> + nlh = nlmsg_put(skb, portid, n->nlmsg_seq, NLMSG_ERROR, sizeof(*errmsg),
> > >>> +                 NLM_F_ACK_TLVS | NLM_F_CAPPED);
> > >>> + if (!nlh)
> > >>> +         return -1;
> > >>> +
> > >>> + errmsg = (struct nlmsgerr *)nlmsg_data(nlh);
> > >>> + errmsg->error = 0;
> > >>> + errmsg->msg = *n;
> > >>> +
> > >>> + if (nla_put_string(skb, NLMSGERR_ATTR_MSG, extack->_msg))
> > >>> +         return -1;
> > >>> +
> > >>> + nlmsg_end(skb, nlh);
> > >>
> > >> I vote "no", notifications should not generate NLMSG_ERRORs.
> > >> (BTW setting pid and seq on notifications is odd, no?)
> > >
> > > I'm not sure if this error message should be counted to notifications generation.
> > > The error message is generated as there is extack message, which is from
> > > qdisc/filter adding/deleting.
> > >
> > > Can't we multicast error message?
> > >
> > > If we can't multicast the extack message via NLMSG_ERROR or NLMSG_DONE. I
> > > think there is no other way to do it via netlink.
> > >
> >
> > it is confusing as an API to send back information or debugging strings
> > marked as an "error message."
> >
>
> I think it's OK to send back information with error message. Based on rfc3549,
>
>    An error code of zero indicates that the message is an ACK response.
>    An ACK response message contains the original Netlink message header,
>    which can be used to compare against (sent sequence numbers, etc).
>
> I tried to do it on netlink_ack[1]. But Jakub pointed that the message ids
> are not same families. So I moved it to net/sched.
>
> I think the argue point is, can we multicast the error message?
>
> [1] https://lore.kernel.org/all/Y4W9qEHzg5h9n%2Fod@Laptop-X1/
>
> Thanks
> Hangbin

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCHv2 net-next] sched: multicast sched extack messages
  2022-12-26 16:31         ` Jamal Hadi Salim
@ 2022-12-27  3:58           ` Hangbin Liu
  2022-12-27 16:23             ` Jamal Hadi Salim
  0 siblings, 1 reply; 10+ messages in thread
From: Hangbin Liu @ 2022-12-27  3:58 UTC (permalink / raw)
  To: Jamal Hadi Salim
  Cc: David Ahern, Jakub Kicinski, netdev, Cong Wang, Jiri Pirko,
	David S. Miller, Eric Dumazet, Paolo Abeni,
	Marcelo Ricardo Leitner, Vlad Buslov

Hi Jamal,
On Mon, Dec 26, 2022 at 11:31:24AM -0500, Jamal Hadi Salim wrote:
> My only concern is this is not generic enough i.e can other objects
> outside of filters do this?
> You are still doing it only for the filter (in tfilter_set_nl_ext() -
> sitting in cls_api)
> As i mentioned earlier, actions can also be offloaded independently;
> would this work with actions extack?
> If it wont work then perhaps we should go the avenue of using
> per-object(in this case filter) specific attributes
> to carry the extack as suggested by Jakub earlier.

Yes, I think we can do it on action objects, e.g. call tfilter_set_nl_ext()
in tca_get_fill:

tcf_add_notify() - tca_get_fill()

I will rename tfilter_set_nl_ext() to tc_set_nl_ext(). BTW, should we also
do it in qdisc/ class?

tclass_notify() - tc_fill_tclass()
qdisc_notify() - tc_fill_qdisc()

Thanks
Hangbin

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCHv2 net-next] sched: multicast sched extack messages
  2022-12-27  3:58           ` Hangbin Liu
@ 2022-12-27 16:23             ` Jamal Hadi Salim
  2022-12-28  8:41               ` Hangbin Liu
  0 siblings, 1 reply; 10+ messages in thread
From: Jamal Hadi Salim @ 2022-12-27 16:23 UTC (permalink / raw)
  To: Hangbin Liu
  Cc: David Ahern, Jakub Kicinski, netdev, Cong Wang, Jiri Pirko,
	David S. Miller, Eric Dumazet, Paolo Abeni,
	Marcelo Ricardo Leitner, Vlad Buslov

Hi Hangbin,

On Mon, Dec 26, 2022 at 10:58 PM Hangbin Liu <liuhangbin@gmail.com> wrote:
>
> Hi Jamal,
> On Mon, Dec 26, 2022 at 11:31:24AM -0500, Jamal Hadi Salim wrote:
> > My only concern is this is not generic enough i.e can other objects
> > outside of filters do this?
> > You are still doing it only for the filter (in tfilter_set_nl_ext() -
> > sitting in cls_api)
> > As i mentioned earlier, actions can also be offloaded independently;
> > would this work with actions extack?
> > If it wont work then perhaps we should go the avenue of using
> > per-object(in this case filter) specific attributes
> > to carry the extack as suggested by Jakub earlier.
>
> Yes, I think we can do it on action objects, e.g. call tfilter_set_nl_ext()
> in tca_get_fill:
>
> tcf_add_notify() - tca_get_fill()
>
> I will rename tfilter_set_nl_ext() to tc_set_nl_ext(). BTW, should we also
> do it in qdisc/ class?
>
> tclass_notify() - tc_fill_tclass()
> qdisc_notify() - tc_fill_qdisc()
>

The only useful cases imo are the ones that do h/w offload. So those two seem
reasonable. Not sure where you place that tc_set_nl_ext() so it is
visible for all.

cheers,
jamal

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCHv2 net-next] sched: multicast sched extack messages
  2022-12-27 16:23             ` Jamal Hadi Salim
@ 2022-12-28  8:41               ` Hangbin Liu
  2022-12-31 13:48                 ` Jamal Hadi Salim
  0 siblings, 1 reply; 10+ messages in thread
From: Hangbin Liu @ 2022-12-28  8:41 UTC (permalink / raw)
  To: Jamal Hadi Salim
  Cc: David Ahern, Jakub Kicinski, netdev, Cong Wang, Jiri Pirko,
	David S. Miller, Eric Dumazet, Paolo Abeni,
	Marcelo Ricardo Leitner, Vlad Buslov

On Tue, Dec 27, 2022 at 11:23:23AM -0500, Jamal Hadi Salim wrote:
> Hi Hangbin,
> 
> On Mon, Dec 26, 2022 at 10:58 PM Hangbin Liu <liuhangbin@gmail.com> wrote:
> >
> > Hi Jamal,
> > On Mon, Dec 26, 2022 at 11:31:24AM -0500, Jamal Hadi Salim wrote:
> > > My only concern is this is not generic enough i.e can other objects
> > > outside of filters do this?
> > > You are still doing it only for the filter (in tfilter_set_nl_ext() -
> > > sitting in cls_api)
> > > As i mentioned earlier, actions can also be offloaded independently;
> > > would this work with actions extack?
> > > If it wont work then perhaps we should go the avenue of using
> > > per-object(in this case filter) specific attributes
> > > to carry the extack as suggested by Jakub earlier.
> >
> > Yes, I think we can do it on action objects, e.g. call tfilter_set_nl_ext()
> > in tca_get_fill:
> >
> > tcf_add_notify() - tca_get_fill()
> >
> > I will rename tfilter_set_nl_ext() to tc_set_nl_ext(). BTW, should we also
> > do it in qdisc/ class?
> >
> > tclass_notify() - tc_fill_tclass()
> > qdisc_notify() - tc_fill_qdisc()
> >
> 
> The only useful cases imo are the ones that do h/w offload. So those two seem
> reasonable. Not sure where you place that tc_set_nl_ext() so it is
> visible for all.

How about put tc_set_nl_ext() to net/sched/sch_generic.c?

Hangbin

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCHv2 net-next] sched: multicast sched extack messages
  2022-12-28  8:41               ` Hangbin Liu
@ 2022-12-31 13:48                 ` Jamal Hadi Salim
  0 siblings, 0 replies; 10+ messages in thread
From: Jamal Hadi Salim @ 2022-12-31 13:48 UTC (permalink / raw)
  To: Hangbin Liu
  Cc: David Ahern, Jakub Kicinski, netdev, Cong Wang, Jiri Pirko,
	David S. Miller, Eric Dumazet, Paolo Abeni,
	Marcelo Ricardo Leitner, Vlad Buslov

Sounds good to me.

cheers,
jamal

On Wed, Dec 28, 2022 at 3:41 AM Hangbin Liu <liuhangbin@gmail.com> wrote:
>
> On Tue, Dec 27, 2022 at 11:23:23AM -0500, Jamal Hadi Salim wrote:
> > Hi Hangbin,
> >
> > On Mon, Dec 26, 2022 at 10:58 PM Hangbin Liu <liuhangbin@gmail.com> wrote:
> > >
> > > Hi Jamal,
> > > On Mon, Dec 26, 2022 at 11:31:24AM -0500, Jamal Hadi Salim wrote:
> > > > My only concern is this is not generic enough i.e can other objects
> > > > outside of filters do this?
> > > > You are still doing it only for the filter (in tfilter_set_nl_ext() -
> > > > sitting in cls_api)
> > > > As i mentioned earlier, actions can also be offloaded independently;
> > > > would this work with actions extack?
> > > > If it wont work then perhaps we should go the avenue of using
> > > > per-object(in this case filter) specific attributes
> > > > to carry the extack as suggested by Jakub earlier.
> > >
> > > Yes, I think we can do it on action objects, e.g. call tfilter_set_nl_ext()
> > > in tca_get_fill:
> > >
> > > tcf_add_notify() - tca_get_fill()
> > >
> > > I will rename tfilter_set_nl_ext() to tc_set_nl_ext(). BTW, should we also
> > > do it in qdisc/ class?
> > >
> > > tclass_notify() - tc_fill_tclass()
> > > qdisc_notify() - tc_fill_qdisc()
> > >
> >
> > The only useful cases imo are the ones that do h/w offload. So those two seem
> > reasonable. Not sure where you place that tc_set_nl_ext() so it is
> > visible for all.
>
> How about put tc_set_nl_ext() to net/sched/sch_generic.c?
>
> Hangbin

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2022-12-31 13:48 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-12-21  9:39 [PATCHv2 net-next] sched: multicast sched extack messages Hangbin Liu
2022-12-22  1:28 ` Jakub Kicinski
2022-12-22  7:48   ` Hangbin Liu
2022-12-22 16:26     ` David Ahern
2022-12-23  2:35       ` Hangbin Liu
2022-12-26 16:31         ` Jamal Hadi Salim
2022-12-27  3:58           ` Hangbin Liu
2022-12-27 16:23             ` Jamal Hadi Salim
2022-12-28  8:41               ` Hangbin Liu
2022-12-31 13:48                 ` Jamal Hadi Salim

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).