pktgen: bug when calling ndelay in x86 architectures

PATCH Applied Daniel Turull 4 participants 11 messages Oct 18, 2011 – Oct 20, 2011 1 message not in the archive

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Daniel Turull

author

2011-10-18 11:08 UTC #0 lore raw

PATCH 1/1 pktgen: bug when calling ndelay in x86 architectures

The value selected to delay the transmission in pktgen with the ndelay function should be lower. In Linux/arch/x86/include/asm/delay.h and Linux/arch/sh/include/asm/delay.h
the maximal expected value for a constant is 20000 ns.

Signed-off-by Daniel Turull <[email protected]>

---

net/core/pktgen.c +1 -1

index 796044a..e17bd41 100644
@@ -2145,7 +2145,7 @@ static void spin(struct pktgen_dev *pkt_dev, ktime_t spin_until)
 	}
 
 	start_time = ktime_now();
-	if (remaining < 100000)
+	if (remaining < 20000)
 		ndelay(remaining);	/* really small just spin */
 	else {
 		/* see do_nanosleep */

Eric Dumazet

2011-10-18 11:56 UTC #1 lore raw

Quoted 11 lines · Daniel Turull

The value selected to delay the transmission in pktgen with the ndelay function should be lower. In Linux/arch/x86/include/asm/delay.h and Linux/arch/sh/include/asm/delay.h
the maximal expected value for a constant is 20000 ns.
Signed-off-by Daniel Turull <[email protected]>
---
net/core/pktgen.c +0 -0
index 796044a..e17bd41 100644

net/core/pktgen.c static void spin(struct pktgen_dev *pkt_dev, ktime_t spin_until) · line 2151

Quoted 8 lines

net/core/pktgen.c +1 -1

@@ -2145,7 +2145,7 @@ static void spin(struct pktgen_dev *pkt_dev, ktime_t spin_until)
 	}
 
 	start_time = ktime_now();
-	if (remaining < 100000)
+	if (remaining < 20000)
 		ndelay(remaining);	/* really small just spin */
 	else {
 		/* see do_nanosleep */

But 'remaining' is not a constant.

If we want exactly 40.000 packets per second rate (25 us between packets), your patch makes this not quite possible without CONFIG_HIGH_RES_TIMERS and probable high jitter because of scheduler effects.

pktgen is kind of special, we _want_ a cpu for our exclusive use.

Ben Hutchings

2011-10-18 14:00 UTC #2 lore raw

Quoted 23 lines · Eric Dumazet

Quoted 11 lines · Daniel Turull
The value selected to delay the transmission in pktgen with the ndelay function should be lower. In Linux/arch/x86/include/asm/delay.h and Linux/arch/sh/include/asm/delay.h
the maximal expected value for a constant is 20000 ns.
Signed-off-by Daniel Turull <[email protected]>
---
net/core/pktgen.c +0 -0
index 796044a..e17bd41 100644
net/core/pktgen.c static void spin(struct pktgen_dev *pkt_dev, ktime_t spin_until) · line 2151
Quoted 8 lines
net/core/pktgen.c +1 -1
@@ -2145,7 +2145,7 @@ static void spin(struct pktgen_dev *pkt_dev, ktime_t spin_until)
 	}
 
 	start_time = ktime_now();
-	if (remaining < 100000)
+	if (remaining < 20000)
 		ndelay(remaining);	/* really small just spin */
 	else {
 		/* see do_nanosleep */
But 'remaining' is not a constant.

Quoted 6 lines

If we want exactly 40.000 packets per second rate (25 us between packets), your patch makes this not quite possible without CONFIG_HIGH_RES_TIMERS and probable high jitter because of scheduler effects.
pktgen is kind of special, we _want_ a cpu for our exclusive use.

AIUI, the reason for limits on delays is not that it's bad practice to spin for so long, but that the delay calculations may overflow or otherwise become inaccurate.

Ben.

Signature · 3 lines

Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Eric Dumazet

2011-10-18 14:47 UTC #3 lore raw

Quoted 3 lines · Ben Hutchings

AIUI, the reason for limits on delays is not that it's bad practice to spin for so long, but that the delay calculations may overflow or otherwise become inaccurate.

OK, I can understand that, then a more appropriate patch would be :

net/core/pktgen.c +6 -3

index 796044a..28bbf5b 100644
@@ -2145,9 +2145,12 @@ static void spin(struct pktgen_dev *pkt_dev, ktime_t spin_until)
 	}
 
 	start_time = ktime_now();
