Petri,
On 16.09.15 10:02, Savolainen, Petri (Nokia - FI/Espoo) wrote:
-----Original Message-----
From: EXT Ivan Khoronzhuk [mailto:ivan.khoronz...@linaro.org]
Sent: Tuesday, September 15, 2015 5:07 PM
To: Savolainen, Petri (Nokia - FI/Espoo); lng-odp@lists.linaro.org
Subject: Re: [lng-odp] [PATCH v2 5/5] performance: odp_pktio_perf: fix
potential overflow for burst_gap
Petri,
On 15.09.15 16:01, Ivan Khoronzhuk wrote:
On 15.09.15 15:54, Ivan Khoronzhuk wrote:
On 15.09.15 15:45, Savolainen, Petri (Nokia - FI/Espoo) wrote:
-----Original Message-----
From: EXT Ivan Khoronzhuk [mailto:ivan.khoronz...@linaro.org]
Sent: Tuesday, September 15, 2015 3:08 PM
To: Savolainen, Petri (Nokia - FI/Espoo); lng-odp@lists.linaro.org
Subject: Re: [lng-odp] [PATCH v2 5/5] performance: odp_pktio_perf:
fix
potential overflow for burst_gap
On 15.09.15 14:42, Savolainen, Petri (Nokia - FI/Espoo) wrote:
-----Original Message-----
From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On
Behalf Of
EXT Ivan Khoronzhuk
Sent: Friday, September 11, 2015 1:05 PM
To: lng-odp@lists.linaro.org
Subject: [lng-odp] [PATCH v2 5/5] performance: odp_pktio_perf:
fix
potential overflow for burst_gap
The direct comparing of "cur_cycles" and "next_tx_cycles" is not
valid, as "next_tx_cycles" can be overflowed and comparison will
give wrong result. So use odp_time_diff_cycles() for that, as it
takes in account ticks overflow.
Signed-off-by: Ivan Khoronzhuk <ivan.khoronz...@linaro.org>
---
test/performance/odp_pktio_perf.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/test/performance/odp_pktio_perf.c
b/test/performance/odp_pktio_perf.c
index ac32b15..85ef2bc 100644
--- a/test/performance/odp_pktio_perf.c
+++ b/test/performance/odp_pktio_perf.c
@@ -303,7 +303,7 @@ static void *run_thread_tx(void *arg)
int thr_id;
odp_queue_t outq;
pkt_tx_stats_t *stats;
- uint64_t next_tx_cycles, start_cycles, cur_cycles,
send_duration;
+ uint64_t burst_start_cycles, start_cycles, cur_cycles,
send_duration;
uint64_t burst_gap_cycles;
uint32_t batch_len;
int unsent_pkts = 0;
@@ -334,11 +334,12 @@ static void *run_thread_tx(void *arg)
cur_cycles = odp_time_cycles();
start_cycles = cur_cycles;
- next_tx_cycles = cur_cycles;
+ burst_start_cycles = odp_time_diff_cycles(cur_cycles,
burst_gap_cycles);
Shouldn't this be:
burst_start_cycles = cur_cycles + burst_gap_cycles;
,which works as long as cycle count wraps at UINT64_MAX. Maybe we
need a cpu.h API for summing two values with correct wrapping.
It's initialization for burst gap, which is changing while
send_duration.
Current algorithm don't wait "burst gap" at first iteration, my
intention to not change it.
So I've used diff, in another case it waits one init gap.
In case of cur_cycles + burst_gap_cycles it waits 2 x burst_gap.
So
it's not correct.
I suppose here shouldn't be functional changes.
The cpu API doesn't have sum function and is not for this case, we
need
time here, that is
Time API is indented for. The new Time API is going to be added
after
this series and will
contain sum function which will replace "+" on odp_time_sum().
Current
API supposes that "+"
correctly handles UINT64_MAX wrap and doesn't contain sum
function.
For example:
burst_gap_cycles = 1k; // e.g. 1msec
cur_cycles = 1M; // wraps at 10M
burst_start_cycles = odp_time_diff_cycles(cur_cycles,
burst_gap_cycles);
burst_start_cycles = 10M - 1M + 1k = 9 001 000
I've checked it some time ago and it was working, I remember I
corrected this, strange.
Seems I forgot it to swap.
Let me check it again.
And maybe I will revise loop a little.
I dislike this diff at init also.
Yes, I forgot to swap burst_gap_cycles and cur_cycles.
so odp_time_diff_cycles(cur_cycles, burst_gap_cycles) ->
odp_time_diff_cycles(burst_gap_cycles, cur_cycles);
Are you OK with this? I will update it after some review of other
patches from this series.
Seems, it's simplest way to not wait at first iteration and not use
additional vars
or comparison with start time.
I think it works correctly only when cur_cycles > burst_gap_cycles (which is
likely but there's no guarantees).
In case of cur_cycles < burst_gap_cycles it should work.
burst_start_cycles = MAX - gap + cur.
For instance:
gap = 100;
cur = 5;
burst_start_cycles = MAX - 100 + 5 = MAX - 95;
After MAX it wraps to 0 + 5, so 95 + 5 = 100; That's correct.
No matter we diff range or counter from cur_time,
it will be correct, second argument cannot wrap.
So, if you are OK, we can leave it as simplest fix.
-Petri
while (odp_time_diff_cycles(start_cycles, cur_cycles) <
send_duration) {
unsigned alloc_cnt = 0, tx_cnt;
- if (cur_cycles < next_tx_cycles) {
+ if (odp_time_diff_cycles(burst_start_cycles,
cur_cycles)
+ < burst_gap_cycles) {
This spins back to the top of the while loop until 'cur_cycles' has
passed 'burst_start_cycles' by ' burst_gap_cycles'. So, in the example
the first packet is sent at cur_cycles == 9 002 000 cycles.
-Petri
cur_cycles = odp_time_cycles();
if (idle_start == 0)
idle_start = cur_cycles;
@@ -351,7 +352,7 @@ static void *run_thread_tx(void *arg)
idle_start = 0;
}
- next_tx_cycles += burst_gap_cycles;
+ burst_start_cycles += burst_gap_cycles;
alloc_cnt = alloc_packets(tx_event, batch_len -
unsent_pkts);
if (alloc_cnt != batch_len)
--
1.9.1
_______________________________________________
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp
--
Regards,
Ivan Khoronzhuk
--
Regards,
Ivan Khoronzhuk
--
Regards,
Ivan Khoronzhuk
_______________________________________________
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp