Re: [PATCH net-next v4] sched: Add dualpi2 qdisc

["Tilmans, Olivier (Nokia - BE/Antwerp)" <olivier.tilmans@nokia-bell-labs.com>]

> +static s64 __scale_delta(s64 diff)
> +{
> +	do_div(diff, (1 << (ALPHA_BETA_GRANULARITY + 1)) - 1);
> +	return diff;
> +}
[...]
> +	delta = __scale_delta(((s64)qdelay - q->pi2.target) * q->pi2.alpha);
> +	delta += __scale_delta(((s64)qdelay - qdelay_old) * q->pi2.beta);

I just noticed that ensuring 64b divide compatibility across platforms 
using do_div() introduced this bug, as do_div() works with unsigned operands.

This will be fixed in a later v5 with the following patch:

---
 net/sched/sch_dualpi2.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/net/sched/sch_dualpi2.c b/net/sched/sch_dualpi2.c
index a6452aa82018..c6c851499d35 100644
--- a/net/sched/sch_dualpi2.c
+++ b/net/sched/sch_dualpi2.c
@@ -385,7 +385,7 @@ static struct sk_buff *dualpi2_qdisc_dequeue(struct Qdisc *sch)
 	return skb;
 }
 
-static s64 __scale_delta(s64 diff)
+static s64 __scale_delta(u64 diff)
 {
 	do_div(diff, (1 << (ALPHA_BETA_GRANULARITY + 1)) - 1);
 	return diff;
@@ -406,16 +406,18 @@ static u32 calculate_probability(struct Qdisc *sch)
 	/* Alpha and beta take at most 32b, i.e, the delay difference would
 	 * overflow for queueing delay differences > ~4.2sec.
 	 */
-	delta = __scale_delta(((s64)qdelay - q->pi2.target) * q->pi2.alpha);
-	delta += __scale_delta(((s64)qdelay - qdelay_old) * q->pi2.beta);
-	new_prob = delta + q->pi2.prob;
+	delta = ((s64)qdelay - q->pi2.target) * q->pi2.alpha;
+	delta += ((s64)qdelay - qdelay_old) * q->pi2.beta;
 	/* Prevent overflow */
 	if (delta > 0) {
+		new_prob = __scale_delta(delta) + q->pi2.prob;
 		if (new_prob < q->pi2.prob)
 			new_prob = MAX_PROB;
+	} else {
+		new_prob = q->pi2.prob - __scale_delta(delta * -1);
 		/* Prevent underflow */
-	} else if (new_prob > q->pi2.prob) {
-		new_prob = 0;
+		if (new_prob > q->pi2.prob)
+			new_prob = 0;
 	}
 	/* If we do not drop on overload, ensure we cap the L4S probability to
 	 * 100% to keep window fairness when overflowing.
-- 

Sorry for this.


Best,
Olivier