-	if (remaining < 100000)
-		ndelay(remaining);	/* really small just spin */
-	else {
+	if (remaining < 100000) {
+		if (remaining >= 10000)
+			udelay(remaining/1000);
+		else
+			ndelay(remaining);
+	} else {
 		/* see do_nanosleep */
 		hrtimer_init_sleeper(&t, current);
 		do {

A message that is not in the archive (sent off-list or not yet received): [email protected]

Eric Dumazet

2011-10-19 10:14 UTC #5 lore raw

Quoted 5 lines · Daniel Turull

Hi,
then if we want to use the spin more often.
maybe we can increase the constant from 100000 (0.1ms) to 1000000 (1ms)? How was the current value chosen?

Based on user needs ;)

Quoted 4 lines

I did some measurements of the inter-arrival time between packets and with bigger values the maximal is reduced in the rates between 2kpps and 20kpps.

ndelay()/udelay() have some inaccuracies, for 'long' values, because of rounding errors.

If we spin, just call ktime_now() in a loop until spin_until is reached...

That way you get max possible resolution, given kernel time service constraints.

Untested patch :

net/core/pktgen.c +6 -4

index 38d6577..5c7e900 100644
@@ -2145,9 +2145,11 @@ static void spin(struct pktgen_dev *pkt_dev, ktime_t spin_until)
 	}
 
 	start_time = ktime_now();
-	if (remaining < 100000)
-		ndelay(remaining);	/* really small just spin */
-	else {
+	if (remaining < 100000) {
+		do {
+			end_time = ktime_now();
+		} while (ktime_lt(end_time, spin_until));
+	} else {
 		/* see do_nanosleep */
 		hrtimer_init_sleeper(&t, current);
 		do {

@@ -2162,8 +2164,8 @@ static void spin(struct pktgen_dev *pkt_dev, ktime_t spin_until)
 			hrtimer_cancel(&t.timer);
 		} while (t.task && pkt_dev->running && !signal_pending(current));
 		__set_current_state(TASK_RUNNING);
+		end_time = ktime_now();
 	}
-	end_time = ktime_now();
 
 	pkt_dev->idle_acc += ktime_to_ns(ktime_sub(end_time, start_time));
 	pkt_dev->next_tx = ktime_add_ns(spin_until, pkt_dev->delay);

Daniel Turull

author

2011-10-20 13:23 UTC #6 lore raw

Hi,

I tested the patch and it works well.

Quoted 8 lines · Eric Dumazet

Quoted 5 lines · Daniel Turull
Hi,
then if we want to use the spin more often.
maybe we can increase the constant from 100000 (0.1ms) to 1000000 (1ms)? How was the current value chosen?
Based on user needs ;)

I think if we increase the constant to 1ms, we will reduce the jitter if we have a rate between 1kpps and 10 kpps, but I guess is not a big deal.

I've plot this new graph with this patch:
http://tslab.ssvl.kth.se/pktgen/img/inter_eric1.eps

Quoted 39 lines

Quoted 5 lines
I did some measurements of the inter-arrival time between packets and with bigger values the maximal is reduced in the rates between 2kpps and 20kpps.
ndelay()/udelay() have some inaccuracies, for 'long' values, because of rounding errors.
If we spin, just call ktime_now() in a loop until spin_until is reached...
That way you get max possible resolution, given kernel time service constraints.
Untested patch :
net/core/pktgen.c +5 -3
index 38d6577..5c7e900 100644
@@ -2145,9 +2145,11 @@ static void spin(struct pktgen_dev *pkt_dev, ktime_t spin_until)
 	}
 
 	start_time = ktime_now();
