| Submitter | Eric Dumazet |
|---|---|
| Date | Nov. 30, 2011, 9:34 p.m. |
| Message ID | <1322688891.2602.15.camel@edumazet-laptop> |
| Download | mbox | patch |
| Permalink | /patch/128591/ |
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
Comments
Le mercredi 30 novembre 2011 à 22:34 +0100, Eric Dumazet a écrit : > Since commit a4a710c4a7490587 (pkt_sched: Change PSCHED_SHIFT from 10 to > 6) it seems [G]RED are broken in red_calc_qavg_from_idle_time() > > This function computes a delay in us units, but this delay is now 16 > times bigger than real delay, so the final qavg result is wrong... > > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> > --- > Maybe we should use native kernel time services in red ? > (ktime_get(), ktime_us_delta()) > > include/net/pkt_sched.h | 1 + > include/net/red.h | 2 +- > 2 files changed, 2 insertions(+), 1 deletion(-) > > diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h > index fffdc60..c719d9b 100644 > --- a/include/net/pkt_sched.h > +++ b/include/net/pkt_sched.h > @@ -43,6 +43,7 @@ typedef long psched_tdiff_t; > /* Avoid doing 64 bit divide */ > #define PSCHED_SHIFT 6 > #define PSCHED_TICKS2NS(x) ((s64)(x) << PSCHED_SHIFT) > +#define PSCHED_TICKS2US(x) (PSCHED_TICKS2NS(x) >> 10) /* approximate 1000 by 1024 */ > #define PSCHED_NS2TICKS(x) ((x) >> PSCHED_SHIFT) > > #define PSCHED_TICKS_PER_SEC PSCHED_NS2TICKS(NSEC_PER_SEC) > diff --git a/include/net/red.h b/include/net/red.h > index 3319f16..a413638 100644 > --- a/include/net/red.h > +++ b/include/net/red.h > @@ -175,7 +175,7 @@ static inline unsigned long red_calc_qavg_from_idle_time(struct red_parms *p) > int shift; > > now = psched_get_time(); > - us_idle = psched_tdiff_bounded(now, p->qidlestart, p->Scell_max); > + us_idle = PSCHED_TICKS2US(psched_tdiff_bounded(now, p->qidlestart, p->Scell_max)); Hmm, this is not correct the bound should be applied on the us result, not on tick I presume (one tick == 64ns) I'll test another patch. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Patch
diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h index fffdc60..c719d9b 100644 --- a/include/net/pkt_sched.h +++ b/include/net/pkt_sched.h @@ -43,6 +43,7 @@ typedef long psched_tdiff_t; /* Avoid doing 64 bit divide */ #define PSCHED_SHIFT 6 #define PSCHED_TICKS2NS(x) ((s64)(x) << PSCHED_SHIFT) +#define PSCHED_TICKS2US(x) (PSCHED_TICKS2NS(x) >> 10) /* approximate 1000 by 1024 */ #define PSCHED_NS2TICKS(x) ((x) >> PSCHED_SHIFT) #define PSCHED_TICKS_PER_SEC PSCHED_NS2TICKS(NSEC_PER_SEC) diff --git a/include/net/red.h b/include/net/red.h index 3319f16..a413638 100644 --- a/include/net/red.h +++ b/include/net/red.h @@ -175,7 +175,7 @@ static inline unsigned long red_calc_qavg_from_idle_time(struct red_parms *p) int shift; now = psched_get_time(); - us_idle = psched_tdiff_bounded(now, p->qidlestart, p->Scell_max); + us_idle = PSCHED_TICKS2US(psched_tdiff_bounded(now, p->qidlestart, p->Scell_max)); /* * The problem: ideally, average length queue recalcultion should
Since commit a4a710c4a7490587 (pkt_sched: Change PSCHED_SHIFT from 10 to 6) it seems [G]RED are broken in red_calc_qavg_from_idle_time() This function computes a delay in us units, but this delay is now 16 times bigger than real delay, so the final qavg result is wrong... Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> --- Maybe we should use native kernel time services in red ? (ktime_get(), ktime_us_delta()) include/net/pkt_sched.h | 1 + include/net/red.h | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html