> -----Original Message-----
> From: EXT Ivan Khoronzhuk [mailto:[email protected]]
> Sent: Tuesday, September 15, 2015 5:07 PM
> To: Savolainen, Petri (Nokia - FI/Espoo); [email protected]
> 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:[email protected]]
> >>>> Sent: Tuesday, September 15, 2015 3:08 PM
> >>>> To: Savolainen, Petri (Nokia - FI/Espoo); [email protected]
> >>>> 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:[email protected]] On
> Behalf Of
> >>>>>> EXT Ivan Khoronzhuk
> >>>>>> Sent: Friday, September 11, 2015 1:05 PM
> >>>>>> To: [email protected]
> >>>>>> 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 <[email protected]>
> >>>>>> ---
> >>>>>> 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
> >>>>>> [email protected]
> >>>>>> https://lists.linaro.org/mailman/listinfo/lng-odp
> >>>>
> >>>> --
> >>>> Regards,
> >>>> Ivan Khoronzhuk
> >>
> >
>
> --
> Regards,
> Ivan Khoronzhuk
_______________________________________________
lng-odp mailing list
[email protected]
https://lists.linaro.org/mailman/listinfo/lng-odp
