On Mon, May 18, 2009 at 06:40:56PM +0200, Eric Dumazet wrote:
quoted text
> Jarek Poplawski a écrit :
> > On Fri, May 15, 2009 at 03:49:31PM +0100, Antonio Almeida wrote:
> > ...
> >> I also note that, for HTB rate configurations over 500Mbit/s on leaf
> >> class, when I stop the traffic, in the output of "tc -s -d class ls
> >> dev eth1" command, I see that leaf's rate (in bits/s) is growing
> >> instead of decreasing (as expected since I've stopped the traffic).
> >> Rate in pps is ok and decreases until 0pps. Rate in bits/s increases
> >> above 1000Mbit and stays there for a few minutes. After two or three
> >> minutes it becomes 0bit. The same happens for it's ancestors (also for
> >> root class).Here's tc output of my leaf class for this situation:
> >>
> >> class htb 1:108 parent 1:10 leaf 108: prio 7 quantum 1514 rate
> >> 555000Kbit ceil 555000Kbit burst 70901b/8 mpu 0b overhead 0b cburst
> >> 70901b/8 mpu 0b overhead 0b level 0
> >>  Sent 120267768144 bytes 242475339 pkt (dropped 62272599, overlimits 0
> >> requeues 0)
> >>  rate 1074Mbit 0pps backlog 0b 0p requeues 0
> >>  lended: 242475339 borrowed: 0 giants: 0
> >>  tokens: 8 ctokens: 8
> > 
> > This looks like a regular bug. I guess it's an overflow in
> > gen_estimator(), but I'm not sure there is nothing more. Could you
> > try the patch below? (An offset warning when patching 2.6.25 is OK)
> > 
> > Thanks,
> > Jarek P.
> > ---
> > 
> >  net/core/gen_estimator.c |    6 +++++-
> >  1 files changed, 5 insertions(+), 1 deletions(-)
> > 
> > diff --git a/net/core/gen_estimator.c b/net/core/gen_estimator.c
> > index 9cc9f95..87f0ced 100644
> > --- a/net/core/gen_estimator.c
> > +++ b/net/core/gen_estimator.c
> > @@ -127,7 +127,11 @@ static void est_timer(unsigned long arg)
> >  		npackets = e->bstats->packets;
> >  		rate = (nbytes - e->last_bytes)<<(7 - idx);
> >  		e->last_bytes = nbytes;
> > -		e->avbps += ((long)rate - (long)e->avbps) >> e->ewma_log;
> > +		if (rate > e->avbps)
> > +			e->avbps += (rate - e->avbps) >> e->ewma_log;
> > +		else
> > +			e->avbps -= (e->avbps - rate) >> e->ewma_log;
> > +
> >  		e->rate_est->bps = (e->avbps+0xF)>>5;
> >  
> >  		rate = (npackets - e->last_packets)<<(12 - idx);
> 
> With a typical estimator "1sec 8sec", ewma_log value is 3
> 
> At gigabit speeds, we are very close to overflow yes, since
> we only have 27 bits available, so 134217728 bytes per second
> or 1073741824 bits per second.
> 
> So formula :
> e->avbps += ((long)rate - (long)e->avbps) >> e->ewma_log;
> is going to overflow.
> 
> One way to avoid the overflow would be to use a smaller estimator, like "500ms 4sec" 
> 
> Or use a 64bits rate & avbps, this is needed fo 10Gb speeds I suppose...

Yes, I considered this too, but because of an overhead I decided to
fix as designed (according to the comment) for now. But probably you
are right, and we should go further, so I'm OK with your patch.

Jarek P.

quoted text
> 
> diff --git a/net/core/gen_estimator.c b/net/core/gen_estimator.c
> index 9cc9f95..150e2f5 100644
> --- a/net/core/gen_estimator.c
> +++ b/net/core/gen_estimator.c
> @@ -86,9 +86,9 @@ struct gen_estimator
>  	spinlock_t		*stats_lock;
>  	int			ewma_log;
>  	u64			last_bytes;
> +	u64			avbps;
>  	u32			last_packets;
>  	u32			avpps;
> -	u32			avbps;
>  	struct rcu_head		e_rcu;
>  	struct rb_node		node;
>  };
> @@ -115,6 +115,7 @@ static void est_timer(unsigned long arg)
>  	rcu_read_lock();
>  	list_for_each_entry_rcu(e, &elist[idx].list, list) {
>  		u64 nbytes;
> +		u64 brate;
>  		u32 npackets;
>  		u32 rate;
>  
> @@ -125,9 +126,9 @@ static void est_timer(unsigned long arg)
>  
>  		nbytes = e->bstats->bytes;
>  		npackets = e->bstats->packets;
> -		rate = (nbytes - e->last_bytes)<<(7 - idx);
> +		brate = (nbytes - e->last_bytes)<<(7 - idx);
>  		e->last_bytes = nbytes;
> -		e->avbps += ((long)rate - (long)e->avbps) >> e->ewma_log;
> +		e->avbps += ((s64)(brate - e->avbps)) >> e->ewma_log;
>  		e->rate_est->bps = (e->avbps+0xF)>>5;
>  
>  		rate = (npackets - e->last_packets)<<(12 - idx);
> 
--
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