From 59ab87f6eb920fd0ee3058ae4c0956f52036b893 Mon Sep 17 00:00:00 2001 From: Andre Guedes Date: Tue, 23 Apr 2019 12:44:20 -0700 Subject: [PATCH 1/5] net: sched: taprio: Remove pointless variable assigment This patch removes a pointless variable assigment in taprio_change(). The 'err' variable is not used from this assignment to the next one so this patch removes it. Signed-off-by: Andre Guedes Signed-off-by: David S. Miller --- net/sched/sch_taprio.c | 1 - 1 file changed, 1 deletion(-) diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c index 001182aa3959..d91a7ec67348 100644 --- a/net/sched/sch_taprio.c +++ b/net/sched/sch_taprio.c @@ -651,7 +651,6 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt, if (err < 0) return err; - err = -EINVAL; if (tb[TCA_TAPRIO_ATTR_PRIOMAP]) mqprio = nla_data(tb[TCA_TAPRIO_ATTR_PRIOMAP]); From 8599099f0c58cec677a47c968e777eee8d64fb80 Mon Sep 17 00:00:00 2001 From: Andre Guedes Date: Tue, 23 Apr 2019 12:44:21 -0700 Subject: [PATCH 2/5] net: sched: taprio: Refactor taprio_get_start_time() This patch does a code refactoring to taprio_get_start_time() function to improve readability and report error properly. If 'base' time is later than 'now', the start time is equal to 'base' and taprio_get_start_time() is done. That's the natural case so we move that code to the beginning of the function. Also, if 'cycle' calculation is zero, something went really wrong with taprio and we should log that internal error properly. Signed-off-by: Andre Guedes Signed-off-by: David S. Miller --- net/sched/sch_taprio.c | 37 +++++++++++++++++++++++-------------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c index d91a7ec67348..d0aae7b5e608 100644 --- a/net/sched/sch_taprio.c +++ b/net/sched/sch_taprio.c @@ -539,7 +539,7 @@ static int taprio_parse_mqprio_opt(struct net_device *dev, return 0; } -static ktime_t taprio_get_start_time(struct Qdisc *sch) +static int taprio_get_start_time(struct Qdisc *sch, ktime_t *start) { struct taprio_sched *q = qdisc_priv(sch); struct sched_entry *entry; @@ -547,27 +547,33 @@ static ktime_t taprio_get_start_time(struct Qdisc *sch) s64 n; base = ns_to_ktime(q->base_time); - cycle = 0; + now = q->get_time(); + + if (ktime_after(base, now)) { + *start = base; + return 0; + } /* Calculate the cycle_time, by summing all the intervals. */ + cycle = 0; list_for_each_entry(entry, &q->entries, list) cycle = ktime_add_ns(cycle, entry->interval); - if (!cycle) - return base; - - now = q->get_time(); - - if (ktime_after(base, now)) - return base; + /* The qdisc is expected to have at least one sched_entry. Moreover, + * any entry must have 'interval' > 0. Thus if the cycle time is zero, + * something went really wrong. In that case, we should warn about this + * inconsistent state and return error. + */ + if (WARN_ON(!cycle)) + return -EFAULT; /* Schedule the start time for the beginning of the next * cycle. */ n = div64_s64(ktime_sub_ns(now, base), cycle); - - return ktime_add_ns(base, (n + 1) * cycle); + *start = ktime_add_ns(base, (n + 1) * cycle); + return 0; } static void taprio_start_sched(struct Qdisc *sch, ktime_t start) @@ -716,9 +722,12 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt, } taprio_set_picos_per_byte(dev, q); - start = taprio_get_start_time(sch); - if (!start) - return 0; + + err = taprio_get_start_time(sch, &start); + if (err < 0) { + NL_SET_ERR_MSG(extack, "Internal error: failed get start time"); + return err; + } taprio_start_sched(sch, start); From 5175aafe71bfb3fc6a1254a966b0f60e7a46ebba Mon Sep 17 00:00:00 2001 From: Andre Guedes Date: Tue, 23 Apr 2019 12:44:22 -0700 Subject: [PATCH 3/5] net: sched: taprio: Remove should_restart_cycle() The 'entry' argument from should_restart_cycle() cannot be NULL since it is already checked by the caller so the WARN_ON() within should_ restart_cycle() could be removed. By doing that, that function becomes a dummy wrapper on list_is_last() so this patch simply gets rid of it and call list_is_last() within advance_sched() instead. Signed-off-by: Andre Guedes Signed-off-by: David S. Miller --- net/sched/sch_taprio.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c index d0aae7b5e608..77cca993710a 100644 --- a/net/sched/sch_taprio.c +++ b/net/sched/sch_taprio.c @@ -209,14 +209,6 @@ static struct sk_buff *taprio_dequeue(struct Qdisc *sch) return NULL; } -static bool should_restart_cycle(const struct taprio_sched *q, - const struct sched_entry *entry) -{ - WARN_ON(!entry); - - return list_is_last(&entry->list, &q->entries); -} - static enum hrtimer_restart advance_sched(struct hrtimer *timer) { struct taprio_sched *q = container_of(timer, struct taprio_sched, @@ -240,7 +232,7 @@ static enum hrtimer_restart advance_sched(struct hrtimer *timer) goto first_run; } - if (should_restart_cycle(q, entry)) + if (list_is_last(&entry->list, &q->entries)) next = list_first_entry(&q->entries, struct sched_entry, list); else From 2684d1b75f215bdf521064bcbc0015dfca9156e7 Mon Sep 17 00:00:00 2001 From: Andre Guedes Date: Tue, 23 Apr 2019 12:44:23 -0700 Subject: [PATCH 4/5] net: sched: taprio: Fix taprio_peek() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit While traversing taprio's children qdisc list, if the gate is closed for a given traffic class, we should continue traversing the list since the remaining qdiscs may have skb ready for transmission. This patch also takes this opportunity and changes the function to use the TAPRIO_ALL_GATES_OPEN macro instead of the magic number '-1'. Fixes: 5a781ccbd19e (“tc: Add support for configuring the taprio scheduler”) Signed-off-by: Andre Guedes Signed-off-by: David S. Miller --- net/sched/sch_taprio.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c index 77cca993710a..0df924f87f3e 100644 --- a/net/sched/sch_taprio.c +++ b/net/sched/sch_taprio.c @@ -90,7 +90,7 @@ static struct sk_buff *taprio_peek(struct Qdisc *sch) rcu_read_lock(); entry = rcu_dereference(q->current_entry); - gate_mask = entry ? entry->gate_mask : -1; + gate_mask = entry ? entry->gate_mask : TAPRIO_ALL_GATES_OPEN; rcu_read_unlock(); if (!gate_mask) @@ -112,7 +112,7 @@ static struct sk_buff *taprio_peek(struct Qdisc *sch) tc = netdev_get_prio_tc_map(dev, prio); if (!(gate_mask & BIT(tc))) - return NULL; + continue; return skb; } From 6e734c82be63ef6a0032c29f6d406d60e4386050 Mon Sep 17 00:00:00 2001 From: Andre Guedes Date: Tue, 23 Apr 2019 12:44:24 -0700 Subject: [PATCH 5/5] net: sched: taprio: Fix taprio_dequeue() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In case we don't have 'guard' or 'budget' to transmit the skb, we should continue traversing the qdisc list since the remaining guard/budget might be enough to transmit a skb from other children qdiscs. Fixes: 5a781ccbd19e (“tc: Add support for configuring the taprio scheduler”) Signed-off-by: Andre Guedes Signed-off-by: David S. Miller --- net/sched/sch_taprio.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c index 0df924f87f3e..df848a36b222 100644 --- a/net/sched/sch_taprio.c +++ b/net/sched/sch_taprio.c @@ -188,12 +188,12 @@ static struct sk_buff *taprio_dequeue(struct Qdisc *sch) */ if (gate_mask != TAPRIO_ALL_GATES_OPEN && ktime_after(guard, entry->close_time)) - return NULL; + continue; /* ... and no budget. */ if (gate_mask != TAPRIO_ALL_GATES_OPEN && atomic_sub_return(len, &entry->budget) < 0) - return NULL; + continue; skb = child->ops->dequeue(child); if (unlikely(!skb))