[PATCH net 0/2] When ungrafting from PRIO, replace child with FIFO

[PATCH net 0/2] When ungrafting from PRIO, replace child with FIFO

[Petr Machata]

When a child Qdisc is removed from one of the PRIO Qdisc's bands, it is
replaced unconditionally by a NOOP qdisc. As a result, any traffic hitting
that band gets dropped. That is incorrect--no Qdisc was explicitly added
when PRIO was created, and after removal, none should have to be added
either.

In patch #2, this problem is fixed for PRIO by first attempting to create a
default Qdisc and only falling back to noop when that fails. This pattern
of attempting to create an invisible FIFO, using NOOP only as a fallback,
is also seen in some other Qdiscs.

The only driver currently offloading PRIO (and thus presumably the only one
impacted by this) is mlxsw. Therefore patch #1 extends mlxsw to handle the
replacement by an invisible FIFO gracefully.

Petr Machata (2):
  mlxsw: spectrum_qdisc: Ignore grafting of invisible FIFO
  net: sch_prio: When ungrafting, replace with FIFO

 drivers/net/ethernet/mellanox/mlxsw/spectrum_qdisc.c |  7 +++++++
 net/sched/sch_prio.c                                 | 10 ++++++++--
 2 files changed, 15 insertions(+), 2 deletions(-)

-- 
2.20.1

[PATCH net 1/2] mlxsw: spectrum_qdisc: Ignore grafting of invisible FIFO

[Petr Machata]

The following patch will change PRIO to replace a removed Qdisc with an
invisible FIFO, instead of NOOP. mlxsw will see this replacement due to the
graft message that is generated. But because FIFO does not issue its own
REPLACE message, when the graft operation takes place, the Qdisc that mlxsw
tracks under the indicated band is still the old one. The child
handle (0:0) therefore does not match, and mlxsw rejects the graft
operation, which leads to an extack message:

    Warning: Offloading graft operation failed.

Fix by ignoring the invisible children in the PRIO graft handler. The
DESTROY message of the removed Qdisc is going to follow shortly and handle
the removal.

Fixes: 32dc5efc6cb4 ("mlxsw: spectrum: qdiscs: prio: Handle graft command")
Signed-off-by: Petr Machata <petrm@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlxsw/spectrum_qdisc.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_qdisc.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_qdisc.c
index 68cc6737d45c..46d43cfd04e9 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_qdisc.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_qdisc.c
@@ -651,6 +651,13 @@ mlxsw_sp_qdisc_prio_graft(struct mlxsw_sp_port *mlxsw_sp_port,
 	    mlxsw_sp_port->tclass_qdiscs[tclass_num].handle == p->child_handle)
 		return 0;
 
+	if (!p->child_handle) {
+		/* This is an invisible FIFO replacing the original Qdisc.
+		 * Ignore it--the original Qdisc's destroy will follow.
+		 */
+		return 0;
+	}
+
 	/* See if the grafted qdisc is already offloaded on any tclass. If so,
 	 * unoffload it.
 	 */
-- 
2.20.1

[PATCH net 2/2] net: sch_prio: When ungrafting, replace with FIFO

[Petr Machata]

When a child Qdisc is removed from one of the PRIO Qdisc's bands, it is
replaced unconditionally by a NOOP qdisc. As a result, any traffic hitting
that band gets dropped. That is incorrect--no Qdisc was explicitly added
when PRIO was created, and after removal, none should have to be added
either.

Fix PRIO by first attempting to create a default Qdisc and only falling
back to noop when that fails. This pattern of attempting to create an
invisible FIFO, using NOOP only as a fallback, is also seen in other
Qdiscs.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Petr Machata <petrm@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
 net/sched/sch_prio.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/net/sched/sch_prio.c b/net/sched/sch_prio.c
index 18b884cfdfe8..647941702f9f 100644
--- a/net/sched/sch_prio.c
+++ b/net/sched/sch_prio.c
@@ -292,8 +292,14 @@ static int prio_graft(struct Qdisc *sch, unsigned long arg, struct Qdisc *new,
 	struct tc_prio_qopt_offload graft_offload;
 	unsigned long band = arg - 1;
 
-	if (new == NULL)
-		new = &noop_qdisc;
+	if (!new) {
+		new = qdisc_create_dflt(sch->dev_queue, &pfifo_qdisc_ops,
+					TC_H_MAKE(sch->handle, arg), extack);
+		if (!new)
+			new = &noop_qdisc;
+		else
+			qdisc_hash_add(new, true);
+	}
 
 	*old = qdisc_replace(sch, new, &q->queues[band]);
 
-- 
2.20.1

Re: [PATCH net 0/2] When ungrafting from PRIO, replace child with FIFO

[David Miller]

From: Petr Machata <petrm@mellanox.com>
Date: Mon, 6 Jan 2020 18:01:53 +0000

> When a child Qdisc is removed from one of the PRIO Qdisc's bands, it is
> replaced unconditionally by a NOOP qdisc. As a result, any traffic hitting
> that band gets dropped. That is incorrect--no Qdisc was explicitly added
> when PRIO was created, and after removal, none should have to be added
> either.
> 
> In patch #2, this problem is fixed for PRIO by first attempting to create a
> default Qdisc and only falling back to noop when that fails. This pattern
> of attempting to create an invisible FIFO, using NOOP only as a fallback,
> is also seen in some other Qdiscs.
> 
> The only driver currently offloading PRIO (and thus presumably the only one
> impacted by this) is mlxsw. Therefore patch #1 extends mlxsw to handle the
> replacement by an invisible FIFO gracefully.

Series applied, and queued up for -stable, thanks!

Re: [PATCH net 0/2] When ungrafting from PRIO, replace child with FIFO

[David Miller]

From: Petr Machata <petrm@mellanox.com>
Date: Mon, 6 Jan 2020 18:09:14 +0000

> Dave, when / if you decide to take this, note that it applies cleanly to
> net-next, however the code ends up being wrong and does not build. I put
> a patch with resolution here:
> 
>     https://github.com/jpirko/linux_mlxsw/commit/733d628ce37c38abab486c13a35b83da4b3ed161.patch
> 
> It's meant to be squashed to the merge commit.

Thanks, this will help me a lot when I do my next merge.