[PATCHv2,1/2] example: packet: add odp_term_global

Message ID 1461168154-6285-2-git-send-email-maxim.uvarov@linaro.org
State New
Headers show

Commit Message

Maxim Uvarov April 20, 2016, 4:02 p.m.
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(-)

Comments

Elo, Matias (Nokia - FI/Espoo) April 25, 2016, 10:26 a.m. | #1
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
Maxim Uvarov April 25, 2016, 10:41 a.m. | #2
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

Patch

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"