| Message ID | 1461168154-6285-2-git-send-email-maxim.uvarov@linaro.org |
|---|---|
| State | New |
| Headers | show |
Hi Maxim, Some comments below. -Matias > -----Original Message----- > From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of EXT Maxim > Uvarov > Sent: Wednesday, April 20, 2016 7:03 PM > To: lng-odp@lists.linaro.org > Subject: [lng-odp] [PATCHv2 1/2] example: packet: add odp_term_global > > Add odp_term_global and comment that it will never > be called. > https://bugs.linaro.org/show_bug.cgi?id=1706 > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> > --- > example/packet/odp_pktio.c | 56 ++++++++++++++++++++++++++++++++++-- > ---------- > 1 file changed, 42 insertions(+), 14 deletions(-) > > diff --git a/example/packet/odp_pktio.c b/example/packet/odp_pktio.c > index dd3e8e3..7110123 100644 > --- a/example/packet/odp_pktio.c > +++ b/example/packet/odp_pktio.c > @@ -69,6 +69,7 @@ typedef struct { > char **if_names; /**< Array of pointers to interface names */ > int mode; /**< Packet IO mode */ > char *if_str; /**< Storage for interface names */ > + int time; /**< Time to run app */ > } appl_args_t; > > /** > @@ -77,6 +78,7 @@ typedef struct { > typedef struct { > char *pktio_dev; /**< Interface name to use */ > int mode; /**< Thread mode */ > + int *exit; /**< Exit the thread */ I see no use for this pointer at least for now. > } thread_args_t; > > /** > @@ -98,6 +100,7 @@ static void swap_pkt_addrs(odp_packet_t pkt_tbl[], > unsigned len); > static void parse_args(int argc, char *argv[], appl_args_t *appl_args); > static void print_info(char *progname, appl_args_t *appl_args); > static void usage(char *progname); > +static int exit_threads; I would move 'exit_threads' inside args_t, so all global application data would be grouped inside one data structure. > > /** > * Create a pktio handle, optionally associating a default input queue. > @@ -177,6 +180,7 @@ static void *pktio_queue_thread(void *arg) > odp_event_t ev; > unsigned long pkt_cnt = 0; > unsigned long err_cnt = 0; > + uint64_t sched_wait = > odp_schedule_wait_time(ODP_TIME_MSEC_IN_NS * 100); > > thr = odp_thread_id(); > thr_args = arg; > @@ -203,18 +207,20 @@ static void *pktio_queue_thread(void *arg) > } > > /* Loop packets */ > - for (;;) { > + while (!exit_threads) { > odp_pktio_t pktio_tmp; > > - if (inq != ODP_QUEUE_INVALID) { > + if (inq != ODP_QUEUE_INVALID) > ev = odp_queue_deq(inq); > - pkt = odp_packet_from_event(ev); > - if (!odp_packet_is_valid(pkt)) > - continue; > - } else { > - ev = odp_schedule(NULL, ODP_SCHED_WAIT); > - pkt = odp_packet_from_event(ev); > - } > + else > + ev = odp_schedule(NULL, sched_wait); > + > + if (ev == ODP_EVENT_INVALID) > + continue; > + > + pkt = odp_packet_from_event(ev); > + if (!odp_packet_is_valid(pkt)) > + continue; > > /* Drop packets with errors */ > if (odp_unlikely(drop_err_pkts(&pkt, 1) == 0)) { > @@ -246,7 +252,6 @@ static void *pktio_queue_thread(void *arg) > } > } > > -/* unreachable */ > return NULL; > } > > @@ -292,7 +297,7 @@ static void *pktio_ifburst_thread(void *arg) > } > > /* Loop packets */ > - for (;;) { > + while (!exit_threads) { > pkts = odp_pktin_recv(pktin, pkt_tbl, MAX_PKT_BURST); > if (pkts > 0) { > /* Drop packets with errors */ > @@ -329,7 +334,6 @@ static void *pktio_ifburst_thread(void *arg) > } > } > > -/* unreachable */ > return NULL; > } > > @@ -416,6 +420,7 @@ int main(int argc, char *argv[]) > > args->thread[i].pktio_dev = args->appl.if_names[if_idx]; > args->thread[i].mode = args->appl.mode; > + args->thread[i].exit = &exit_threads; > > if (args->appl.mode == APPL_MODE_PKT_BURST) > thr_run_func = pktio_ifburst_thread; > @@ -435,15 +440,31 @@ int main(int argc, char *argv[]) > cpu = odp_cpumask_next(&cpumask, cpu); > } > > + if (args->appl.time) { > + sleep(args->appl.time); > + for (i = 0; i < num_workers; ++i) { Should probably be 'i < args->appl.if_count'. > + odp_pktio_t pktio; > + > + pktio = odp_pktio_lookup(args->thread[i].pktio_dev); > + odp_pktio_stop(pktio); > + } > + sleep(1); /* use simple delay to let workers clean up queues */ > + exit_threads = 1; > + } You could use odp_time_wait_ns() instead of sleep. > + > /* Master thread waits for other threads to exit */ > odph_linux_pthread_join(thread_tbl, num_workers); > > + for (i = 0; i < num_workers; ++i) Should probably be 'i < args->appl.if_count'. > + odp_pktio_close(odp_pktio_lookup(args->thread[i].pktio_dev)); > + > free(args->appl.if_names); > free(args->appl.if_str); > free(args); > - printf("Exit\n\n"); > > - return 0; > + odp_pool_destroy(pool); > + odp_term_local(); > + return odp_term_global(); odp_term_global() is missing instance argument. > } > > /** > @@ -529,8 +550,10 @@ static void parse_args(int argc, char *argv[], appl_args_t > *appl_args) > char *token; > size_t len; > int i; > + > static struct option longopts[] = { > {"count", required_argument, NULL, 'c'}, > + {"time", required_argument, NULL, 't'}, > {"interface", required_argument, NULL, 'i'}, /* return 'i' */ > {"mode", required_argument, NULL, 'm'}, /* return > 'm' */ > {"help", no_argument, NULL, 'h'}, /* return 'h' */ > @@ -538,6 +561,7 @@ static void parse_args(int argc, char *argv[], appl_args_t > *appl_args) > }; > > appl_args->mode = APPL_MODE_PKT_SCHED; > + appl_args->time = 0; /**< loop forever */ > > while (1) { > opt = getopt_long(argc, argv, "+c:i:+m:t:h", > @@ -550,6 +574,9 @@ static void parse_args(int argc, char *argv[], appl_args_t > *appl_args) > case 'c': > appl_args->cpu_count = atoi(optarg); > break; > + case 't': > + appl_args->time = atoi(optarg); > + break; > /* parse packet-io interface names */ > case 'i': > len = strlen(optarg); > @@ -685,6 +712,7 @@ static void usage(char *progname) > "\n" > "Optional OPTIONS\n" > " -c, --count <number> CPU count.\n" > + " -t, --time <seconds> Number of seconds to run.\n" > " -m, --mode 0: Receive and send directly (no queues)\n" > " 1: Receive and send via queues.\n" > " 2: Receive via scheduler, send via queues.\n" > -- > 2.7.1.250.gff4ea60 > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp
On 04/25/16 13:26, Elo, Matias (Nokia - FI/Espoo) wrote: > Hi Maxim, > > Some comments below. > > -Matias Thanks, I will send v3. (instance is missing be cause that patch was not yet merged.) Maxim. >> -----Original Message----- >> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of EXT Maxim >> Uvarov >> Sent: Wednesday, April 20, 2016 7:03 PM >> To: lng-odp@lists.linaro.org >> Subject: [lng-odp] [PATCHv2 1/2] example: packet: add odp_term_global >> >> Add odp_term_global and comment that it will never >> be called. >> https://bugs.linaro.org/show_bug.cgi?id=1706 >> >> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> >> --- >> example/packet/odp_pktio.c | 56 ++++++++++++++++++++++++++++++++++-- >> ---------- >> 1 file changed, 42 insertions(+), 14 deletions(-) >> >> diff --git a/example/packet/odp_pktio.c b/example/packet/odp_pktio.c >> index dd3e8e3..7110123 100644 >> --- a/example/packet/odp_pktio.c >> +++ b/example/packet/odp_pktio.c >> @@ -69,6 +69,7 @@ typedef struct { >> char **if_names; /**< Array of pointers to interface names */ >> int mode; /**< Packet IO mode */ >> char *if_str; /**< Storage for interface names */ >> + int time; /**< Time to run app */ >> } appl_args_t; >> >> /** >> @@ -77,6 +78,7 @@ typedef struct { >> typedef struct { >> char *pktio_dev; /**< Interface name to use */ >> int mode; /**< Thread mode */ >> + int *exit; /**< Exit the thread */ > I see no use for this pointer at least for now. > >> } thread_args_t; >> >> /** >> @@ -98,6 +100,7 @@ static void swap_pkt_addrs(odp_packet_t pkt_tbl[], >> unsigned len); >> static void parse_args(int argc, char *argv[], appl_args_t *appl_args); >> static void print_info(char *progname, appl_args_t *appl_args); >> static void usage(char *progname); >> +static int exit_threads; > I would move 'exit_threads' inside args_t, so all global application data would be grouped inside one data structure. > >> /** >> * Create a pktio handle, optionally associating a default input queue. >> @@ -177,6 +180,7 @@ static void *pktio_queue_thread(void *arg) >> odp_event_t ev; >> unsigned long pkt_cnt = 0; >> unsigned long err_cnt = 0; >> + uint64_t sched_wait = >> odp_schedule_wait_time(ODP_TIME_MSEC_IN_NS * 100); >> >> thr = odp_thread_id(); >> thr_args = arg; >> @@ -203,18 +207,20 @@ static void *pktio_queue_thread(void *arg) >> } >> >> /* Loop packets */ >> - for (;;) { >> + while (!exit_threads) { >> odp_pktio_t pktio_tmp; >> >> - if (inq != ODP_QUEUE_INVALID) { >> + if (inq != ODP_QUEUE_INVALID) >> ev = odp_queue_deq(inq); >> - pkt = odp_packet_from_event(ev); >> - if (!odp_packet_is_valid(pkt)) >> - continue; >> - } else { >> - ev = odp_schedule(NULL, ODP_SCHED_WAIT); >> - pkt = odp_packet_from_event(ev); >> - } >> + else >> + ev = odp_schedule(NULL, sched_wait); >> + >> + if (ev == ODP_EVENT_INVALID) >> + continue; >> + >> + pkt = odp_packet_from_event(ev); >> + if (!odp_packet_is_valid(pkt)) >> + continue; >> >> /* Drop packets with errors */ >> if (odp_unlikely(drop_err_pkts(&pkt, 1) == 0)) { >> @@ -246,7 +252,6 @@ static void *pktio_queue_thread(void *arg) >> } >> } >> >> -/* unreachable */ >> return NULL; >> } >> >> @@ -292,7 +297,7 @@ static void *pktio_ifburst_thread(void *arg) >> } >> >> /* Loop packets */ >> - for (;;) { >> + while (!exit_threads) { >> pkts = odp_pktin_recv(pktin, pkt_tbl, MAX_PKT_BURST); >> if (pkts > 0) { >> /* Drop packets with errors */ >> @@ -329,7 +334,6 @@ static void *pktio_ifburst_thread(void *arg) >> } >> } >> >> -/* unreachable */ >> return NULL; >> } >> >> @@ -416,6 +420,7 @@ int main(int argc, char *argv[]) >> >> args->thread[i].pktio_dev = args->appl.if_names[if_idx]; >> args->thread[i].mode = args->appl.mode; >> + args->thread[i].exit = &exit_threads; >> >> if (args->appl.mode == APPL_MODE_PKT_BURST) >> thr_run_func = pktio_ifburst_thread; >> @@ -435,15 +440,31 @@ int main(int argc, char *argv[]) >> cpu = odp_cpumask_next(&cpumask, cpu); >> } >> >> + if (args->appl.time) { >> + sleep(args->appl.time); >> + for (i = 0; i < num_workers; ++i) { > Should probably be 'i < args->appl.if_count'. > >> + odp_pktio_t pktio; >> + >> + pktio = odp_pktio_lookup(args->thread[i].pktio_dev); >> + odp_pktio_stop(pktio); >> + } >> + sleep(1); /* use simple delay to let workers clean up queues */ >> + exit_threads = 1; >> + } > You could use odp_time_wait_ns() instead of sleep. > >> + >> /* Master thread waits for other threads to exit */ >> odph_linux_pthread_join(thread_tbl, num_workers); >> >> + for (i = 0; i < num_workers; ++i) > Should probably be 'i < args->appl.if_count'. > >> + odp_pktio_close(odp_pktio_lookup(args->thread[i].pktio_dev)); >> + >> free(args->appl.if_names); >> free(args->appl.if_str); >> free(args); >> - printf("Exit\n\n"); >> >> - return 0; >> + odp_pool_destroy(pool); >> + odp_term_local(); >> + return odp_term_global(); > odp_term_global() is missing instance argument. > >> } >> >> /** >> @@ -529,8 +550,10 @@ static void parse_args(int argc, char *argv[], appl_args_t >> *appl_args) >> char *token; >> size_t len; >> int i; >> + >> static struct option longopts[] = { >> {"count", required_argument, NULL, 'c'}, >> + {"time", required_argument, NULL, 't'}, >> {"interface", required_argument, NULL, 'i'}, /* return 'i' */ >> {"mode", required_argument, NULL, 'm'}, /* return >> 'm' */ >> {"help", no_argument, NULL, 'h'}, /* return 'h' */ >> @@ -538,6 +561,7 @@ static void parse_args(int argc, char *argv[], appl_args_t >> *appl_args) >> }; >> >> appl_args->mode = APPL_MODE_PKT_SCHED; >> + appl_args->time = 0; /**< loop forever */ >> >> while (1) { >> opt = getopt_long(argc, argv, "+c:i:+m:t:h", >> @@ -550,6 +574,9 @@ static void parse_args(int argc, char *argv[], appl_args_t >> *appl_args) >> case 'c': >> appl_args->cpu_count = atoi(optarg); >> break; >> + case 't': >> + appl_args->time = atoi(optarg); >> + break; >> /* parse packet-io interface names */ >> case 'i': >> len = strlen(optarg); >> @@ -685,6 +712,7 @@ static void usage(char *progname) >> "\n" >> "Optional OPTIONS\n" >> " -c, --count <number> CPU count.\n" >> + " -t, --time <seconds> Number of seconds to run.\n" >> " -m, --mode 0: Receive and send directly (no queues)\n" >> " 1: Receive and send via queues.\n" >> " 2: Receive via scheduler, send via queues.\n" >> -- >> 2.7.1.250.gff4ea60 >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> https://lists.linaro.org/mailman/listinfo/lng-odp
diff --git a/example/packet/odp_pktio.c b/example/packet/odp_pktio.c index dd3e8e3..7110123 100644 --- a/example/packet/odp_pktio.c +++ b/example/packet/odp_pktio.c @@ -69,6 +69,7 @@ typedef struct { char **if_names; /**< Array of pointers to interface names */ int mode; /**< Packet IO mode */ char *if_str; /**< Storage for interface names */ + int time; /**< Time to run app */ } appl_args_t; /** @@ -77,6 +78,7 @@ typedef struct { typedef struct { char *pktio_dev; /**< Interface name to use */ int mode; /**< Thread mode */ + int *exit; /**< Exit the thread */ } thread_args_t; /** @@ -98,6 +100,7 @@ static void swap_pkt_addrs(odp_packet_t pkt_tbl[], unsigned len); static void parse_args(int argc, char *argv[], appl_args_t *appl_args); static void print_info(char *progname, appl_args_t *appl_args); static void usage(char *progname); +static int exit_threads; /** * Create a pktio handle, optionally associating a default input queue. @@ -177,6 +180,7 @@ static void *pktio_queue_thread(void *arg) odp_event_t ev; unsigned long pkt_cnt = 0; unsigned long err_cnt = 0; + uint64_t sched_wait = odp_schedule_wait_time(ODP_TIME_MSEC_IN_NS * 100); thr = odp_thread_id(); thr_args = arg; @@ -203,18 +207,20 @@ static void *pktio_queue_thread(void *arg) } /* Loop packets */ - for (;;) { + while (!exit_threads) { odp_pktio_t pktio_tmp; - if (inq != ODP_QUEUE_INVALID) { + if (inq != ODP_QUEUE_INVALID) ev = odp_queue_deq(inq); - pkt = odp_packet_from_event(ev); - if (!odp_packet_is_valid(pkt)) - continue; - } else { - ev = odp_schedule(NULL, ODP_SCHED_WAIT); - pkt = odp_packet_from_event(ev); - } + else + ev = odp_schedule(NULL, sched_wait); + + if (ev == ODP_EVENT_INVALID) + continue; + + pkt = odp_packet_from_event(ev); + if (!odp_packet_is_valid(pkt)) + continue; /* Drop packets with errors */ if (odp_unlikely(drop_err_pkts(&pkt, 1) == 0)) { @@ -246,7 +252,6 @@ static void *pktio_queue_thread(void *arg) } } -/* unreachable */ return NULL; } @@ -292,7 +297,7 @@ static void *pktio_ifburst_thread(void *arg) } /* Loop packets */ - for (;;) { + while (!exit_threads) { pkts = odp_pktin_recv(pktin, pkt_tbl, MAX_PKT_BURST); if (pkts > 0) { /* Drop packets with errors */ @@ -329,7 +334,6 @@ static void *pktio_ifburst_thread(void *arg) } } -/* unreachable */ return NULL; } @@ -416,6 +420,7 @@ int main(int argc, char *argv[]) args->thread[i].pktio_dev = args->appl.if_names[if_idx]; args->thread[i].mode = args->appl.mode; + args->thread[i].exit = &exit_threads; if (args->appl.mode == APPL_MODE_PKT_BURST) thr_run_func = pktio_ifburst_thread; @@ -435,15 +440,31 @@ int main(int argc, char *argv[]) cpu = odp_cpumask_next(&cpumask, cpu); } + if (args->appl.time) { + sleep(args->appl.time); + for (i = 0; i < num_workers; ++i) { + odp_pktio_t pktio; + + pktio = odp_pktio_lookup(args->thread[i].pktio_dev); + odp_pktio_stop(pktio); + } + sleep(1); /* use simple delay to let workers clean up queues */ + exit_threads = 1; + } + /* Master thread waits for other threads to exit */ odph_linux_pthread_join(thread_tbl, num_workers); + for (i = 0; i < num_workers; ++i) + odp_pktio_close(odp_pktio_lookup(args->thread[i].pktio_dev)); + free(args->appl.if_names); free(args->appl.if_str); free(args); - printf("Exit\n\n"); - return 0; + odp_pool_destroy(pool); + odp_term_local(); + return odp_term_global(); } /** @@ -529,8 +550,10 @@ static void parse_args(int argc, char *argv[], appl_args_t *appl_args) char *token; size_t len; int i; + static struct option longopts[] = { {"count", required_argument, NULL, 'c'}, + {"time", required_argument, NULL, 't'}, {"interface", required_argument, NULL, 'i'}, /* return 'i' */ {"mode", required_argument, NULL, 'm'}, /* return 'm' */ {"help", no_argument, NULL, 'h'}, /* return 'h' */ @@ -538,6 +561,7 @@ static void parse_args(int argc, char *argv[], appl_args_t *appl_args) }; appl_args->mode = APPL_MODE_PKT_SCHED; + appl_args->time = 0; /**< loop forever */ while (1) { opt = getopt_long(argc, argv, "+c:i:+m:t:h", @@ -550,6 +574,9 @@ static void parse_args(int argc, char *argv[], appl_args_t *appl_args) case 'c': appl_args->cpu_count = atoi(optarg); break; + case 't': + appl_args->time = atoi(optarg); + break; /* parse packet-io interface names */ case 'i': len = strlen(optarg); @@ -685,6 +712,7 @@ static void usage(char *progname) "\n" "Optional OPTIONS\n" " -c, --count <number> CPU count.\n" + " -t, --time <seconds> Number of seconds to run.\n" " -m, --mode 0: Receive and send directly (no queues)\n" " 1: Receive and send via queues.\n" " 2: Receive via scheduler, send via queues.\n"
Add odp_term_global and comment that it will never be called. https://bugs.linaro.org/show_bug.cgi?id=1706 Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> --- example/packet/odp_pktio.c | 56 ++++++++++++++++++++++++++++++++++------------ 1 file changed, 42 insertions(+), 14 deletions(-)