| Message ID | 1451943994-27936-4-git-send-email-aconole@redhat.com |
|---|---|
| State | Not Applicable |
| Headers | show |
> -----Original Message----- > From: Aaron Conole [mailto:aconole@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 <aconole@redhat.com> > Cc: Kevin Traynor <kevin.traynor@intel.com> > --- > 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<<i)); > + 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() value */ > + err = pthread_setaffinity_np(pthread_self(), sizeof(cpu_set_t), > + &cpuset); > + if (err) { > + ovs_abort(0, "Thread getaffinity error %d.", err); > + } > } > > dpdk_argv = argv; > -- > 2.6.1.133.gf5b6079
"Traynor, Kevin" <kevin.traynor@intel.com> writes: >> -----Original Message----- >> From: Aaron Conole [mailto:aconole@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 <aconole@redhat.com> >> Cc: Kevin Traynor <kevin.traynor@intel.com> >> --- >> 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!
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); 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<<i)); + 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() value */ + err = pthread_setaffinity_np(pthread_self(), sizeof(cpu_set_t), + &cpuset); + if (err) { + ovs_abort(0, "Thread getaffinity error %d.", err); + } } dpdk_argv = argv;
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 <aconole@redhat.com> Cc: Kevin Traynor <kevin.traynor@intel.com> --- 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(-)