diff mbox series

[net-next,09/10] net: sch: prio: Add offload ability for grafting a child

Message ID 20180228094507.22354-10-jiri@resnulli.us
State Accepted, archived
Delegated to: David Miller
Headers show
Series mlxsw: Offload multi-queue RED support | expand

Commit Message

Jiri Pirko Feb. 28, 2018, 9:45 a.m. UTC 
From: Nogah Frankel <nogahf@mellanox.com>

Offload sch_prio graft command for capable drivers.
Warn in case of a failure, unless the graft was done as part of a destroy
operation (the new qdisc is a noop) or if all the qdiscs (the parent, the
old child, and the new one) are not offloaded.

Signed-off-by: Nogah Frankel <nogahf@mellanox.com>
Reviewed-by: Yuval Mintz <yuvalm@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 include/net/pkt_cls.h |  8 ++++++++
 net/sched/sch_prio.c  | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+)

Comments

Alexander Aring March 2, 2018, 3:38 a.m. UTC | #1 
Hi,

On Wed, Feb 28, 2018 at 4:45 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> From: Nogah Frankel <nogahf@mellanox.com>
>
> Offload sch_prio graft command for capable drivers.
> Warn in case of a failure, unless the graft was done as part of a destroy
> operation (the new qdisc is a noop) or if all the qdiscs (the parent, the
> old child, and the new one) are not offloaded.
>
> Signed-off-by: Nogah Frankel <nogahf@mellanox.com>
> Reviewed-by: Yuval Mintz <yuvalm@mellanox.com>
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> ---
>  include/net/pkt_cls.h |  8 ++++++++
>  net/sched/sch_prio.c  | 32 ++++++++++++++++++++++++++++++++
...
> +               if (*old)
> +                       any_qdisc_is_offloaded |= (*old)->flags &
> +                                                  TCQ_F_OFFLOADED;
> +
> +               if (any_qdisc_is_offloaded)
> +                       NL_SET_ERR_MSG(extack, "Offloading graft operation failed.");
> +       }
> +
>         return 0;
>  }
>

I guess to make extack working, you need to return an errno if failed.

- Alex
Jakub Kicinski March 2, 2018, 3:48 a.m. UTC | #2 
On Thu, 1 Mar 2018 22:38:50 -0500, Alexander Aring wrote:
> I guess to make extack working, you need to return an errno if failed.

AFAIK extack is printed as a warning if operation did not fail.
Jiri Pirko March 2, 2018, 8:37 a.m. UTC | #3 
Fri, Mar 02, 2018 at 04:48:40AM CET, kubakici@wp.pl wrote:
>On Thu, 1 Mar 2018 22:38:50 -0500, Alexander Aring wrote:
>> I guess to make extack working, you need to return an errno if failed.
>
>AFAIK extack is printed as a warning if operation did not fail.

Yes, I checked this and it is printed as warning.
Alexander Aring March 2, 2018, 2:21 p.m. UTC | #4 
Hi,

On Fri, Mar 2, 2018 at 3:37 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> Fri, Mar 02, 2018 at 04:48:40AM CET, kubakici@wp.pl wrote:
>>On Thu, 1 Mar 2018 22:38:50 -0500, Alexander Aring wrote:
>>> I guess to make extack working, you need to return an errno if failed.
>>
>>AFAIK extack is printed as a warning if operation did not fail.
>
> Yes, I checked this and it is printed as warning.

ouch, so far I know extack it allows only one messages delivered back
to the user space.

If we introduce a warning in the successful path here, it could be
that in the callpath (after "successful" part of this callback), that
somebody else want to add a warning and overwrites actually your
warning (even, he is not aware that this warning was set - okay I
suppose you can do another check on NULL and set a warning, that
somebody overwrites a warning :-D).

If extack messages get's append and is some kind of for_each_nested
string in netlink -> we have no problem, but I guess this not how it's
working. :-/

