| Message ID | 20170109032152.73022-1-diproiettod@vmware.com |
|---|---|
| State | Superseded |
| Headers | show |
hi Daniele, I reviewed this patch. One question to ask: should we check the hugepage mm before calling the rte_eal_init()? improvement on next version? Thanks. Nick > On Jan 9, 2017, at 11:21 AM, Daniele Di Proietto <diproiettod@vmware.com> wrote: > > With this commit, we allow the user to set other_config:dpdk-init=true > after the process is started. This makes it easier to start Open > vSwitch with DPDK using standard init scripts without restarting the > service. > > This is still far from ideal, because initializing DPDK might still > abort the process (e.g. if there not enough memory), so the user must > check the status of the process after setting dpdk-init to true. > > Nonetheless, I think this is an improvement, because it doesn't require > restarting the whole unit. > > CC: Aaron Conole <aconole@redhat.com <mailto:aconole@redhat.com>> > Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com <mailto:diproiettod@vmware.com>> > --- > v1->v2: No change, first non-RFC post. > --- > lib/dpdk-stub.c | 8 ++++---- > lib/dpdk.c | 30 ++++++++++++++++++++---------- > tests/ofproto-macros.at <http://ofproto-macros.at/> | 2 +- > 3 files changed, 25 insertions(+), 15 deletions(-)
Daniele Di Proietto <diproiettod@vmware.com> writes: > With this commit, we allow the user to set other_config:dpdk-init=true > after the process is started. This makes it easier to start Open > vSwitch with DPDK using standard init scripts without restarting the > service. > > This is still far from ideal, because initializing DPDK might still > abort the process (e.g. if there not enough memory), so the user must > check the status of the process after setting dpdk-init to true. > > Nonetheless, I think this is an improvement, because it doesn't require > restarting the whole unit. > > CC: Aaron Conole <aconole@redhat.com> > Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com> > --- > v1->v2: No change, first non-RFC post. > --- Looks good - just one minor detail below > lib/dpdk-stub.c | 8 ++++---- > lib/dpdk.c | 30 ++++++++++++++++++++---------- > tests/ofproto-macros.at | 2 +- > 3 files changed, 25 insertions(+), 15 deletions(-) > > diff --git a/lib/dpdk-stub.c b/lib/dpdk-stub.c > index bd981bb90..daef7291f 100644 > --- a/lib/dpdk-stub.c > +++ b/lib/dpdk-stub.c > @@ -27,13 +27,13 @@ VLOG_DEFINE_THIS_MODULE(dpdk); > void > dpdk_init(const struct smap *ovs_other_config) > { > - static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; > + if (smap_get_bool(ovs_other_config, "dpdk-init", false)) { > + static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; > > - if (ovsthread_once_start(&once)) { > - if (smap_get_bool(ovs_other_config, "dpdk-init", false)) { > + if (ovsthread_once_start(&once)) { > VLOG_ERR("DPDK not supported in this copy of Open vSwitch."); > + ovsthread_once_done(&once); > } > - ovsthread_once_done(&once); > } > } > > diff --git a/lib/dpdk.c b/lib/dpdk.c > index ee4360b22..008c6c06d 100644 > --- a/lib/dpdk.c > +++ b/lib/dpdk.c > @@ -273,12 +273,6 @@ dpdk_init__(const struct smap *ovs_other_config) > cpu_set_t cpuset; > char *sock_dir_subcomponent; > > - if (!smap_get_bool(ovs_other_config, "dpdk-init", false)) { > - VLOG_INFO("DPDK Disabled - to change this requires a restart.\n"); > - return; > - } > - > - VLOG_INFO("DPDK Enabled, initializing"); > if (process_vhost_flags("vhost-sock-dir", xstrdup(ovs_rundir()), > NAME_MAX, ovs_other_config, > &sock_dir_subcomponent)) { > @@ -413,11 +407,27 @@ dpdk_init__(const struct smap *ovs_other_config) > void > dpdk_init(const struct smap *ovs_other_config) > { > - static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; > + static bool enabled = false; This doesn't appear to be used, apart from the first test of the following conditional (where it will always pass to the second). Did I miss something? > + if (enabled || !ovs_other_config) { > + return; > + } > + > + if (smap_get_bool(ovs_other_config, "dpdk-init", false)) { > + static struct ovsthread_once once_enable = OVSTHREAD_ONCE_INITIALIZER; > > - if (ovs_other_config && ovsthread_once_start(&once)) { > - dpdk_init__(ovs_other_config); > - ovsthread_once_done(&once); > + if (ovsthread_once_start(&once_enable)) { > + VLOG_INFO("DPDK Enabled - initializing..."); > + dpdk_init__(ovs_other_config); > + VLOG_INFO("DPDK Enabled - initialized"); > + ovsthread_once_done(&once_enable); > + } > + } else { > + static struct ovsthread_once once_disable = OVSTHREAD_ONCE_INITIALIZER; > + if (ovsthread_once_start(&once_disable)) { > + VLOG_INFO("DPDK Disabled - Use other_config:dpdk-init to enable"); > + ovsthread_once_done(&once_disable); > + } > } > } > > diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at > index 5477777b8..faff5b0a8 100644 > --- a/tests/ofproto-macros.at > +++ b/tests/ofproto-macros.at > @@ -331,7 +331,7 @@ m4_define([_OVS_VSWITCHD_START], > /ofproto|INFO|using datapath ID/d > /netdev_linux|INFO|.*device has unknown hardware address family/d > /ofproto|INFO|datapath ID changed to fedcba9876543210/d > -/dpdk|INFO|DPDK Disabled - to change this requires a restart./d']]) > +/dpdk|INFO|DPDK Disabled - Use other_config:dpdk-init to enable/d']]) > ]) > > # OVS_VSWITCHD_START([vsctl-args], [vsctl-output], [=override],
nickcooper-zhangtonghao <nic@opencloud.tech> writes: > hi Daniele, > I reviewed this patch. One question to ask: should we check the > hugepage mm before calling the rte_eal_init()? improvement on next version? Are you concerned for the possible rte_panic() call which could happen if the hugepage configuration is not set correctly? > Thanks. > Nick > >> On Jan 9, 2017, at 11:21 AM, Daniele Di Proietto >> <diproiettod@vmware.com> wrote: >> >> With this commit, we allow the user to set other_config:dpdk-init=true >> after the process is started. This makes it easier to start Open >> vSwitch with DPDK using standard init scripts without restarting the >> service. >> >> This is still far from ideal, because initializing DPDK might still >> abort the process (e.g. if there not enough memory), so the user must >> check the status of the process after setting dpdk-init to true. >> >> Nonetheless, I think this is an improvement, because it doesn't require >> restarting the whole unit. >> >> CC: Aaron Conole <aconole@redhat.com <mailto:aconole@redhat.com>> >> Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com >> <mailto:diproiettod@vmware.com>> >> --- >> v1->v2: No change, first non-RFC post. >> --- >> lib/dpdk-stub.c | 8 ++++---- >> lib/dpdk.c | 30 ++++++++++++++++++++---------- >> tests/ofproto-macros.at <http://ofproto-macros.at/> | 2 +- >> 3 files changed, 25 insertions(+), 15 deletions(-) > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Yes, but this patch looks good to me. > On Jan 9, 2017, at 11:35 PM, Aaron Conole <aconole@redhat.com> wrote: > > nickcooper-zhangtonghao <nic@opencloud.tech <mailto:nic@opencloud.tech>> writes: > >> hi Daniele, >> I reviewed this patch. One question to ask: should we check the >> hugepage mm before calling the rte_eal_init()? improvement on next version? > > Are you concerned for the possible rte_panic() call which could happen > if the hugepage configuration is not set correctly?
On 09/01/2017 07:14, "Aaron Conole" <aconole@redhat.com> wrote: >Daniele Di Proietto <diproiettod@vmware.com> writes: > >> With this commit, we allow the user to set other_config:dpdk-init=true >> after the process is started. This makes it easier to start Open >> vSwitch with DPDK using standard init scripts without restarting the >> service. >> >> This is still far from ideal, because initializing DPDK might still >> abort the process (e.g. if there not enough memory), so the user must >> check the status of the process after setting dpdk-init to true. >> >> Nonetheless, I think this is an improvement, because it doesn't require >> restarting the whole unit. >> >> CC: Aaron Conole <aconole@redhat.com> >> Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com> >> --- >> v1->v2: No change, first non-RFC post. >> --- > >Looks good - just one minor detail below > >> lib/dpdk-stub.c | 8 ++++---- >> lib/dpdk.c | 30 ++++++++++++++++++++---------- >> tests/ofproto-macros.at | 2 +- >> 3 files changed, 25 insertions(+), 15 deletions(-) >> >> diff --git a/lib/dpdk-stub.c b/lib/dpdk-stub.c >> index bd981bb90..daef7291f 100644 >> --- a/lib/dpdk-stub.c >> +++ b/lib/dpdk-stub.c >> @@ -27,13 +27,13 @@ VLOG_DEFINE_THIS_MODULE(dpdk); >> void >> dpdk_init(const struct smap *ovs_other_config) >> { >> - static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; >> + if (smap_get_bool(ovs_other_config, "dpdk-init", false)) { >> + static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; >> >> - if (ovsthread_once_start(&once)) { >> - if (smap_get_bool(ovs_other_config, "dpdk-init", false)) { >> + if (ovsthread_once_start(&once)) { >> VLOG_ERR("DPDK not supported in this copy of Open vSwitch."); >> + ovsthread_once_done(&once); >> } >> - ovsthread_once_done(&once); >> } >> } >> >> diff --git a/lib/dpdk.c b/lib/dpdk.c >> index ee4360b22..008c6c06d 100644 >> --- a/lib/dpdk.c >> +++ b/lib/dpdk.c >> @@ -273,12 +273,6 @@ dpdk_init__(const struct smap *ovs_other_config) >> cpu_set_t cpuset; >> char *sock_dir_subcomponent; >> >> - if (!smap_get_bool(ovs_other_config, "dpdk-init", false)) { >> - VLOG_INFO("DPDK Disabled - to change this requires a restart.\n"); >> - return; >> - } >> - >> - VLOG_INFO("DPDK Enabled, initializing"); >> if (process_vhost_flags("vhost-sock-dir", xstrdup(ovs_rundir()), >> NAME_MAX, ovs_other_config, >> &sock_dir_subcomponent)) { >> @@ -413,11 +407,27 @@ dpdk_init__(const struct smap *ovs_other_config) >> void >> dpdk_init(const struct smap *ovs_other_config) >> { >> - static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; >> + static bool enabled = false; > >This doesn't appear to be used, apart from the first test of the >following conditional (where it will always pass to the second). Did I >miss something? Oops, it should be used. I need to set it to true after calling dpdk_init__(), otherwise the following scenario might happen: 1) other_config:dpdk-init is set to "true" 2) vswitchd is started and dpdk_init__() is called 3) other_config:dpdk-init is set to "false" 4) The log message "DPDK Disabled - Use other_config:dpdk-init to enable" is printed, giving the illusion that DPDK was disabled. I'll add 'enable=true' in the next version. Thanks, Daniele > >> + if (enabled || !ovs_other_config) { >> + return; >> + } >> + >> + if (smap_get_bool(ovs_other_config, "dpdk-init", false)) { >> + static struct ovsthread_once once_enable = OVSTHREAD_ONCE_INITIALIZER; >> >> - if (ovs_other_config && ovsthread_once_start(&once)) { >> - dpdk_init__(ovs_other_config); >> - ovsthread_once_done(&once); >> + if (ovsthread_once_start(&once_enable)) { >> + VLOG_INFO("DPDK Enabled - initializing..."); >> + dpdk_init__(ovs_other_config); >> + VLOG_INFO("DPDK Enabled - initialized"); >> + ovsthread_once_done(&once_enable); >> + } >> + } else { >> + static struct ovsthread_once once_disable = OVSTHREAD_ONCE_INITIALIZER; >> + if (ovsthread_once_start(&once_disable)) { >> + VLOG_INFO("DPDK Disabled - Use other_config:dpdk-init to enable"); >> + ovsthread_once_done(&once_disable); >> + } >> } >> } >> >> diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at >> index 5477777b8..faff5b0a8 100644 >> --- a/tests/ofproto-macros.at >> +++ b/tests/ofproto-macros.at >> @@ -331,7 +331,7 @@ m4_define([_OVS_VSWITCHD_START], >> /ofproto|INFO|using datapath ID/d >> /netdev_linux|INFO|.*device has unknown hardware address family/d >> /ofproto|INFO|datapath ID changed to fedcba9876543210/d >> -/dpdk|INFO|DPDK Disabled - to change this requires a restart./d']]) >> +/dpdk|INFO|DPDK Disabled - Use other_config:dpdk-init to enable/d']]) >> ]) >> >> # OVS_VSWITCHD_START([vsctl-args], [vsctl-output], [=override],
On 09/01/2017 03:49, "nickcooper-zhangtonghao" <nic@opencloud.tech> wrote: > > > >hi Daniele, >I reviewed this patch. One question to ask: should we check the >hugepage mm before calling the rte_eal_init()? improvement on next version? How do you suggest to check for hugepage before calling rte_eal_init()? I think everybody agrees that in the long term we need to avoid aborting if the initialization fails, but most of that work need to happen in dpdk library. If there's a simple check we could do here, I'm fine with including that, if it's something more complicated and needs to be a separate patch, we should probably defer it, since we're on feature freeze now. Thanks, Daniele > > > >Thanks. >Nick > > > >On Jan 9, 2017, at 11:21 AM, Daniele Di Proietto <diproiettod@vmware.com> wrote: > >With > this commit, we allow the user to set other_config:dpdk-init=true >after > the process is started. This makes it easier to start Open >vSwitch > with DPDK using standard init scripts without restarting the >service. > >This > is still far from ideal, because initializing DPDK might still >abort > the process (e.g. if there not enough memory), so the user must >check > the status of the process after setting dpdk-init to true. > >Nonetheless, > I think this is an improvement, because it doesn't require >restarting > the whole unit. > >CC: > Aaron Conole <aconole@redhat.com> >Signed-off-by: > Daniele Di Proietto <diproiettod@vmware.com> >--- >v1->v2: > No change, first non-RFC post. >--- >lib/dpdk-stub.c > | 8 ++++---- >lib/dpdk.c > | 30 ++++++++++++++++++++---------- >tests/ofproto-macros.at <http://ofproto-macros.at/> | > 2 +- >3 > files changed, 25 insertions(+), 15 deletions(-) > > > > >
Thank you for explaining this to me. I got it. > On Jan 10, 2017, at 10:09 AM, Daniele Di Proietto <diproiettod@vmware.com> wrote: > >> >> >> >> hi Daniele, >> I reviewed this patch. One question to ask: should we check the >> hugepage mm before calling the rte_eal_init()? improvement on next version? > > How do you suggest to check for hugepage before calling rte_eal_init()? > > I think everybody agrees that in the long term we need to avoid aborting if the initialization fails, but most of that work need to happen in dpdk library. > > If there's a simple check we could do here, I'm fine with including that, if it's something more complicated and needs to be a separate patch, we should probably defer it, since we're on feature freeze now. > > Thanks, > > Daniele
diff --git a/lib/dpdk-stub.c b/lib/dpdk-stub.c index bd981bb90..daef7291f 100644 --- a/lib/dpdk-stub.c +++ b/lib/dpdk-stub.c @@ -27,13 +27,13 @@ VLOG_DEFINE_THIS_MODULE(dpdk); void dpdk_init(const struct smap *ovs_other_config) { - static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; + if (smap_get_bool(ovs_other_config, "dpdk-init", false)) { + static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; - if (ovsthread_once_start(&once)) { - if (smap_get_bool(ovs_other_config, "dpdk-init", false)) { + if (ovsthread_once_start(&once)) { VLOG_ERR("DPDK not supported in this copy of Open vSwitch."); + ovsthread_once_done(&once); } - ovsthread_once_done(&once); } } diff --git a/lib/dpdk.c b/lib/dpdk.c index ee4360b22..008c6c06d 100644 --- a/lib/dpdk.c +++ b/lib/dpdk.c @@ -273,12 +273,6 @@ dpdk_init__(const struct smap *ovs_other_config) cpu_set_t cpuset; char *sock_dir_subcomponent; - if (!smap_get_bool(ovs_other_config, "dpdk-init", false)) { - VLOG_INFO("DPDK Disabled - to change this requires a restart.\n"); - return; - } - - VLOG_INFO("DPDK Enabled, initializing"); if (process_vhost_flags("vhost-sock-dir", xstrdup(ovs_rundir()), NAME_MAX, ovs_other_config, &sock_dir_subcomponent)) { @@ -413,11 +407,27 @@ dpdk_init__(const struct smap *ovs_other_config) void dpdk_init(const struct smap *ovs_other_config) { - static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; + static bool enabled = false; + + if (enabled || !ovs_other_config) { + return; + } + + if (smap_get_bool(ovs_other_config, "dpdk-init", false)) { + static struct ovsthread_once once_enable = OVSTHREAD_ONCE_INITIALIZER; - if (ovs_other_config && ovsthread_once_start(&once)) { - dpdk_init__(ovs_other_config); - ovsthread_once_done(&once); + if (ovsthread_once_start(&once_enable)) { + VLOG_INFO("DPDK Enabled - initializing..."); + dpdk_init__(ovs_other_config); + VLOG_INFO("DPDK Enabled - initialized"); + ovsthread_once_done(&once_enable); + } + } else { + static struct ovsthread_once once_disable = OVSTHREAD_ONCE_INITIALIZER; + if (ovsthread_once_start(&once_disable)) { + VLOG_INFO("DPDK Disabled - Use other_config:dpdk-init to enable"); + ovsthread_once_done(&once_disable); + } } } diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at index 5477777b8..faff5b0a8 100644 --- a/tests/ofproto-macros.at +++ b/tests/ofproto-macros.at @@ -331,7 +331,7 @@ m4_define([_OVS_VSWITCHD_START], /ofproto|INFO|using datapath ID/d /netdev_linux|INFO|.*device has unknown hardware address family/d /ofproto|INFO|datapath ID changed to fedcba9876543210/d -/dpdk|INFO|DPDK Disabled - to change this requires a restart./d']]) +/dpdk|INFO|DPDK Disabled - Use other_config:dpdk-init to enable/d']]) ]) # OVS_VSWITCHD_START([vsctl-args], [vsctl-output], [=override],
With this commit, we allow the user to set other_config:dpdk-init=true after the process is started. This makes it easier to start Open vSwitch with DPDK using standard init scripts without restarting the service. This is still far from ideal, because initializing DPDK might still abort the process (e.g. if there not enough memory), so the user must check the status of the process after setting dpdk-init to true. Nonetheless, I think this is an improvement, because it doesn't require restarting the whole unit. CC: Aaron Conole <aconole@redhat.com> Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com> --- v1->v2: No change, first non-RFC post. --- lib/dpdk-stub.c | 8 ++++---- lib/dpdk.c | 30 ++++++++++++++++++++---------- tests/ofproto-macros.at | 2 +- 3 files changed, 25 insertions(+), 15 deletions(-)