| Message ID | 1482665989-791-4-git-send-email-paulb@mellanox.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On 25 December 2016 at 03:39, Paul Blakey <paulb@mellanox.com> wrote: > Add a new configuration option - hw-offload that enables netdev > flow api. Enabling this option will allow offloading flows > using netdev implementation instead of the kernel datapath. > This configuration option defaults to false - disabled. > > Signed-off-by: Paul Blakey <paulb@mellanox.com> > Reviewed-by: Roi Dayan <roid@mellanox.com> > --- > lib/netdev.c | 18 ++++++++++++++++++ > lib/netdev.h | 2 ++ > vswitchd/bridge.c | 2 ++ > vswitchd/vswitch.xml | 11 +++++++++++ > 4 files changed, 33 insertions(+) > > diff --git a/lib/netdev.c b/lib/netdev.c > index 3ac3c48..b289166 100644 > --- a/lib/netdev.c > +++ b/lib/netdev.c > @@ -2071,7 +2071,25 @@ netdev_init_flow_api(struct netdev *netdev) > { > const struct netdev_class *class = netdev->netdev_class; > > + if (!netdev_flow_api_enabled) { > + return EOPNOTSUPP; > + } > + > return (class->init_flow_api > ? class->init_flow_api(netdev) > : EOPNOTSUPP); > } > + > +bool netdev_flow_api_enabled = false; > + > +void > +netdev_set_flow_api_enabled(bool enabled) > +{ > + static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; > + > + if (ovsthread_once_start(&once)) { > + netdev_flow_api_enabled = enabled; > + VLOG_INFO("netdev: Flow API %s", enabled ? "Enabled" : "Disabled"); > + ovsthread_once_done(&once); > + } > +} Requiring restart to apply this option seems a bit arbitrary, why not allow it to be changed at runtime?
On 05/01/2017 03:26, Joe Stringer wrote: > On 25 December 2016 at 03:39, Paul Blakey <paulb@mellanox.com> wrote: >> Add a new configuration option - hw-offload that enables netdev >> flow api. Enabling this option will allow offloading flows >> using netdev implementation instead of the kernel datapath. >> This configuration option defaults to false - disabled. >> >> Signed-off-by: Paul Blakey <paulb@mellanox.com> >> Reviewed-by: Roi Dayan <roid@mellanox.com> >> --- >> lib/netdev.c | 18 ++++++++++++++++++ >> lib/netdev.h | 2 ++ >> vswitchd/bridge.c | 2 ++ >> vswitchd/vswitch.xml | 11 +++++++++++ >> 4 files changed, 33 insertions(+) >> >> diff --git a/lib/netdev.c b/lib/netdev.c >> index 3ac3c48..b289166 100644 >> --- a/lib/netdev.c >> +++ b/lib/netdev.c >> @@ -2071,7 +2071,25 @@ netdev_init_flow_api(struct netdev *netdev) >> { >> const struct netdev_class *class = netdev->netdev_class; >> >> + if (!netdev_flow_api_enabled) { >> + return EOPNOTSUPP; >> + } >> + >> return (class->init_flow_api >> ? class->init_flow_api(netdev) >> : EOPNOTSUPP); >> } >> + >> +bool netdev_flow_api_enabled = false; >> + >> +void >> +netdev_set_flow_api_enabled(bool enabled) >> +{ >> + static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; >> + >> + if (ovsthread_once_start(&once)) { >> + netdev_flow_api_enabled = enabled; >> + VLOG_INFO("netdev: Flow API %s", enabled ? "Enabled" : "Disabled"); >> + ovsthread_once_done(&once); >> + } >> +} > > Requiring restart to apply this option seems a bit arbitrary, why not > allow it to be changed at runtime? > Because it isn't something that is supposed to change often, It might cause problems if its enabled later on with different (netdev) implementations. We can try and test changing it to runtime at a later time.
On 22 January 2017 at 08:13, Paul Blakey <paulb@mellanox.com> wrote: > > > On 05/01/2017 03:26, Joe Stringer wrote: >> >> On 25 December 2016 at 03:39, Paul Blakey <paulb@mellanox.com> wrote: >>> >>> Add a new configuration option - hw-offload that enables netdev >>> flow api. Enabling this option will allow offloading flows >>> using netdev implementation instead of the kernel datapath. >>> This configuration option defaults to false - disabled. >>> >>> Signed-off-by: Paul Blakey <paulb@mellanox.com> >>> Reviewed-by: Roi Dayan <roid@mellanox.com> >>> --- >>> lib/netdev.c | 18 ++++++++++++++++++ >>> lib/netdev.h | 2 ++ >>> vswitchd/bridge.c | 2 ++ >>> vswitchd/vswitch.xml | 11 +++++++++++ >>> 4 files changed, 33 insertions(+) >>> >>> diff --git a/lib/netdev.c b/lib/netdev.c >>> index 3ac3c48..b289166 100644 >>> --- a/lib/netdev.c >>> +++ b/lib/netdev.c >>> @@ -2071,7 +2071,25 @@ netdev_init_flow_api(struct netdev *netdev) >>> { >>> const struct netdev_class *class = netdev->netdev_class; >>> >>> + if (!netdev_flow_api_enabled) { >>> + return EOPNOTSUPP; >>> + } >>> + >>> return (class->init_flow_api >>> ? class->init_flow_api(netdev) >>> : EOPNOTSUPP); >>> } >>> + >>> +bool netdev_flow_api_enabled = false; >>> + >>> +void >>> +netdev_set_flow_api_enabled(bool enabled) >>> +{ >>> + static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; >>> + >>> + if (ovsthread_once_start(&once)) { >>> + netdev_flow_api_enabled = enabled; >>> + VLOG_INFO("netdev: Flow API %s", enabled ? "Enabled" : >>> "Disabled"); >>> + ovsthread_once_done(&once); >>> + } >>> +} >> >> >> Requiring restart to apply this option seems a bit arbitrary, why not >> allow it to be changed at runtime? >> > > Because it isn't something that is supposed to change often, > It might cause problems if its enabled later on with different (netdev) > implementations. We can try and test changing it to runtime at a later time. Forcing the user to restart OVS to make a database setting take effect is an unusual requirement. Even uncommon configurations that require datapath-level changes (eg, reconfiguring queues) is applied in OVS without requiring the user to intervene and restart. I understand that even DPDK init doesn't require restart any more. Here's another question: Let's say that hardware offloads is enabled and traffic runs. User turns it off, restarts OVS. What happens to the hardware flows? Presumably OVS doesn't manage them any more because hw-offloads is off, but they would continue to forward traffic. If OVS then gets different OpenFlow rules then the forwarding could be wrong.
On 1/23/2017 8:41 PM, Joe Stringer wrote: > On 22 January 2017 at 08:13, Paul Blakey <paulb@mellanox.com> wrote: >> >> On 05/01/2017 03:26, Joe Stringer wrote: >>> On 25 December 2016 at 03:39, Paul Blakey <paulb@mellanox.com> wrote: >>>> Add a new configuration option - hw-offload that enables netdev >>>> flow api. Enabling this option will allow offloading flows >>>> using netdev implementation instead of the kernel datapath. >>>> This configuration option defaults to false - disabled. >>>> >>>> Signed-off-by: Paul Blakey <paulb@mellanox.com> >>>> Reviewed-by: Roi Dayan <roid@mellanox.com> >>>> --- >>>> lib/netdev.c | 18 ++++++++++++++++++ >>>> lib/netdev.h | 2 ++ >>>> vswitchd/bridge.c | 2 ++ >>>> vswitchd/vswitch.xml | 11 +++++++++++ >>>> 4 files changed, 33 insertions(+) >>>> >>>> diff --git a/lib/netdev.c b/lib/netdev.c >>>> index 3ac3c48..b289166 100644 >>>> --- a/lib/netdev.c >>>> +++ b/lib/netdev.c >>>> @@ -2071,7 +2071,25 @@ netdev_init_flow_api(struct netdev *netdev) >>>> { >>>> const struct netdev_class *class = netdev->netdev_class; >>>> >>>> + if (!netdev_flow_api_enabled) { >>>> + return EOPNOTSUPP; >>>> + } >>>> + >>>> return (class->init_flow_api >>>> ? class->init_flow_api(netdev) >>>> : EOPNOTSUPP); >>>> } >>>> + >>>> +bool netdev_flow_api_enabled = false; >>>> + >>>> +void >>>> +netdev_set_flow_api_enabled(bool enabled) >>>> +{ >>>> + static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; >>>> + >>>> + if (ovsthread_once_start(&once)) { >>>> + netdev_flow_api_enabled = enabled; >>>> + VLOG_INFO("netdev: Flow API %s", enabled ? "Enabled" : >>>> "Disabled"); >>>> + ovsthread_once_done(&once); >>>> + } >>>> +} >>> >>> Requiring restart to apply this option seems a bit arbitrary, why not >>> allow it to be changed at runtime? >>> >> Because it isn't something that is supposed to change often, >> It might cause problems if its enabled later on with different (netdev) >> implementations. We can try and test changing it to runtime at a later time. > Forcing the user to restart OVS to make a database setting take effect > is an unusual requirement. Even uncommon configurations that require > datapath-level changes (eg, reconfiguring queues) is applied in OVS > without requiring the user to intervene and restart. I understand that > even DPDK init doesn't require restart any more. > > Here's another question: Let's say that hardware offloads is enabled > and traffic runs. User turns it off, restarts OVS. What happens to the > hardware flows? Presumably OVS doesn't manage them any more because > hw-offloads is off, but they would continue to forward traffic. If OVS > then gets different OpenFlow rules then the forwarding could be wrong. Hi Joe, We understand that forcing the user to restart OVS is not something common we want to do. We can make this option changeable at runtime and flush all the rules when hw-offload changes from true to false. If something will block us we'll let you know and if a future requirement will need this then it can be changed later. To answer your next question then this is also what happens now when the user restarts OVS. All rules are flushed so there shoudn't be HW forwarding rules. Thanks, Roi
On 24 January 2017 at 00:45, Roi Dayan <roid@mellanox.com> wrote: > > > On 1/23/2017 8:41 PM, Joe Stringer wrote: >> >> On 22 January 2017 at 08:13, Paul Blakey <paulb@mellanox.com> wrote: >>> >>> >>> On 05/01/2017 03:26, Joe Stringer wrote: >>>> >>>> On 25 December 2016 at 03:39, Paul Blakey <paulb@mellanox.com> wrote: >>>>> >>>>> Add a new configuration option - hw-offload that enables netdev >>>>> flow api. Enabling this option will allow offloading flows >>>>> using netdev implementation instead of the kernel datapath. >>>>> This configuration option defaults to false - disabled. >>>>> >>>>> Signed-off-by: Paul Blakey <paulb@mellanox.com> >>>>> Reviewed-by: Roi Dayan <roid@mellanox.com> >>>>> --- >>>>> lib/netdev.c | 18 ++++++++++++++++++ >>>>> lib/netdev.h | 2 ++ >>>>> vswitchd/bridge.c | 2 ++ >>>>> vswitchd/vswitch.xml | 11 +++++++++++ >>>>> 4 files changed, 33 insertions(+) >>>>> >>>>> diff --git a/lib/netdev.c b/lib/netdev.c >>>>> index 3ac3c48..b289166 100644 >>>>> --- a/lib/netdev.c >>>>> +++ b/lib/netdev.c >>>>> @@ -2071,7 +2071,25 @@ netdev_init_flow_api(struct netdev *netdev) >>>>> { >>>>> const struct netdev_class *class = netdev->netdev_class; >>>>> >>>>> + if (!netdev_flow_api_enabled) { >>>>> + return EOPNOTSUPP; >>>>> + } >>>>> + >>>>> return (class->init_flow_api >>>>> ? class->init_flow_api(netdev) >>>>> : EOPNOTSUPP); >>>>> } >>>>> + >>>>> +bool netdev_flow_api_enabled = false; >>>>> + >>>>> +void >>>>> +netdev_set_flow_api_enabled(bool enabled) >>>>> +{ >>>>> + static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; >>>>> + >>>>> + if (ovsthread_once_start(&once)) { >>>>> + netdev_flow_api_enabled = enabled; >>>>> + VLOG_INFO("netdev: Flow API %s", enabled ? "Enabled" : >>>>> "Disabled"); >>>>> + ovsthread_once_done(&once); >>>>> + } >>>>> +} >>>> >>>> >>>> Requiring restart to apply this option seems a bit arbitrary, why not >>>> allow it to be changed at runtime? >>>> >>> Because it isn't something that is supposed to change often, >>> It might cause problems if its enabled later on with different (netdev) >>> implementations. We can try and test changing it to runtime at a later >>> time. >> >> Forcing the user to restart OVS to make a database setting take effect >> is an unusual requirement. Even uncommon configurations that require >> datapath-level changes (eg, reconfiguring queues) is applied in OVS >> without requiring the user to intervene and restart. I understand that >> even DPDK init doesn't require restart any more. >> >> Here's another question: Let's say that hardware offloads is enabled >> and traffic runs. User turns it off, restarts OVS. What happens to the >> hardware flows? Presumably OVS doesn't manage them any more because >> hw-offloads is off, but they would continue to forward traffic. If OVS >> then gets different OpenFlow rules then the forwarding could be wrong. > > > Hi Joe, > > We understand that forcing the user to restart OVS is not something common > we want to do. > We can make this option changeable at runtime and flush all the rules when > hw-offload changes from true to false. > If something will block us we'll let you know and if a future requirement > will need this then it can be changed later. > > To answer your next question then this is also what happens now when the > user restarts OVS. All rules are flushed so there shoudn't be HW forwarding > rules. How are they flushed? I don't think ovs-vswitchd flushes flows when it stops. Typically if you restart OVS, then on startup the flow table will be empty so before you get a chance to install your OpenFlow flows again, OVS will be running, dumping datapath flows, and deleting them because they don't match the current OpenFlow table. However, now that you've disabled hw flows, I suspect that OVS won't try to manage them. So the hw flows will continue to forward according to policy prior to ovs-vswitchd shutdown. There is also a way to start OVS where it will wait for you to finish initializing the OpenFlow tables before it starts revalidating the flows (ovs_vsctl set open_vswitch . other_config:flow-restore-wait="true") - see utilities/ovs-ctl.in for how this may be used. These are all only considerations where the datapath has its own life beyond the OVS binary. They don't apply when running with userspace datapath, clearly, because the datapath and ovs-vswitchd are the same in that case.
On 1/24/2017 9:12 PM, Joe Stringer wrote: > On 24 January 2017 at 00:45, Roi Dayan <roid@mellanox.com> wrote: >> >> On 1/23/2017 8:41 PM, Joe Stringer wrote: >>> On 22 January 2017 at 08:13, Paul Blakey <paulb@mellanox.com> wrote: >>>> >>>> On 05/01/2017 03:26, Joe Stringer wrote: >>>>> On 25 December 2016 at 03:39, Paul Blakey <paulb@mellanox.com> wrote: >>>>>> Add a new configuration option - hw-offload that enables netdev >>>>>> flow api. Enabling this option will allow offloading flows >>>>>> using netdev implementation instead of the kernel datapath. >>>>>> This configuration option defaults to false - disabled. >>>>>> >>>>>> Signed-off-by: Paul Blakey <paulb@mellanox.com> >>>>>> Reviewed-by: Roi Dayan <roid@mellanox.com> >>>>>> --- >>>>>> lib/netdev.c | 18 ++++++++++++++++++ >>>>>> lib/netdev.h | 2 ++ >>>>>> vswitchd/bridge.c | 2 ++ >>>>>> vswitchd/vswitch.xml | 11 +++++++++++ >>>>>> 4 files changed, 33 insertions(+) >>>>>> >>>>>> diff --git a/lib/netdev.c b/lib/netdev.c >>>>>> index 3ac3c48..b289166 100644 >>>>>> --- a/lib/netdev.c >>>>>> +++ b/lib/netdev.c >>>>>> @@ -2071,7 +2071,25 @@ netdev_init_flow_api(struct netdev *netdev) >>>>>> { >>>>>> const struct netdev_class *class = netdev->netdev_class; >>>>>> >>>>>> + if (!netdev_flow_api_enabled) { >>>>>> + return EOPNOTSUPP; >>>>>> + } >>>>>> + >>>>>> return (class->init_flow_api >>>>>> ? class->init_flow_api(netdev) >>>>>> : EOPNOTSUPP); >>>>>> } >>>>>> + >>>>>> +bool netdev_flow_api_enabled = false; >>>>>> + >>>>>> +void >>>>>> +netdev_set_flow_api_enabled(bool enabled) >>>>>> +{ >>>>>> + static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; >>>>>> + >>>>>> + if (ovsthread_once_start(&once)) { >>>>>> + netdev_flow_api_enabled = enabled; >>>>>> + VLOG_INFO("netdev: Flow API %s", enabled ? "Enabled" : >>>>>> "Disabled"); >>>>>> + ovsthread_once_done(&once); >>>>>> + } >>>>>> +} >>>>> >>>>> Requiring restart to apply this option seems a bit arbitrary, why not >>>>> allow it to be changed at runtime? >>>>> >>>> Because it isn't something that is supposed to change often, >>>> It might cause problems if its enabled later on with different (netdev) >>>> implementations. We can try and test changing it to runtime at a later >>>> time. >>> Forcing the user to restart OVS to make a database setting take effect >>> is an unusual requirement. Even uncommon configurations that require >>> datapath-level changes (eg, reconfiguring queues) is applied in OVS >>> without requiring the user to intervene and restart. I understand that >>> even DPDK init doesn't require restart any more. >>> >>> Here's another question: Let's say that hardware offloads is enabled >>> and traffic runs. User turns it off, restarts OVS. What happens to the >>> hardware flows? Presumably OVS doesn't manage them any more because >>> hw-offloads is off, but they would continue to forward traffic. If OVS >>> then gets different OpenFlow rules then the forwarding could be wrong. >> >> Hi Joe, >> >> We understand that forcing the user to restart OVS is not something common >> we want to do. >> We can make this option changeable at runtime and flush all the rules when >> hw-offload changes from true to false. >> If something will block us we'll let you know and if a future requirement >> will need this then it can be changed later. >> >> To answer your next question then this is also what happens now when the >> user restarts OVS. All rules are flushed so there shoudn't be HW forwarding >> rules. > How are they flushed? > > I don't think ovs-vswitchd flushes flows when it stops. > > Typically if you restart OVS, then on startup the flow table will be > empty so before you get a chance to install your OpenFlow flows again, > OVS will be running, dumping datapath flows, and deleting them because > they don't match the current OpenFlow table. However, now that you've > disabled hw flows, I suspect that OVS won't try to manage them. So the > hw flows will continue to forward according to policy prior to > ovs-vswitchd shutdown. > > There is also a way to start OVS where it will wait for you to finish > initializing the OpenFlow tables before it starts revalidating the > flows (ovs_vsctl set open_vswitch . > other_config:flow-restore-wait="true") - see utilities/ovs-ctl.in for > how this may be used. > > These are all only considerations where the datapath has its own life > beyond the OVS binary. They don't apply when running with userspace > datapath, clearly, because the datapath and ovs-vswitchd are the same > in that case. They are flushed on next start. In netdev_linux_set_policing we added a call to flush all rules without checking the hw-offload config. Also before it in master branch there is a call to remove ingress and readd it. removing the ingress cause flush of all the rules. So on restart we were always in a situation were all rules are flushed.
diff --git a/lib/netdev.c b/lib/netdev.c index 3ac3c48..b289166 100644 --- a/lib/netdev.c +++ b/lib/netdev.c @@ -2071,7 +2071,25 @@ netdev_init_flow_api(struct netdev *netdev) { const struct netdev_class *class = netdev->netdev_class; + if (!netdev_flow_api_enabled) { + return EOPNOTSUPP; + } + return (class->init_flow_api ? class->init_flow_api(netdev) : EOPNOTSUPP); } + +bool netdev_flow_api_enabled = false; + +void +netdev_set_flow_api_enabled(bool enabled) +{ + static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; + + if (ovsthread_once_start(&once)) { + netdev_flow_api_enabled = enabled; + VLOG_INFO("netdev: Flow API %s", enabled ? "Enabled" : "Disabled"); + ovsthread_once_done(&once); + } +} diff --git a/lib/netdev.h b/lib/netdev.h index c04632d..5be67ea 100644 --- a/lib/netdev.h +++ b/lib/netdev.h @@ -168,6 +168,8 @@ int netdev_flow_get(struct netdev *, struct match *, struct nlattr **actions, struct dpif_flow_stats *, ovs_u128 *, struct ofpbuf *); int netdev_flow_del(struct netdev *, struct dpif_flow_stats *, ovs_u128 *); int netdev_init_flow_api(struct netdev *); +extern bool netdev_flow_api_enabled; +void netdev_set_flow_api_enabled(bool flow_api_enabled); /* native tunnel APIs */ /* Structure to pass parameters required to build a tunnel header. */ diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c index 7f33070..e90a31a 100644 --- a/vswitchd/bridge.c +++ b/vswitchd/bridge.c @@ -2919,6 +2919,8 @@ bridge_run(void) cfg = ovsrec_open_vswitch_first(idl); if (cfg) { + netdev_set_flow_api_enabled(smap_get_bool(&cfg->other_config, + "hw-offload", false)); dpdk_init(&cfg->other_config); } diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml index 66c349b..9bc9f5f 100644 --- a/vswitchd/vswitch.xml +++ b/vswitchd/vswitch.xml @@ -169,6 +169,17 @@ <p> The default is 10000. </p> + </column> + + <column name="other_config" key="hw-offload" + type='{"type": "boolean"}'> + <p> + Set this value to <code>true</code> to enable netdev flow offload. + </p> + <p> + The default value is <code>false</code>. Changing this value requires + restarting the daemon + </p> </column> <column name="other_config" key="dpdk-init"