[patch net-next 0/2] net: sched: Fix RED qdisc offload flag

[patch net-next 0/2] net: sched: Fix RED qdisc offload flag

[Nogah Frankel]

Replacing the RED offload flag (TC_RED_OFFLOADED) with the generic one
(TCQ_F_OFFLOADED) deleted some of the logic behind it. This patchset fixes
this problem.

Nogah Frankel (2):
  net_sch: red: Fix the new offload indication
  net: sched: Move offload check till after dump call

 net/sched/sch_api.c |  5 ++---
 net/sched/sch_red.c | 26 ++++++++++++++------------
 2 files changed, 16 insertions(+), 15 deletions(-)

-- 
2.4.3

[patch net-next 1/2] net_sch: red: Fix the new offload indication

[Nogah Frankel]

Update the offload flag, TCQ_F_OFFLOADED, in each dump call (and ignore
the offloading function return value in relation to this flag).
This is done because a qdisc is being initialized, and therefore offloaded
before being grafted. Since the ability of the driver to offload the qdisc
depends on its location, a qdisc can be offloaded and un-offloaded by graft
calls, that doesn't effect the qdisc itself.

Fixes: 428a68af3a7c ("net: sched: Move to new offload indication in RED"
Signed-off-by: Nogah Frankel <nogahf@mellanox.com>
Reviewed-by: Yuval Mintz <yuvalm@mellanox.com>
---
 net/sched/sch_red.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/net/sched/sch_red.c b/net/sched/sch_red.c
index ec0bd36..a392eaa 100644
--- a/net/sched/sch_red.c
+++ b/net/sched/sch_red.c
@@ -157,7 +157,6 @@ static int red_offload(struct Qdisc *sch, bool enable)
 		.handle = sch->handle,
 		.parent = sch->parent,
 	};
-	int err;
 
 	if (!tc_can_offload(dev) || !dev->netdev_ops->ndo_setup_tc)
 		return -EOPNOTSUPP;
@@ -172,14 +171,7 @@ static int red_offload(struct Qdisc *sch, bool enable)
 		opt.command = TC_RED_DESTROY;
 	}
 
-	err = dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_QDISC_RED, &opt);
-
-	if (!err && enable)
-		sch->flags |= TCQ_F_OFFLOADED;
-	else
-		sch->flags &= ~TCQ_F_OFFLOADED;
-
-	return err;
+	return dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_QDISC_RED, &opt);
 }
 
 static void red_destroy(struct Qdisc *sch)
@@ -297,12 +289,22 @@ static int red_dump_offload_stats(struct Qdisc *sch, struct tc_red_qopt *opt)
 			.stats.qstats = &sch->qstats,
 		},
 	};
+	int err;
+
+	sch->flags &= ~TCQ_F_OFFLOADED;
 
-	if (!(sch->flags & TCQ_F_OFFLOADED))
+	if (!tc_can_offload(dev) || !dev->netdev_ops->ndo_setup_tc)
+		return 0;
+
+	err = dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_QDISC_RED,
+					    &hw_stats);
+	if (err == -EOPNOTSUPP)
 		return 0;
 
-	return dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_QDISC_RED,
-					     &hw_stats);
+	if (!err)
+		sch->flags |= TCQ_F_OFFLOADED;
+
+	return err;
 }
 
 static int red_dump(struct Qdisc *sch, struct sk_buff *skb)
-- 
2.4.3

Re: [patch net-next 1/2] net_sch: red: Fix the new offload indication

[Jiri Pirko]

Mon, Dec 25, 2017 at 09:51:41AM CET, nogahf@mellanox.com wrote:
>Update the offload flag, TCQ_F_OFFLOADED, in each dump call (and ignore
>the offloading function return value in relation to this flag).
>This is done because a qdisc is being initialized, and therefore offloaded
>before being grafted. Since the ability of the driver to offload the qdisc
>depends on its location, a qdisc can be offloaded and un-offloaded by graft
>calls, that doesn't effect the qdisc itself.
>
>Fixes: 428a68af3a7c ("net: sched: Move to new offload indication in RED"
>Signed-off-by: Nogah Frankel <nogahf@mellanox.com>
>Reviewed-by: Yuval Mintz <yuvalm@mellanox.com>

Acked-by: Jiri Pirko <jiri@mellanox.com>

[patch net-next 2/2] net: sched: Move offload check till after dump call

[Nogah Frankel]

Move the check of the offload state to after the qdisc dump action was
called, so the qdisc could update it if it was changed.

Fixes: 7a4fa29106d9 ("net: sched: Add TCA_HW_OFFLOAD")
Signed-off-by: Nogah Frankel <nogahf@mellanox.com>
Reviewed-by: Yuval Mintz <yuvalm@mellanox.com>
---
 net/sched/sch_api.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index 3a3a1da..758f132 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -807,11 +807,10 @@ static int tc_fill_qdisc(struct sk_buff *skb, struct Qdisc *q, u32 clid,
 	tcm->tcm_info = refcount_read(&q->refcnt);
 	if (nla_put_string(skb, TCA_KIND, q->ops->id))
 		goto nla_put_failure;
-	if (nla_put_u8(skb, TCA_HW_OFFLOAD, !!(q->flags & TCQ_F_OFFLOADED)))
-		goto nla_put_failure;
 	if (q->ops->dump && q->ops->dump(q, skb) < 0)
 		goto nla_put_failure;
-
+	if (nla_put_u8(skb, TCA_HW_OFFLOAD, !!(q->flags & TCQ_F_OFFLOADED)))
+		goto nla_put_failure;
 	qlen = qdisc_qlen_sum(q);
 
 	stab = rtnl_dereference(q->stab);
-- 
2.4.3

Re: [patch net-next 2/2] net: sched: Move offload check till after dump call

[Jiri Pirko]

Mon, Dec 25, 2017 at 09:51:42AM CET, nogahf@mellanox.com wrote:
>Move the check of the offload state to after the qdisc dump action was
>called, so the qdisc could update it if it was changed.
>
>Fixes: 7a4fa29106d9 ("net: sched: Add TCA_HW_OFFLOAD")
>Signed-off-by: Nogah Frankel <nogahf@mellanox.com>
>Reviewed-by: Yuval Mintz <yuvalm@mellanox.com>

Acked-by: Jiri Pirko <jiri@mellanox.com>

Re: [patch net-next 0/2] net: sched: Fix RED qdisc offload flag

[David Miller]

From: Nogah Frankel <nogahf@mellanox.com>
Date: Mon, 25 Dec 2017 10:51:40 +0200

> Replacing the RED offload flag (TC_RED_OFFLOADED) with the generic one
> (TCQ_F_OFFLOADED) deleted some of the logic behind it. This patchset fixes
> this problem.

Series applied, thank you.