Message ID | 20200305162540.4363-1-lesliemonis@gmail.com |
---|---|
State | Accepted |
Delegated to: | David Ahern |
Headers | show |
Series | [iproute2-next] tc: pie: change maximum integer value of tc_pie_xstats->prob | expand |
On 3/5/20 9:25 AM, Leslie Monis wrote: > Kernel commit 105e808c1da2 ("pie: remove pie_vars->accu_prob_overflows"), > changes the maximum value of tc_pie_xstats->prob from (2^64 - 1) to > (2^56 - 1). > > Signed-off-by: Mohit P. Tahiliani <tahiliani@nitk.edu.in> > Signed-off-by: Gautam Ramakrishnan <gautamramk@gmail.com> > Signed-off-by: Leslie Monis <lesliemonis@gmail.com> > --- > tc/q_pie.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > applied to iproute2-next. Thanks
On 3/8/20 7:49 PM, David Ahern wrote: > On 3/5/20 9:25 AM, Leslie Monis wrote: >> Kernel commit 105e808c1da2 ("pie: remove pie_vars->accu_prob_overflows"), >> changes the maximum value of tc_pie_xstats->prob from (2^64 - 1) to >> (2^56 - 1). >> >> Signed-off-by: Mohit P. Tahiliani <tahiliani@nitk.edu.in> >> Signed-off-by: Gautam Ramakrishnan <gautamramk@gmail.com> >> Signed-off-by: Leslie Monis <lesliemonis@gmail.com> >> --- >> tc/q_pie.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> > > applied to iproute2-next. Thanks > > This means that iproute2 is incompatible with old kernels. commit 105e808c1da2 ("pie: remove pie_vars->accu_prob_overflows") was wrong, it should not have changed user ABI. The rule is : iproute2 v-X should work with linux-<whatever-version> Since pie MAX_PROB was implicitly in the user ABI, it can not be changed, at least from user point of view.
On 3/9/20 10:48 AM, Eric Dumazet wrote: > > > On 3/8/20 7:49 PM, David Ahern wrote: >> On 3/5/20 9:25 AM, Leslie Monis wrote: >>> Kernel commit 105e808c1da2 ("pie: remove pie_vars->accu_prob_overflows"), >>> changes the maximum value of tc_pie_xstats->prob from (2^64 - 1) to >>> (2^56 - 1). >>> >>> Signed-off-by: Mohit P. Tahiliani <tahiliani@nitk.edu.in> >>> Signed-off-by: Gautam Ramakrishnan <gautamramk@gmail.com> >>> Signed-off-by: Leslie Monis <lesliemonis@gmail.com> >>> --- >>> tc/q_pie.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >> >> applied to iproute2-next. Thanks >> >> > > This means that iproute2 is incompatible with old kernels. > > commit 105e808c1da2 ("pie: remove pie_vars->accu_prob_overflows") was wrong, > it should not have changed user ABI. > > The rule is : iproute2 v-X should work with linux-<whatever-version> > > Since pie MAX_PROB was implicitly in the user ABI, it can not be changed, > at least from user point of view. > So this kernel patch might be needed : diff --git a/net/sched/sch_pie.c b/net/sched/sch_pie.c index f52442d39bf57a7cf7af2595638a277e9c1ecf60..c65077f0c0f39832ee97f4e89f25639306b19281 100644 --- a/net/sched/sch_pie.c +++ b/net/sched/sch_pie.c @@ -493,7 +493,7 @@ static int pie_dump_stats(struct Qdisc *sch, struct gnet_dump *d) { struct pie_sched_data *q = qdisc_priv(sch); struct tc_pie_xstats st = { - .prob = q->vars.prob, + .prob = q->vars.prob << BITS_PER_BYTE, .delay = ((u32)PSCHED_TICKS2NS(q->vars.qdelay)) / NSEC_PER_USEC, .packets_in = q->stats.packets_in,
On Mon, Mar 9, 2020 at 11:24 PM Eric Dumazet <eric.dumazet@gmail.com> wrote: > > On 3/9/20 10:48 AM, Eric Dumazet wrote: > > > > This means that iproute2 is incompatible with old kernels. > > > > commit 105e808c1da2 ("pie: remove pie_vars->accu_prob_overflows") was wrong, > > it should not have changed user ABI. > > > > The rule is : iproute2 v-X should work with linux-<whatever-version> > > I'm apologize. I wasn't aware of this rule. > > Since pie MAX_PROB was implicitly in the user ABI, it can not be changed, > > at least from user point of view. > > You're right. It shouldn't have affected user space. But I'm afraid the value of MAX_PROB in the kernel did change in v5.1. commit 3f7ae5f3dc52 ("net: sched: pie: add more cases to auto-tune alpha and beta") introduced that change. I'm not sure what to do about this. How can I fix it? > > So this kernel patch might be needed : > > diff --git a/net/sched/sch_pie.c b/net/sched/sch_pie.c > index f52442d39bf57a7cf7af2595638a277e9c1ecf60..c65077f0c0f39832ee97f4e89f25639306b19281 100644 > --- a/net/sched/sch_pie.c > +++ b/net/sched/sch_pie.c > @@ -493,7 +493,7 @@ static int pie_dump_stats(struct Qdisc *sch, struct gnet_dump *d) > { > struct pie_sched_data *q = qdisc_priv(sch); > struct tc_pie_xstats st = { > - .prob = q->vars.prob, > + .prob = q->vars.prob << BITS_PER_BYTE, > .delay = ((u32)PSCHED_TICKS2NS(q->vars.qdelay)) / > NSEC_PER_USEC, > .packets_in = q->stats.packets_in, Thanks. This is a much better solution. Should I go ahead and submit this to net-next? I guess the applied patch (topic of this thread) has to be reverted.
On 3/9/20 11:42 AM, Leslie Monis wrote: > On Mon, Mar 9, 2020 at 11:24 PM Eric Dumazet <eric.dumazet@gmail.com> wrote: >> >> On 3/9/20 10:48 AM, Eric Dumazet wrote: >>> >>> This means that iproute2 is incompatible with old kernels. >>> >>> commit 105e808c1da2 ("pie: remove pie_vars->accu_prob_overflows") was wrong, >>> it should not have changed user ABI. >>> >>> The rule is : iproute2 v-X should work with linux-<whatever-version> >>> > > I'm apologize. I wasn't aware of this rule. > >>> Since pie MAX_PROB was implicitly in the user ABI, it can not be changed, >>> at least from user point of view. >>> > > You're right. It shouldn't have affected user space. > But I'm afraid the value of MAX_PROB in the kernel did change in v5.1. > commit 3f7ae5f3dc52 ("net: sched: pie: add more cases to auto-tune > alpha and beta") > introduced that change. I'm not sure what to do about this. How can I fix it? > >> >> So this kernel patch might be needed : >> >> diff --git a/net/sched/sch_pie.c b/net/sched/sch_pie.c >> index f52442d39bf57a7cf7af2595638a277e9c1ecf60..c65077f0c0f39832ee97f4e89f25639306b19281 100644 >> --- a/net/sched/sch_pie.c >> +++ b/net/sched/sch_pie.c >> @@ -493,7 +493,7 @@ static int pie_dump_stats(struct Qdisc *sch, struct gnet_dump *d) >> { >> struct pie_sched_data *q = qdisc_priv(sch); >> struct tc_pie_xstats st = { >> - .prob = q->vars.prob, >> + .prob = q->vars.prob << BITS_PER_BYTE, >> .delay = ((u32)PSCHED_TICKS2NS(q->vars.qdelay)) / >> NSEC_PER_USEC, >> .packets_in = q->stats.packets_in, > > Thanks. This is a much better solution. > Should I go ahead and submit this to net-next? Sure, please go ahead ! > I guess the applied patch (topic of this thread) has to be reverted. >
diff --git a/tc/q_pie.c b/tc/q_pie.c index 709a78b4..e6939652 100644 --- a/tc/q_pie.c +++ b/tc/q_pie.c @@ -223,9 +223,9 @@ static int pie_print_xstats(struct qdisc_util *qu, FILE *f, st = RTA_DATA(xstats); - /* prob is returned as a fracion of maximum integer value */ + /* prob is returned as a fracion of (2^56 - 1) */ print_float(PRINT_ANY, "prob", " prob %lg", - (double)st->prob / (double)UINT64_MAX); + (double)st->prob / (double)(UINT64_MAX >> 8)); print_uint(PRINT_JSON, "delay", NULL, st->delay); print_string(PRINT_FP, NULL, " delay %s", sprint_time(st->delay, b1));