netem: fix 64 bit divide

Message ID 20171114172859.27930-1-sthemmin@microsoft.com
State Superseded
Delegated to: David Miller
Headers show
Series
  • netem: fix 64 bit divide
Related show

Commit Message

Stephen Hemminger Nov. 14, 2017, 5:28 p.m.
Since times are now expressed in 64 bit nanosecond, need to now do
true 64 bit divide. Change the name of the function to bette express
the new units.

Fixes: 99803171ef04 ("netem: add uapi to express delay and jitter in nanoseconds")
Reported-by: Randy Dunlap <rdunlap@infradead.org>
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 net/sched/sch_netem.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

Comments

Randy Dunlap Nov. 14, 2017, 5:57 p.m. | #1
On 11/14/2017 09:28 AM, Stephen Hemminger wrote:
> Since times are now expressed in 64 bit nanosecond, need to now do
> true 64 bit divide. Change the name of the function to bette express
> the new units.
> 
> Fixes: 99803171ef04 ("netem: add uapi to express delay and jitter in nanoseconds")
> Reported-by: Randy Dunlap <rdunlap@infradead.org>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

Hi Stephen,

I still get it. Maybe it's this % operator:

		skb->data[prandom_u32() % skb_headlen(skb)] ^=
			1<<(prandom_u32() % 8);

in net_enqueue().

> ---
>  net/sched/sch_netem.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
> index b686e755fda9..644323d6081c 100644
> --- a/net/sched/sch_netem.c
> +++ b/net/sched/sch_netem.c
> @@ -339,10 +339,8 @@ static s64 tabledist(s64 mu, s64 sigma,
>  	return  x / NETEM_DIST_SCALE + (sigma / NETEM_DIST_SCALE) * t + mu;
>  }
>  
> -static u64 packet_len_2_sched_time(unsigned int len,
> -				   struct netem_sched_data *q)
> +static u64 packet_time_ns(u64 len, const struct netem_sched_data *q)
>  {
> -	u64 offset;
>  	len += q->packet_overhead;
>  
>  	if (q->cell_size) {
> @@ -352,9 +350,8 @@ static u64 packet_len_2_sched_time(unsigned int len,
>  			cells++;
>  		len = cells * (q->cell_size + q->cell_overhead);
>  	}
> -	offset = (u64)len * NSEC_PER_SEC;
> -	do_div(offset, q->rate);
> -	return offset;
> +
> +	return div64_u64(len * NSEC_PER_SEC, q->rate);
>  }
>  
>  static void tfifo_reset(struct Qdisc *sch)
> @@ -556,7 +553,7 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>  				now = last->time_to_send;
>  			}
>  
> -			delay += packet_len_2_sched_time(qdisc_pkt_len(skb), q);
> +			delay += packet_time_ns(qdisc_pkt_len(skb), q);
>  		}
>  
>  		cb->time_to_send = now + delay;
>
Stephen Hemminger Nov. 14, 2017, 7:09 p.m. | #2
On Tue, 14 Nov 2017 09:57:11 -0800
Randy Dunlap <rdunlap@infradead.org> wrote:

> On 11/14/2017 09:28 AM, Stephen Hemminger wrote:
> > Since times are now expressed in 64 bit nanosecond, need to now do
> > true 64 bit divide. Change the name of the function to bette express
> > the new units.
> > 
> > Fixes: 99803171ef04 ("netem: add uapi to express delay and jitter in nanoseconds")
> > Reported-by: Randy Dunlap <rdunlap@infradead.org>
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>  
> 
> Hi Stephen,
> 
> I still get it. Maybe it's this % operator:
> 
> 		skb->data[prandom_u32() % skb_headlen(skb)] ^=
> 			1<<(prandom_u32() % 8);
> 
> in net_enqueue().
> 
> > ---
> >  net/sched/sch_netem.c | 11 ++++-------
> >  1 file changed, 4 insertions(+), 7 deletions(-)
> > 
> > diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
> > index b686e755fda9..644323d6081c 100644
> > --- a/net/sched/sch_netem.c
> > +++ b/net/sched/sch_netem.c
> > @@ -339,10 +339,8 @@ static s64 tabledist(s64 mu, s64 sigma,
> >  	return  x / NETEM_DIST_SCALE + (sigma / NETEM_DIST_SCALE) * t + mu;
> >  }
> >  
> > -static u64 packet_len_2_sched_time(unsigned int len,
> > -				   struct netem_sched_data *q)
> > +static u64 packet_time_ns(u64 len, const struct netem_sched_data *q)
> >  {
> > -	u64 offset;
> >  	len += q->packet_overhead;
> >  
> >  	if (q->cell_size) {
> > @@ -352,9 +350,8 @@ static u64 packet_len_2_sched_time(unsigned int len,
> >  			cells++;
> >  		len = cells * (q->cell_size + q->cell_overhead);
> >  	}
> > -	offset = (u64)len * NSEC_PER_SEC;
> > -	do_div(offset, q->rate);
> > -	return offset;
> > +
> > +	return div64_u64(len * NSEC_PER_SEC, q->rate);
> >  }
> >  
> >  static void tfifo_reset(struct Qdisc *sch)
> > @@ -556,7 +553,7 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
> >  				now = last->time_to_send;
> >  			}
> >  
> > -			delay += packet_len_2_sched_time(qdisc_pkt_len(skb), q);
> > +			delay += packet_time_ns(qdisc_pkt_len(skb), q);
> >  		}
> >  
> >  		cb->time_to_send = now + delay;
> >   
> 
> 

Close, the problem is here:
		return (rnd % (2*sigma)) - sigma + mu;
    139d:	8b 4d dc             	mov    -0x24(%ebp),%ecx
    13a0:	8b 5d e0             	mov    -0x20(%ebp),%ebx
    13a3:	0f a4 cb 01          	shld   $0x1,%ecx,%ebx
    13a7:	01 c9                	add    %ecx,%ecx
    13a9:	53                   	push   %ebx
    13aa:	51                   	push   %ecx
    13ab:	e8 fc ff ff ff       	call   13ac <netem_enqueue+0x9dc>
			13ac: R_386_PC32	__moddi3

Will add second patch for that.

Patch

diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index b686e755fda9..644323d6081c 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -339,10 +339,8 @@  static s64 tabledist(s64 mu, s64 sigma,
 	return  x / NETEM_DIST_SCALE + (sigma / NETEM_DIST_SCALE) * t + mu;
 }
 
-static u64 packet_len_2_sched_time(unsigned int len,
-				   struct netem_sched_data *q)
+static u64 packet_time_ns(u64 len, const struct netem_sched_data *q)
 {
-	u64 offset;
 	len += q->packet_overhead;
 
 	if (q->cell_size) {
@@ -352,9 +350,8 @@  static u64 packet_len_2_sched_time(unsigned int len,
 			cells++;
 		len = cells * (q->cell_size + q->cell_overhead);
 	}
-	offset = (u64)len * NSEC_PER_SEC;
-	do_div(offset, q->rate);
-	return offset;
+
+	return div64_u64(len * NSEC_PER_SEC, q->rate);
 }
 
 static void tfifo_reset(struct Qdisc *sch)
@@ -556,7 +553,7 @@  static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 				now = last->time_to_send;
 			}
 
-			delay += packet_len_2_sched_time(qdisc_pkt_len(skb), q);
+			delay += packet_time_ns(qdisc_pkt_len(skb), q);
 		}
 
 		cb->time_to_send = now + delay;