> -----Original Message----- > From: EXT Ivan Khoronzhuk [mailto:ivan.khoronz...@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.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- > o...@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.
It's OK. -Petri _______________________________________________ lng-odp mailing list lng-odp@lists.linaro.org https://lists.linaro.org/mailman/listinfo/lng-odp
