| Message ID | 1441965889-15727-6-git-send-email-ivan.khoronzhuk@linaro.org |
|---|---|
| State | Accepted |
| Commit | 13dda972a77468949981c6e558e0da9851954839 |
| Headers | show |
> -----Original Message----- > From: lng-odp [mailto:lng-odp-bounces@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.khoronzhuk@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. -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) { > 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
On 15.09.15 14:42, Savolainen, Petri (Nokia - FI/Espoo) wrote: > > >> -----Original Message----- >> From: lng-odp [mailto:lng-odp-bounces@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.khoronzhuk@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. > > > > > >> 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) { >> 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
> -----Original Message----- > From: EXT Ivan Khoronzhuk [mailto:ivan.khoronzhuk@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-bounces@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.khoronzhuk@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 > > > > > > > > > > > > >> 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
On 15.09.15 15:45, Savolainen, Petri (Nokia - FI/Espoo) wrote: > > >> -----Original Message----- >> From: EXT Ivan Khoronzhuk [mailto:ivan.khoronzhuk@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-bounces@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.khoronzhuk@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. > > >> >>> >>> >>> >>> >>> >>>> 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
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.khoronzhuk@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-bounces@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.khoronzhuk@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. > >> >> >>> >>>> >>>> >>>> >>>> >>>> >>>>> 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 >
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.khoronzhuk@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-bounces@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.khoronzhuk@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. > >> >>> >>> >>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>>>>> 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 >> >
> -----Original Message----- > From: EXT Ivan Khoronzhuk [mailto:ivan.khoronzhuk@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.khoronzhuk@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-bounces@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.khoronzhuk@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). Would it be more robust to init burst_start_cycles = cur_cycles, and thus wait also in the first iteration. Maybe iteration counting needs updating then, but the time calculation would be more robust. -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
On 16.09.15 10:02, Savolainen, Petri (Nokia - FI/Espoo) wrote: > > >> -----Original Message----- >> From: EXT Ivan Khoronzhuk [mailto:ivan.khoronzhuk@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.khoronzhuk@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-bounces@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.khoronzhuk@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). > Would it be more robust to init burst_start_cycles = cur_cycles, and thus wait also in the first iteration. > Maybe iteration counting needs updating then, but the time calculation would be more robust. It's not very good to miss first iteration. It's supposed that during burst period we send some number of packets. In case of first dummy gap it will simply wait, w/o sending. The results of test can be used for comparison with some previous results and can create additional questions. We shouldn't add even such small diff (or not small in some cases?) What about to init to init burst_start_cycles = cur_cycles and add some start = 1; if (odp_time_diff(burst_start_time, cur_time) < burst_gap && start != 1) { } start = 0; Or check if burst_start_time != start_time: if (odp_time_diff(burst_start_time, cur_time) < burst_gap && burst_start_time != start_time) { } What do you say? > > -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
On 16.09.15 10:52, Ivan Khoronzhuk wrote: > > > On 16.09.15 10:02, Savolainen, Petri (Nokia - FI/Espoo) wrote: >> >> >>> -----Original Message----- >>> From: EXT Ivan Khoronzhuk [mailto:ivan.khoronzhuk@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.khoronzhuk@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-bounces@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.khoronzhuk@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). >> Would it be more robust to init burst_start_cycles = cur_cycles, and thus wait also in the first iteration. >> Maybe iteration counting needs updating then, but the time calculation would be more robust. > > It's not very good to miss first iteration. > It's supposed that during burst period we send some number of packets. > In case of first dummy gap it will simply wait, w/o sending. > The results of test can be used for comparison with some previous results > and can create additional questions. We shouldn't add even such small diff (or not small in some cases?) > > What about to init to init burst_start_cycles = cur_cycles > and add some start = 1; > > if (odp_time_diff(burst_start_time, cur_time) < burst_gap && start != 1) { > } > > start = 0; > > Or check if burst_start_time != start_time: > if (odp_time_diff(burst_start_time, cur_time) < burst_gap && > burst_start_time != start_time) { > } > > What do you say? > And probably add also if (burst_start_time != start_time) burst_start_time += burst_gap; >>>>>>>> >>>>>>>> >>>>>>>>> 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 >
Petri, On 16.09.15 10:02, Savolainen, Petri (Nokia - FI/Espoo) wrote: > > >> -----Original Message----- >> From: EXT Ivan Khoronzhuk [mailto:ivan.khoronzhuk@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.khoronzhuk@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-bounces@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.khoronzhuk@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
> -----Original Message----- > From: EXT Ivan Khoronzhuk [mailto:ivan.khoronzhuk@linaro.org] > Sent: Wednesday, September 16, 2015 11:19 AM > 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 16.09.15 10:02, Savolainen, Petri (Nokia - FI/Espoo) wrote: > > > > > >> -----Original Message----- > >> From: EXT Ivan Khoronzhuk [mailto:ivan.khoronzhuk@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.khoronzhuk@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-bounces@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.khoronzhuk@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. It's OK. -Petri
On 16.09.15 12:16, Savolainen, Petri (Nokia - FI/Espoo) wrote: > > >> -----Original Message----- >> From: EXT Ivan Khoronzhuk [mailto:ivan.khoronzhuk@linaro.org] >> Sent: Wednesday, September 16, 2015 11:19 AM >> 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 16.09.15 10:02, Savolainen, Petri (Nokia - FI/Espoo) wrote: >>> >>> >>>> -----Original Message----- >>>> From: EXT Ivan Khoronzhuk [mailto:ivan.khoronzhuk@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.khoronzhuk@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-bounces@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.khoronzhuk@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. > > > It's OK. > > -Petri > It's fixed in v3
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); 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) { 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)
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.khoronzhuk@linaro.org> --- test/performance/odp_pktio_perf.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)