| Message ID | 1438870038-25155-2-git-send-email-ivan.khoronzhuk@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Thu, Aug 06, 2015 at 05:07:17PM +0300, Ivan Khoronzhuk wrote: > The direct comparing of "cur_cycles" and "end_cycles" is not valid, > as "end_cycles" can be overflowed and comparison will give wrong > result. So use odp_time_diff_cycles() for that, as it takes in > account cycles overflow. > > Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> > --- > test/performance/odp_pktio_perf.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/test/performance/odp_pktio_perf.c b/test/performance/odp_pktio_perf.c > index fcbc4ec..8f1daeb 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, end_cycles, cur_cycles; > + uint64_t next_tx_cycles, end_cycles, cur_cycles, send_duration; > uint64_t burst_gap_cycles; > uint32_t batch_len; > int unsent_pkts = 0; > @@ -331,12 +331,11 @@ static void *run_thread_tx(void *arg) > > odp_barrier_wait(&globals->tx_barrier); > > + send_duration = odp_time_ns_to_cycles(targs->duration * ODP_TIME_SEC); > cur_cycles = odp_time_cycles(); > next_tx_cycles = cur_cycles; > - end_cycles = cur_cycles + > - odp_time_ns_to_cycles(targs->duration * ODP_TIME_SEC); > - > - while (cur_cycles < end_cycles) { > + end_cycles = cur_cycles + send_duration; > + while (odp_time_diff_cycles(cur_cycles, end_cycles) < send_duration) { This isn't right. The initial condition here is that the difference is exactly equal to send_duration, so this loop won't be entered at all. I would have thought the right way to fix this is to record the start_cycles then do; while (odp_time_diff_cycles(start_cycles, cur_cycles) < send_duration) { It looks like odp_time_diff_cycles() as it assumes a wrap when t1==t2, but I think that should be fixed to just return 0. How often does your counter overflow? there's still going to be a problem if it overflows in less than targs->duration seconds, if that's possible though it's probably not a suitable counter for this purpose.
Hi, Stuart On 06.08.15 19:44, Stuart Haslam wrote: > On Thu, Aug 06, 2015 at 05:07:17PM +0300, Ivan Khoronzhuk wrote: >> The direct comparing of "cur_cycles" and "end_cycles" is not valid, >> as "end_cycles" can be overflowed and comparison will give wrong >> result. So use odp_time_diff_cycles() for that, as it takes in >> account cycles overflow. >> >> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> >> --- >> test/performance/odp_pktio_perf.c | 9 ++++----- >> 1 file changed, 4 insertions(+), 5 deletions(-) >> >> diff --git a/test/performance/odp_pktio_perf.c b/test/performance/odp_pktio_perf.c >> index fcbc4ec..8f1daeb 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, end_cycles, cur_cycles; >> + uint64_t next_tx_cycles, end_cycles, cur_cycles, send_duration; >> uint64_t burst_gap_cycles; >> uint32_t batch_len; >> int unsent_pkts = 0; >> @@ -331,12 +331,11 @@ static void *run_thread_tx(void *arg) >> >> odp_barrier_wait(&globals->tx_barrier); >> >> + send_duration = odp_time_ns_to_cycles(targs->duration * ODP_TIME_SEC); >> cur_cycles = odp_time_cycles(); >> next_tx_cycles = cur_cycles; >> - end_cycles = cur_cycles + >> - odp_time_ns_to_cycles(targs->duration * ODP_TIME_SEC); >> - >> - while (cur_cycles < end_cycles) { >> + end_cycles = cur_cycles + send_duration; >> + while (odp_time_diff_cycles(cur_cycles, end_cycles) < send_duration) { > > This isn't right. The initial condition here is that the difference is > exactly equal to send_duration, so this loop won't be entered at all. That's why I had sent v2 > > I would have thought the right way to fix this is to record the > start_cycles then do; It was my zero version. It requires more changes w/o reasons. > > while (odp_time_diff_cycles(start_cycles, cur_cycles) < send_duration) { > > It looks like odp_time_diff_cycles() as it assumes a wrap when t1==t2, > but I think that should be fixed to just return 0. Now it's not changed, so fix is correct. > > How often does your counter overflow? there's still going to be a problem It's more then possible. > if it overflows in less than targs->duration seconds, if that's possible It has nothing to do with duration. You never know the current counter state when start the test. Do you think you reset it at init? It can be in any state at test time. > though it's probably not a suitable counter for this purpose. counter is always 64bit according to the API. It cannot be another, as all ODP test based on it ). The freq can be ~ CPU freq. >
On 06.08.15 20:06, Ivan Khoronzhuk wrote: > Hi, Stuart > > On 06.08.15 19:44, Stuart Haslam wrote: >> On Thu, Aug 06, 2015 at 05:07:17PM +0300, Ivan Khoronzhuk wrote: >>> The direct comparing of "cur_cycles" and "end_cycles" is not valid, >>> as "end_cycles" can be overflowed and comparison will give wrong >>> result. So use odp_time_diff_cycles() for that, as it takes in >>> account cycles overflow. >>> >>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> >>> --- >>> test/performance/odp_pktio_perf.c | 9 ++++----- >>> 1 file changed, 4 insertions(+), 5 deletions(-) >>> >>> diff --git a/test/performance/odp_pktio_perf.c >>> b/test/performance/odp_pktio_perf.c >>> index fcbc4ec..8f1daeb 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, end_cycles, cur_cycles; >>> + uint64_t next_tx_cycles, end_cycles, cur_cycles, send_duration; >>> uint64_t burst_gap_cycles; >>> uint32_t batch_len; >>> int unsent_pkts = 0; >>> @@ -331,12 +331,11 @@ static void *run_thread_tx(void *arg) >>> >>> odp_barrier_wait(&globals->tx_barrier); >>> >>> + send_duration = odp_time_ns_to_cycles(targs->duration * >>> ODP_TIME_SEC); >>> cur_cycles = odp_time_cycles(); >>> next_tx_cycles = cur_cycles; >>> - end_cycles = cur_cycles + >>> - odp_time_ns_to_cycles(targs->duration * ODP_TIME_SEC); >>> - >>> - while (cur_cycles < end_cycles) { >>> + end_cycles = cur_cycles + send_duration; >>> + while (odp_time_diff_cycles(cur_cycles, end_cycles) < >>> send_duration) { >> >> This isn't right. The initial condition here is that the difference is >> exactly equal to send_duration, so this loop won't be entered at all. > > That's why I had sent v2 > >> >> I would have thought the right way to fix this is to record the >> start_cycles then do; > > It was my zero version. It requires more changes w/o reasons. > But I have no objection to back to it. It allows a little more gap to avoid overlap. > > >> >
On Thu, Aug 06, 2015 at 08:06:18PM +0300, Ivan Khoronzhuk wrote: > Hi, Stuart > > On 06.08.15 19:44, Stuart Haslam wrote: > >On Thu, Aug 06, 2015 at 05:07:17PM +0300, Ivan Khoronzhuk wrote: > >>The direct comparing of "cur_cycles" and "end_cycles" is not valid, > >>as "end_cycles" can be overflowed and comparison will give wrong > >>result. So use odp_time_diff_cycles() for that, as it takes in > >>account cycles overflow. > >> > >>Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> > >>--- > >> test/performance/odp_pktio_perf.c | 9 ++++----- > >> 1 file changed, 4 insertions(+), 5 deletions(-) > >> > >>diff --git a/test/performance/odp_pktio_perf.c b/test/performance/odp_pktio_perf.c > >>index fcbc4ec..8f1daeb 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, end_cycles, cur_cycles; > >>+ uint64_t next_tx_cycles, end_cycles, cur_cycles, send_duration; > >> uint64_t burst_gap_cycles; > >> uint32_t batch_len; > >> int unsent_pkts = 0; > >>@@ -331,12 +331,11 @@ static void *run_thread_tx(void *arg) > >> > >> odp_barrier_wait(&globals->tx_barrier); > >> > >>+ send_duration = odp_time_ns_to_cycles(targs->duration * ODP_TIME_SEC); > >> cur_cycles = odp_time_cycles(); > >> next_tx_cycles = cur_cycles; > >>- end_cycles = cur_cycles + > >>- odp_time_ns_to_cycles(targs->duration * ODP_TIME_SEC); > >>- > >>- while (cur_cycles < end_cycles) { > >>+ end_cycles = cur_cycles + send_duration; > >>+ while (odp_time_diff_cycles(cur_cycles, end_cycles) < send_duration) { > > > >This isn't right. The initial condition here is that the difference is > >exactly equal to send_duration, so this loop won't be entered at all. > > That's why I had sent v2 > > > > >I would have thought the right way to fix this is to record the > >start_cycles then do; > > It was my zero version. It requires more changes w/o reasons. > > > > >while (odp_time_diff_cycles(start_cycles, cur_cycles) < send_duration) { > > > >It looks like odp_time_diff_cycles() as it assumes a wrap when t1==t2, > >but I think that should be fixed to just return 0. > > Now it's not changed, so fix is correct. > > > > >How often does your counter overflow? there's still going to be a problem > > It's more then possible. > > >if it overflows in less than targs->duration seconds, if that's possible > > It has nothing to do with duration. You never know the current counter > state when start the test. Do you think you reset it at init? It can > be in any state at test time. Duration is important, you can't measure a 60 second interval in this way using a counter that wraps every 20 seconds. I asked because I wasn't sure if you were actually hitting an issue with this code or fixing a theoretical one, given that a 64-bit counter will wrap every couple of centuries or so, I thought you may be using a 32-bit counter. > >though it's probably not a suitable counter for this purpose. > > counter is always 64bit according to the API. It cannot be another, > as all ODP test based on it ). The freq can be ~ CPU freq. > It has to return a 64-bit value but it's not specified that it has to be a 64-bit counter or what frequency it runs at.. we should probably add some guidelines for acceptable ranges though. -- Stuart.
diff --git a/test/performance/odp_pktio_perf.c b/test/performance/odp_pktio_perf.c index fcbc4ec..8f1daeb 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, end_cycles, cur_cycles; + uint64_t next_tx_cycles, end_cycles, cur_cycles, send_duration; uint64_t burst_gap_cycles; uint32_t batch_len; int unsent_pkts = 0; @@ -331,12 +331,11 @@ static void *run_thread_tx(void *arg) odp_barrier_wait(&globals->tx_barrier); + send_duration = odp_time_ns_to_cycles(targs->duration * ODP_TIME_SEC); cur_cycles = odp_time_cycles(); next_tx_cycles = cur_cycles; - end_cycles = cur_cycles + - odp_time_ns_to_cycles(targs->duration * ODP_TIME_SEC); - - while (cur_cycles < end_cycles) { + end_cycles = cur_cycles + send_duration; + while (odp_time_diff_cycles(cur_cycles, end_cycles) < send_duration) { unsigned alloc_cnt = 0, tx_cnt; if (cur_cycles < next_tx_cycles) {
The direct comparing of "cur_cycles" and "end_cycles" is not valid, as "end_cycles" can be overflowed and comparison will give wrong result. So use odp_time_diff_cycles() for that, as it takes in account cycles overflow. Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> --- test/performance/odp_pktio_perf.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)