-	if (remaining < 100000)
-		ndelay(remaining);	/* really small just spin */
-	else {
+	if (remaining < 100000) {
+		do {
+			end_time = ktime_now();
+		} while (ktime_lt(end_time, spin_until));
+	} else {
 		/* see do_nanosleep */
 		hrtimer_init_sleeper(&t, current);
 		do {

net/core/pktgen.c static void spin(struct pktgen_dev *pkt_dev, ktime_t spin_until) · line 2171

Quoted 9 lines

net/core/pktgen.c +1 -1

@@ -2162,8 +2164,8 @@ static void spin(struct pktgen_dev *pkt_dev, ktime_t spin_until)
 			hrtimer_cancel(&t.timer);
 		} while (t.task && pkt_dev->running && !signal_pending(current));
 		__set_current_state(TASK_RUNNING);
+		end_time = ktime_now();
 	}
-	end_time = ktime_now();
 
 	pkt_dev->idle_acc += ktime_to_ns(ktime_sub(end_time, start_time));
 	pkt_dev->next_tx = ktime_add_ns(spin_until, pkt_dev->delay);

Daniel

Eric Dumazet

2011-10-20 13:44 UTC #7 lore raw

Quoted 4 lines · Daniel Turull

Hi,
I tested the patch and it works well.

Thanks !

Quoted 8 lines

I think if we increase the constant to 1ms, we will reduce the jitter if we have a rate between 1kpps and 10 kpps, but I guess is not a big deal.
I've plot this new graph with this patch:
http://tslab.ssvl.kth.se/pktgen/img/inter_eric1.eps

Unfortunately, the sender cpu might be preempted by timer irq or other expensive irq, so the Min/Max values are not very different I guess.

I dont understand your Min values.

At 100 pps, how is it possible to have a Min value of ~5000 ns ?

Daniel Turull

author

2011-10-20 14:26 UTC #8 lore raw

Quoted 18 lines · Eric Dumazet

Quoted 4 lines · Daniel Turull
Hi,
I tested the patch and it works well.
Thanks !
Quoted 8 lines
I think if we increase the constant to 1ms, we will reduce the jitter if we have a rate between 1kpps and 10 kpps, but I guess is not a big deal.
I've plot this new graph with this patch:
http://tslab.ssvl.kth.se/pktgen/img/inter_eric1.eps

Quoted 8 lines

Unfortunately, the sender cpu might be preempted by timer irq or other expensive irq, so the Min/Max values are not very different I guess.
I dont understand your Min values.
At 100 pps, how is it possible to have a Min value of ~5000 ns ?

My assumption is that for low rate, the min value is caused in the beginning of the test. When we start the transmission in pktgen_run(), we set the pkt_dev->next_tx to the current time but the are more operation to do, so the first transmission is a bit delayed. Even more if the cpu is preempted.
For the second packet, we are taking the pkt_dev->next_tx as a reference and add the delay, in order to decide when to send.
So, my guess is that the first packet is delayed
and then we send the second packet only after a short time, in order to keep the average rate in the transmission.

Daniel

David Miller

maintainer

2011-10-20 20:28 UTC #9 lore raw

In reply to Eric Dumazet

From: Eric Dumazet <[email protected]>
Date: Tue, 18 Oct 2011 16:47:44 +0200

Quoted 7 lines · Ben Hutchings

Quoted 4 lines
AIUI, the reason for limits on delays is not that it's bad practice to spin for so long, but that the delay calculations may overflow or otherwise become inaccurate.
OK, I can understand that, then a more appropriate patch would be :

I think doing the udelay/ndelay thing is the way to go for 'net' and -stable. We can do something sophisticated with ktime et al. in 'net-next'.

Eric, could you please formally submit this patch with proper changelog etc.?

Thanks.

Eric Dumazet

2011-10-20 20:55 UTC #10 lore raw

Quoted 9 lines · David Miller

From: Eric Dumazet <[email protected]>
Date: Tue, 18 Oct 2011 16:47:44 +0200
Quoted 6 lines · Ben Hutchings
Quoted 4 lines
AIUI, the reason for limits on delays is not that it's bad practice to spin for so long, but that the delay calculations may overflow or otherwise become inaccurate.

Quoted 6 lines

Quoted 1 line
OK, I can understand that, then a more appropriate patch would be :
I think doing the udelay/ndelay thing is the way to go for 'net' and -stable. We can do something sophisticated with ktime et al. in 'net-next'.

Well, I am not sure a patch is needed for net, since there is no bug, but maybe small inaccuracies ? Correct me if I misunderstood Daniel !

Quoted 2 lines

Eric, could you please formally submit this patch with proper changelog etc.?

Sure !

[PATCH net-next] pktgen: remove ndelay() call

Daniel Turull reported inaccuracies in pktgen when using low packet rates, because we call ndelay(val) with values bigger than 20000.

Instead of calling ndelay() for delays < 100us, we can instead loop calling ktime_now() only.

Reported-by Daniel Turull <[email protected]>

Signed-off-by Eric Dumazet <[email protected]>

---
 net/core/pktgen.c |   11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

net/core/pktgen.c +7 -4

index 6bbf008..0001c24 100644
@@ -2145,9 +2145,12 @@ static void spin(struct pktgen_dev *pkt_dev, ktime_t spin_until)
 	}
 
 	start_time = ktime_now();
-	if (remaining < 100000)
-		ndelay(remaining);	/* really small just spin */
-	else {
+	if (remaining < 100000) {
+		/* for small delays (<100us), just loop until limit is reached */
+		do {
+			end_time = ktime_now();
+		} while (ktime_lt(end_time, spin_until));
+	} else {
 		/* see do_nanosleep */
 		hrtimer_init_sleeper(&t, current);
 		do {

@@ -2162,8 +2165,8 @@ static void spin(struct pktgen_dev *pkt_dev, ktime_t spin_until)
 			hrtimer_cancel(&t.timer);
 		} while (t.task && pkt_dev->running && !signal_pending(current));
 		__set_current_state(TASK_RUNNING);
+		end_time = ktime_now();
 	}
-	end_time = ktime_now();
 
 	pkt_dev->idle_acc += ktime_to_ns(ktime_sub(end_time, start_time));
 	pkt_dev->next_tx = ktime_add_ns(spin_until, pkt_dev->delay);

David Miller

maintainer Applied

2011-10-20 21:04 UTC #11 lore raw

From: Eric Dumazet <[email protected]>
Date: Thu, 20 Oct 2011 22:55:38 +0200

Quoted 2 lines

Well, I am not sure a patch is needed for net, since there is no bug, but maybe small inaccuracies ? Correct me if I misunderstood Daniel !

Ok that appears to be the case, so this is not something we should deal with in the 'net' tree.

The constant ndelay() case would purposely cause a build failure for values larger than 40000, but the specific call site we are discussing in pktgen is never constant and therefore should would never trigger that bug check.

Quoted 6 lines

Quoted 2 lines
Eric, could you please formally submit this patch with proper changelog etc.?
Sure !
[PATCH net-next] pktgen: remove ndelay() call

Applied, thanks!