- Alex
Marcelo Ricardo Leitner March 2, 2018, 2:45 p.m. UTC | #5 
On Fri, Mar 02, 2018 at 09:21:56AM -0500, Alexander Aring wrote:
> Hi,
> 
> On Fri, Mar 2, 2018 at 3:37 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> > Fri, Mar 02, 2018 at 04:48:40AM CET, kubakici@wp.pl wrote:
> >>On Thu, 1 Mar 2018 22:38:50 -0500, Alexander Aring wrote:
> >>> I guess to make extack working, you need to return an errno if failed.
> >>
> >>AFAIK extack is printed as a warning if operation did not fail.
> >
> > Yes, I checked this and it is printed as warning.
> 
> ouch, so far I know extack it allows only one messages delivered back
> to the user space.
> 
> If we introduce a warning in the successful path here, it could be
> that in the callpath (after "successful" part of this callback), that
> somebody else want to add a warning and overwrites actually your
> warning (even, he is not aware that this warning was set - okay I
> suppose you can do another check on NULL and set a warning, that
> somebody overwrites a warning :-D).
> 
> If extack messages get's append and is some kind of for_each_nested
> string in netlink -> we have no problem, but I guess this not how it's
> working. :-/

IOW I guess what we are looking for here is a way to use extack to
track more than an error/warning message, but to be a bit more
complete error reporting tool.

The case here is pretty much like the case with tc flower offloading,
to issue a message when it couldn't offload while it wasn't fatal for
the rule (in case SKIP_SW wasn't specified). The reason for why the
offloading couldn't happen could have been a temporary one and such
log of a warning is important for troubleshooting.

  Marcelo
diff mbox series

Patch

diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 87406252f0a3..e828d31be5da 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -806,6 +806,7 @@  enum tc_prio_command {
 	TC_PRIO_REPLACE,
 	TC_PRIO_DESTROY,
 	TC_PRIO_STATS,
+	TC_PRIO_GRAFT,
 };
 
 struct tc_prio_qopt_offload_params {
@@ -818,6 +819,11 @@  struct tc_prio_qopt_offload_params {
 	struct gnet_stats_queue *qstats;
 };
 
+struct tc_prio_qopt_offload_graft_params {
+	u8 band;
+	u32 child_handle;
+};
+
 struct tc_prio_qopt_offload {
 	enum tc_prio_command command;
 	u32 handle;
@@ -825,6 +831,8 @@  struct tc_prio_qopt_offload {
 	union {
 		struct tc_prio_qopt_offload_params replace_params;
 		struct tc_qopt_offload_stats stats;
+		struct tc_prio_qopt_offload_graft_params graft_params;
 	};
 };
+
 #endif
diff --git a/net/sched/sch_prio.c b/net/sched/sch_prio.c
index ba2d6d17d95a..222e53d3d27a 100644
--- a/net/sched/sch_prio.c
+++ b/net/sched/sch_prio.c
@@ -308,12 +308,44 @@  static int prio_graft(struct Qdisc *sch, unsigned long arg, struct Qdisc *new,
 		      struct Qdisc **old, struct netlink_ext_ack *extack)
 {
 	struct prio_sched_data *q = qdisc_priv(sch);
+	struct tc_prio_qopt_offload graft_offload;
+	struct net_device *dev = qdisc_dev(sch);
 	unsigned long band = arg - 1;
+	bool any_qdisc_is_offloaded;
+	int err;
 
 	if (new == NULL)
 		new = &noop_qdisc;
 
 	*old = qdisc_replace(sch, new, &q->queues[band]);
+
+	if (!tc_can_offload(dev))
+		return 0;
+
+	graft_offload.handle = sch->handle;
+	graft_offload.parent = sch->parent;
+	graft_offload.graft_params.band = band;
+	graft_offload.graft_params.child_handle = new->handle;
+	graft_offload.command = TC_PRIO_GRAFT;
+
+	err = dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_QDISC_PRIO,
+					    &graft_offload);
+
+	/* Don't report error if the graft is part of destroy operation. */
+	if (err && new != &noop_qdisc) {
+		/* Don't report error if the parent, the old child and the new
+		 * one are not offloaded.
+		 */
+		any_qdisc_is_offloaded = sch->flags & TCQ_F_OFFLOADED;
+		any_qdisc_is_offloaded |= new->flags & TCQ_F_OFFLOADED;
+		if (*old)
+			any_qdisc_is_offloaded |= (*old)->flags &
+						   TCQ_F_OFFLOADED;
+
+		if (any_qdisc_is_offloaded)
+			NL_SET_ERR_MSG(extack, "Offloading graft operation failed.");
+	}
+
 	return 0;
 }