Re: [ovs-dev] [PATCH v2 3/3] netdev-dpdk: Autofill lcore coremask if absent

2016-01-12 Thread Aaron Conole
"Traynor, Kevin"  writes:
>> -Original Message-
>> From: Aaron Conole [mailto:acon...@redhat.com]
>> Sent: Monday, January 4, 2016 9:47 PM
>> To: dev@openvswitch.org; Flavio Leitner; Traynor, Kevin
>> Subject: [PATCH v2 3/3] netdev-dpdk: Autofill lcore coremask if absent
>> 
>> The user has control over the DPDK internal lcore coremask, but this
>> parameter can be autofilled with a bit more intelligence. If the user
>> does not fill this parameter in, we use the lowest set bit in the
>> current task CPU affinity. Otherwise, we will reassign the current
>> thread to the specified lcore mask, in addition to the dpdk lcore
>> threads.
>> 
>> Signed-off-by: Aaron Conole 
>> Cc: Kevin Traynor 
>> ---
>> v2:
>> * Fix a conditional branch coding standard issue
>> * When lcore coremask is set, do not reset the affinities as
>>   suggested by Kevin Traynor
>> 
>>  lib/netdev-dpdk.c | 58 -
>> --
>>  1 file changed, 47 insertions(+), 11 deletions(-)
>> 
>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>> index 2ce9f71..75f40ff 100644
>> --- a/lib/netdev-dpdk.c
>> +++ b/lib/netdev-dpdk.c
>> @@ -65,6 +65,8 @@ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5,
>> 20);
>>  #define OVS_CACHE_LINE_SIZE CACHE_LINE_SIZE
>>  #define OVS_VPORT_DPDK "ovs_dpdk"
>> 
>> +#define MAX_BUFSIZ 256
>> +
>>  /*
>>   * need to reserve tons of extra space in the mbufs so we can align the
>>   * DMA addresses to 4KB.
>> @@ -2192,7 +2194,8 @@ grow_argv(char ***argv, size_t cur_siz, size_t grow_by)
>>  }
>> 
>>  static int
>> -get_dpdk_args(const struct ovsrec_open_vswitch *ovs_cfg, char ***argv)
>> +get_dpdk_args(const struct ovsrec_open_vswitch *ovs_cfg, char ***argv,
>> +  int argc)
>>  {
>>  struct dpdk_options_map {
>>  const char *ovs_configuration;
>> @@ -2200,14 +2203,14 @@ get_dpdk_args(const struct ovsrec_open_vswitch
>> *ovs_cfg, char ***argv)
>>  bool default_enabled;
>>  const char *default_value;
>>  } opts[] = {
>> -{"dpdk-lcore-mask", "-c", true, "0x1"},
>> +{"dpdk-lcore-mask", "-c", false, NULL},
>>  /* XXX: DPDK 2.2.0 support, the true should become false for -n */
>>  {"dpdk-mem-channels", "-n", true, "4"},
>>  {"dpdk-alloc-mem", "-m", false, NULL},
>>  {"dpdk-socket-mem", "--socket-mem", true, "1024,0"},
>>  {"dpdk-hugepage-dir", "--huge-dir", false, NULL},
>>  };
>> -int i, ret = 1;
>> +int i, ret = argc;
>> 
>>  for(i = 0; i < (sizeof(opts) / sizeof(opts[0])); ++i) {
>>  const char *lookup = smap_get(&ovs_cfg->other_config,
>> @@ -2250,7 +2253,8 @@ __dpdk_init(const struct ovsrec_open_vswitch *ovs_cfg)
>>  {
>>  char **argv = NULL;
>>  int result;
>> -int argc;
>> +int argc = 0, argc_tmp;
>> +bool auto_determine = true;
>>  int err;
>>  cpu_set_t cpuset;
>> 
>> @@ -2279,12 +2283,41 @@ __dpdk_init(const struct ovsrec_open_vswitch
>> *ovs_cfg)
>>  ovs_abort(0, "Thread getaffinity error %d.", err);
>>  }
>> 
>> -argv = grow_argv(&argv, 0, 1);
>> +argv = grow_argv(&argv, argc, argc+1);
>
> argc and 1 are added in grow_argv(), so I think it should be 
> 'argv = grow_argv(&argv, argc, 1);'. Similar for other grow_argv()
> calls

Good catch, I completely missed it. Yes this is an over-allocation.

Thanks for the review, Kevin!
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev


Re: [ovs-dev] [PATCH v2 3/3] netdev-dpdk: Autofill lcore coremask if absent

2016-01-12 Thread Traynor, Kevin

> -Original Message-
> From: Aaron Conole [mailto:acon...@redhat.com]
> Sent: Monday, January 4, 2016 9:47 PM
> To: dev@openvswitch.org; Flavio Leitner; Traynor, Kevin
> Subject: [PATCH v2 3/3] netdev-dpdk: Autofill lcore coremask if absent
> 
> The user has control over the DPDK internal lcore coremask, but this
> parameter can be autofilled with a bit more intelligence. If the user
> does not fill this parameter in, we use the lowest set bit in the
> current task CPU affinity. Otherwise, we will reassign the current
> thread to the specified lcore mask, in addition to the dpdk lcore
> threads.
> 
> Signed-off-by: Aaron Conole 
> Cc: Kevin Traynor 
> ---
> v2:
> * Fix a conditional branch coding standard issue
> * When lcore coremask is set, do not reset the affinities as
>   suggested by Kevin Traynor
> 
>  lib/netdev-dpdk.c | 58 -
> --
>  1 file changed, 47 insertions(+), 11 deletions(-)
> 
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 2ce9f71..75f40ff 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -65,6 +65,8 @@ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5,
> 20);
>  #define OVS_CACHE_LINE_SIZE CACHE_LINE_SIZE
>  #define OVS_VPORT_DPDK "ovs_dpdk"
> 
> +#define MAX_BUFSIZ 256
> +
>  /*
>   * need to reserve tons of extra space in the mbufs so we can align the
>   * DMA addresses to 4KB.
> @@ -2192,7 +2194,8 @@ grow_argv(char ***argv, size_t cur_siz, size_t grow_by)
>  }
> 
>  static int
> -get_dpdk_args(const struct ovsrec_open_vswitch *ovs_cfg, char ***argv)
> +get_dpdk_args(const struct ovsrec_open_vswitch *ovs_cfg, char ***argv,
> +  int argc)
>  {
>  struct dpdk_options_map {
>  const char *ovs_configuration;
> @@ -2200,14 +2203,14 @@ get_dpdk_args(const struct ovsrec_open_vswitch
> *ovs_cfg, char ***argv)
>  bool default_enabled;
>  const char *default_value;
>  } opts[] = {
> -{"dpdk-lcore-mask", "-c", true, "0x1"},
> +{"dpdk-lcore-mask", "-c", false, NULL},
>  /* XXX: DPDK 2.2.0 support, the true should become false for -n */
>  {"dpdk-mem-channels", "-n", true, "4"},
>  {"dpdk-alloc-mem", "-m", false, NULL},
>  {"dpdk-socket-mem", "--socket-mem", true, "1024,0"},
>  {"dpdk-hugepage-dir", "--huge-dir", false, NULL},
>  };
> -int i, ret = 1;
> +int i, ret = argc;
> 
>  for(i = 0; i < (sizeof(opts) / sizeof(opts[0])); ++i) {
>  const char *lookup = smap_get(&ovs_cfg->other_config,
> @@ -2250,7 +2253,8 @@ __dpdk_init(const struct ovsrec_open_vswitch *ovs_cfg)
>  {
>  char **argv = NULL;
>  int result;
> -int argc;
> +int argc = 0, argc_tmp;
> +bool auto_determine = true;
>  int err;
>  cpu_set_t cpuset;
> 
> @@ -2279,12 +2283,41 @@ __dpdk_init(const struct ovsrec_open_vswitch
> *ovs_cfg)
>  ovs_abort(0, "Thread getaffinity error %d.", err);
>  }
> 
> -argv = grow_argv(&argv, 0, 1);
> +argv = grow_argv(&argv, argc, argc+1);

argc and 1 are added in grow_argv(), so I think it should be 
'argv = grow_argv(&argv, argc, 1);'. Similar for other grow_argv() calls

>  if (!argv) {
>  ovs_abort(0, "Unable to allocate an initial argv.");
>  }
> -argv[0] = strdup("ovs"); /* TODO use prctl to get process name */
> -argc = get_dpdk_args(ovs_cfg, &argv);
> +argv[argc++] = strdup("ovs"); /* TODO use prctl to get process name */
> +
> +argc_tmp = get_dpdk_args(ovs_cfg, &argv, argc);
> +
> +while(argc_tmp != argc) {
> +if (!strcmp("-c", argv[argc++])) {
> +auto_determine = false;
> +break;
> +}
> +}
> +
> +/**
> + * NOTE: This is an unsophisticated mechanism for determining the DPDK
> + * lcore for the DPDK Master.
> + */
> +if (auto_determine) {
> +int i;
> +for (i = 0; i < CPU_SETSIZE; i++) {
> +if (CPU_ISSET(i, &cpuset)) {
> +char buf[MAX_BUFSIZ];
> +snprintf(buf, MAX_BUFSIZ, "0x%08llX", (1ULL< +argv = grow_argv(&argv, argc, argc+2);
> +if (!argv) {
> +ovs_abort(0, "Unable to grow argv for coremask");
> +}
> +argv[argc++] = strdup("-c");
> +argv[argc++] = strdup(buf);
> +i = CPU_SETSIZE;
> +}
> +}
> +}
> 
>  argv = grow_argv(&argv, argc, argc+1);
>  if (!argv) {
> @@ -2300,10 +2333,13 @@ __dpdk_init(const struct ovsrec_open_vswitch
> *ovs_cfg)
>  ovs_abort(result, "Cannot init EAL");
>  }
> 
> -/* Set the main thread affinity back to pre rte_eal_init() value */
> -err = pthread_setaffinity_np(pthread_self(), sizeof(cpu_set_t),
> &cpuset);
> -if (err) {
> -ovs_abort(0, "Thread getaffinity error %d.", err);
> +if (auto_determine) {
> +/* Set the main thread affinity back to pre rte_eal_init() valu