| Message ID | f5a4dc8baab9154b2755e8ccd36cd85ec09be60b.1608728404.git.marcelo.leitner@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] vswitchd: doc that tc-policy needs a restart | expand |
On 12/23/20 2:01 PM, Marcelo Ricardo Leitner wrote: > tc-policy, just like hw-offload, is protected by ovsthread_once_start() > in netdev_set_flow_api_enabled() so lets document that changing it > requires a restart in order for it to have effect. > > Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> > --- > vswitchd/vswitch.xml | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml > index df5aa41a23da2622eee69f143a52a7e3b970488a..0b853ff26b76aebec8692ac1c59a54a7333e6330 100644 > --- a/vswitchd/vswitch.xml > +++ b/vswitchd/vswitch.xml > @@ -267,7 +267,8 @@ > <ref column="other_config" key="hw-offload"/> is enabled. > </p> > <p> > - The default value is <code>none</code>. > + The default value is <code>none</code>. Changing this value requires > + restarting the daemon That is not fully correct. Only disabling requires restart. Changing the value from 'false' to 'true' is allowed in runtime. > </p> > </column> > >
On Fri, Jan 29, 2021 at 10:31:46PM +0100, Ilya Maximets wrote: > On 12/23/20 2:01 PM, Marcelo Ricardo Leitner wrote: > > tc-policy, just like hw-offload, is protected by ovsthread_once_start() > > in netdev_set_flow_api_enabled() so lets document that changing it > > requires a restart in order for it to have effect. > > > > Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> > > --- > > vswitchd/vswitch.xml | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml > > index df5aa41a23da2622eee69f143a52a7e3b970488a..0b853ff26b76aebec8692ac1c59a54a7333e6330 100644 > > --- a/vswitchd/vswitch.xml > > +++ b/vswitchd/vswitch.xml > > @@ -267,7 +267,8 @@ > > <ref column="other_config" key="hw-offload"/> is enabled. > > </p> > > <p> > > - The default value is <code>none</code>. > > + The default value is <code>none</code>. Changing this value requires > > + restarting the daemon > > That is not fully correct. Only disabling requires restart. > Changing the value from 'false' to 'true' is allowed in runtime. I don't see where that is allowed. The only call I see to tc_set_policy() is from netdev_set_flow_api_enabled(), under the protection of ovsthread_once_start(). > > > </p> > > </column> > > > > >
On 1/29/21 11:33 PM, Marcelo Ricardo Leitner wrote: > On Fri, Jan 29, 2021 at 10:31:46PM +0100, Ilya Maximets wrote: >> On 12/23/20 2:01 PM, Marcelo Ricardo Leitner wrote: >>> tc-policy, just like hw-offload, is protected by ovsthread_once_start() >>> in netdev_set_flow_api_enabled() so lets document that changing it >>> requires a restart in order for it to have effect. >>> >>> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> >>> --- >>> vswitchd/vswitch.xml | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml >>> index df5aa41a23da2622eee69f143a52a7e3b970488a..0b853ff26b76aebec8692ac1c59a54a7333e6330 100644 >>> --- a/vswitchd/vswitch.xml >>> +++ b/vswitchd/vswitch.xml >>> @@ -267,7 +267,8 @@ >>> <ref column="other_config" key="hw-offload"/> is enabled. >>> </p> >>> <p> >>> - The default value is <code>none</code>. >>> + The default value is <code>none</code>. Changing this value requires >>> + restarting the daemon >> >> That is not fully correct. Only disabling requires restart. >> Changing the value from 'false' to 'true' is allowed in runtime. > > I don't see where that is allowed. The only call I see to > tc_set_policy() is from netdev_set_flow_api_enabled(), under the > protection of ovsthread_once_start(). Yep. But that call is inside the 'if': https://github.com/openvswitch/ovs/blob/master/lib/netdev-offload.c#L673 And this ovsthread_once_start() only executed if 'hw-offload' was set to 'true'. This way you should be able to turn on offloading at any time, but turning off requires restart. > >> >>> </p> >>> </column> >>> >>> >>
On Fri, Jan 29, 2021 at 11:48:37PM +0100, Ilya Maximets wrote: > On 1/29/21 11:33 PM, Marcelo Ricardo Leitner wrote: > > On Fri, Jan 29, 2021 at 10:31:46PM +0100, Ilya Maximets wrote: > >> On 12/23/20 2:01 PM, Marcelo Ricardo Leitner wrote: > >>> tc-policy, just like hw-offload, is protected by ovsthread_once_start() > >>> in netdev_set_flow_api_enabled() so lets document that changing it > >>> requires a restart in order for it to have effect. > >>> > >>> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> > >>> --- > >>> vswitchd/vswitch.xml | 3 ++- > >>> 1 file changed, 2 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml > >>> index df5aa41a23da2622eee69f143a52a7e3b970488a..0b853ff26b76aebec8692ac1c59a54a7333e6330 100644 > >>> --- a/vswitchd/vswitch.xml > >>> +++ b/vswitchd/vswitch.xml > >>> @@ -267,7 +267,8 @@ > >>> <ref column="other_config" key="hw-offload"/> is enabled. > >>> </p> > >>> <p> > >>> - The default value is <code>none</code>. > >>> + The default value is <code>none</code>. Changing this value requires > >>> + restarting the daemon > >> > >> That is not fully correct. Only disabling requires restart. > >> Changing the value from 'false' to 'true' is allowed in runtime. > > > > I don't see where that is allowed. The only call I see to > > tc_set_policy() is from netdev_set_flow_api_enabled(), under the > > protection of ovsthread_once_start(). > > Yep. But that call is inside the 'if': > https://github.com/openvswitch/ovs/blob/master/lib/netdev-offload.c#L673 > > And this ovsthread_once_start() only executed if 'hw-offload' was set to Right. That's covered on the paragraph before the change: <p> This is only relevant if <ref column="other_config" key="hw-offload"/> is enabled. </p> > 'true'. This way you should be able to turn on offloading at any time, > but turning off requires restart. Ohh I see what you mean. You're referring to hw-offload then, ok. The patch here is about tc-policy and not hw-offload, though, that's why I was not following. For tc-policy, it is only parsed once when hw-offload is enabled, not mattering if enabled after starting or starting with it enabled. Right? Then depending on the below, we could word it as + The default value is <code>none</code>. Changing this value requires + restarting the daemon if hw-offload was already enabled. For hw-offload, seems you're right. It would require bridge_run() to be called again. I see an 'while (!exiting)' in vswitchd main() that would allow it, but I don't know how one could trigger that? > > > > >> > >>> </p> > >>> </column> > >>> > >>> > >> >
On 1/30/21 12:36 AM, Marcelo Ricardo Leitner wrote: > On Fri, Jan 29, 2021 at 11:48:37PM +0100, Ilya Maximets wrote: >> On 1/29/21 11:33 PM, Marcelo Ricardo Leitner wrote: >>> On Fri, Jan 29, 2021 at 10:31:46PM +0100, Ilya Maximets wrote: >>>> On 12/23/20 2:01 PM, Marcelo Ricardo Leitner wrote: >>>>> tc-policy, just like hw-offload, is protected by ovsthread_once_start() >>>>> in netdev_set_flow_api_enabled() so lets document that changing it >>>>> requires a restart in order for it to have effect. >>>>> >>>>> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> >>>>> --- >>>>> vswitchd/vswitch.xml | 3 ++- >>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml >>>>> index df5aa41a23da2622eee69f143a52a7e3b970488a..0b853ff26b76aebec8692ac1c59a54a7333e6330 100644 >>>>> --- a/vswitchd/vswitch.xml >>>>> +++ b/vswitchd/vswitch.xml >>>>> @@ -267,7 +267,8 @@ >>>>> <ref column="other_config" key="hw-offload"/> is enabled. >>>>> </p> >>>>> <p> >>>>> - The default value is <code>none</code>. >>>>> + The default value is <code>none</code>. Changing this value requires >>>>> + restarting the daemon >>>> >>>> That is not fully correct. Only disabling requires restart. >>>> Changing the value from 'false' to 'true' is allowed in runtime. >>> >>> I don't see where that is allowed. The only call I see to >>> tc_set_policy() is from netdev_set_flow_api_enabled(), under the >>> protection of ovsthread_once_start(). >> >> Yep. But that call is inside the 'if': >> https://github.com/openvswitch/ovs/blob/master/lib/netdev-offload.c#L673 >> >> And this ovsthread_once_start() only executed if 'hw-offload' was set to > > Right. That's covered on the paragraph before the change: > > <p> > This is only relevant if > <ref column="other_config" key="hw-offload"/> is enabled. > </p> > >> 'true'. This way you should be able to turn on offloading at any time, >> but turning off requires restart. > > Ohh I see what you mean. You're referring to hw-offload then, ok. The > patch here is about tc-policy and not hw-offload, though, that's why I > was not following. I misunderstood too. At some point I started to think that this change is for 'hw-offload', I don't know why. But anyway, my words seem to have some meaning. :) And we're on a same page now. > > For tc-policy, it is only parsed once when hw-offload is enabled, not > mattering if enabled after starting or starting with it enabled. > Right? Then depending on the below, we could word it as > + The default value is <code>none</code>. Changing this value requires > + restarting the daemon if hw-offload was already enabled. Right. And this sounds better, yes. But, please, use <ref column="other_config" key="hw-offload"/> to reference the 'hw-offload' configuration knob. > > For hw-offload, seems you're right. It would require bridge_run() to > be called again. I see an 'while (!exiting)' in vswitchd main() that > would allow it, but I don't know how one could trigger that? It's just an infinite event processing loop. It sleeps inside the poll_block() and wakes up on any event, e.g. some data on a database connection socket (due to db update) that needs processing or interface state changes or whatever else. > >> >>> >>>> >>>>> </p> >>>>> </column> >>>>> >>>>> >>>> >>
diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml index df5aa41a23da2622eee69f143a52a7e3b970488a..0b853ff26b76aebec8692ac1c59a54a7333e6330 100644 --- a/vswitchd/vswitch.xml +++ b/vswitchd/vswitch.xml @@ -267,7 +267,8 @@ <ref column="other_config" key="hw-offload"/> is enabled. </p> <p> - The default value is <code>none</code>. + The default value is <code>none</code>. Changing this value requires + restarting the daemon </p> </column>
tc-policy, just like hw-offload, is protected by ovsthread_once_start() in netdev_set_flow_api_enabled() so lets document that changing it requires a restart in order for it to have effect. Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> --- vswitchd/vswitch.xml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)