[net-next 1/2] xen-netback: add module parameter to disable ctrl-ring

[net-next 1/2] xen-netback: add module parameter to disable ctrl-ring

[ChiaHao Hsu]

In order to support live migration of guests between kernels
that do and do not support 'feature-ctrl-ring', we add a
module parameter that allows the feature to be disabled
at run time, instead of using hardcode value.
The default value is enable.

Signed-off-by: ChiaHao Hsu <andyhsu@amazon.com>
---
 drivers/net/xen-netback/common.h  |  2 ++
 drivers/net/xen-netback/netback.c |  6 ++++++
 drivers/net/xen-netback/xenbus.c  | 23 ++++++++++++++---------
 3 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
index 4a16d6e33c09..bfb7a3054917 100644
--- a/drivers/net/xen-netback/common.h
+++ b/drivers/net/xen-netback/common.h
@@ -276,6 +276,7 @@ struct backend_info {
 	u8 have_hotplug_status_watch:1;
 
 	const char *hotplug_script;
+	bool ctrl_ring_enabled;
 };
 
 struct xenvif {
@@ -413,6 +414,7 @@ static inline pending_ring_idx_t nr_pending_reqs(struct xenvif_queue *queue)
 
 irqreturn_t xenvif_interrupt(int irq, void *dev_id);
 
+extern bool control_ring;
 extern bool separate_tx_rx_irq;
 extern bool provides_xdp_headroom;
 
diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index 39a01c2a3058..a119ae673862 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -48,6 +48,12 @@
 
 #include <asm/xen/hypercall.h>
 
+/* Provide an option to disable control ring which is used to pass
+ * large quantities of data from frontend to backend.
+ */
+bool control_ring = true;
+module_param(control_ring, bool, 0644);
+
 /* Provide an option to disable split event channels at load time as
  * event channels are limited resource. Split event channels are
  * enabled by default.
diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
index a5439c130130..9801b8d10239 100644
--- a/drivers/net/xen-netback/xenbus.c
+++ b/drivers/net/xen-netback/xenbus.c
@@ -755,10 +755,12 @@ static void connect(struct backend_info *be)
 	xen_register_watchers(dev, be->vif);
 	read_xenbus_vif_flags(be);
 
-	err = connect_ctrl_ring(be);
-	if (err) {
-		xenbus_dev_fatal(dev, err, "connecting control ring");
-		return;
+	if (be->ctrl_ring_enabled) {
+		err = connect_ctrl_ring(be);
+		if (err) {
+			xenbus_dev_fatal(dev, err, "connecting control ring");
+			return;
+		}
 	}
 
 	/* Use the number of queues requested by the frontend */
@@ -1123,11 +1125,14 @@ static int netback_probe(struct xenbus_device *dev,
 	if (err)
 		pr_debug("Error writing multi-queue-max-queues\n");
 
-	err = xenbus_printf(XBT_NIL, dev->nodename,
-			    "feature-ctrl-ring",
-			    "%u", true);
-	if (err)
-		pr_debug("Error writing feature-ctrl-ring\n");
+	be->ctrl_ring_enabled = READ_ONCE(control_ring);
+	if (be->ctrl_ring_enabled) {
+		err = xenbus_printf(XBT_NIL, dev->nodename,
+				    "feature-ctrl-ring",
+				    "%u", true);
+		if (err)
+			pr_debug("Error writing feature-ctrl-ring\n");
+	}
 
 	backend_switch_state(be, XenbusStateInitWait);
 
-- 
2.23.3

Re: [net-next 1/2] xen-netback: add module parameter to disable ctrl-ring

[Paul Durrant]

On 11/03/2021 22:59, ChiaHao Hsu wrote:
> In order to support live migration of guests between kernels
> that do and do not support 'feature-ctrl-ring', we add a
> module parameter that allows the feature to be disabled
> at run time, instead of using hardcode value.
> The default value is enable.
> 
> Signed-off-by: ChiaHao Hsu <andyhsu@amazon.com>

Reviewed-by: Paul Durrant <paul@xen.org>

Re: [net-next 1/2] xen-netback: add module parameter to disable ctrl-ring

[Andrew Lunn]

On Thu, Mar 11, 2021 at 10:59:44PM +0000, ChiaHao Hsu wrote:
> In order to support live migration of guests between kernels
> that do and do not support 'feature-ctrl-ring', we add a
> module parameter that allows the feature to be disabled
> at run time, instead of using hardcode value.
> The default value is enable.

Hi ChiaHao

There is a general dislike for module parameters. What other mechanisms
have you looked at? Would an ethtool private flag work?

     Andrew

Re: [net-next 1/2] xen-netback: add module parameter to disable ctrl-ring

[Hsu, Chiahao]

Andrew Lunn 於 2021/3/12 15:52 寫道:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>
>
>
> On Thu, Mar 11, 2021 at 10:59:44PM +0000, ChiaHao Hsu wrote:
>> In order to support live migration of guests between kernels
>> that do and do not support 'feature-ctrl-ring', we add a
>> module parameter that allows the feature to be disabled
>> at run time, instead of using hardcode value.
>> The default value is enable.
> Hi ChiaHao
>
> There is a general dislike for module parameters. What other mechanisms
> have you looked at? Would an ethtool private flag work?
>
>       Andrew


Hi Andrew,

I can survey other mechanisms, however before I start doing that,

could you share more details about what the problem is with using module 
parameters? thanks.

ChiaHao

Re: [net-next 1/2] xen-netback: add module parameter to disable ctrl-ring

[Andrew Lunn]

On Fri, Mar 12, 2021 at 04:18:02PM +0100, Hsu, Chiahao wrote:
> 
> Andrew Lunn 於 2021/3/12 15:52 寫道:
> > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> > 
> > 
> > 
> > On Thu, Mar 11, 2021 at 10:59:44PM +0000, ChiaHao Hsu wrote:
> > > In order to support live migration of guests between kernels
> > > that do and do not support 'feature-ctrl-ring', we add a
> > > module parameter that allows the feature to be disabled
> > > at run time, instead of using hardcode value.
> > > The default value is enable.
> > Hi ChiaHao
> > 
> > There is a general dislike for module parameters. What other mechanisms
> > have you looked at? Would an ethtool private flag work?
> > 
> >       Andrew
> 
> 
> Hi Andrew,
> 
> I can survey other mechanisms, however before I start doing that,
> 
> could you share more details about what the problem is with using module
> parameters? thanks.

It is not very user friendly. No two kernel modules use the same
module parameters. Often you see the same name, but different
meaning. There is poor documentation, you often need to read the
kernel sources it figure out what it does, etc.

Ideally, you want a mechanism which is shared by multiple drivers and
is well documented.

Does virtio have the same problems? What about VmWare? HyperV? Could
you make a generic solution which works for all these technologies?
Is this just a networking problem? Or does disk, graphics etc, need
something similar?

    Andrew

Re: [net-next 1/2] xen-netback: add module parameter to disable ctrl-ring

[Leon Romanovsky]

On Fri, Mar 12, 2021 at 09:36:59PM +0100, Andrew Lunn wrote:
> On Fri, Mar 12, 2021 at 04:18:02PM +0100, Hsu, Chiahao wrote:
> >
> > Andrew Lunn 於 2021/3/12 15:52 寫道:
> > > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> > >
> > >
> > >
> > > On Thu, Mar 11, 2021 at 10:59:44PM +0000, ChiaHao Hsu wrote:
> > > > In order to support live migration of guests between kernels
> > > > that do and do not support 'feature-ctrl-ring', we add a
> > > > module parameter that allows the feature to be disabled
> > > > at run time, instead of using hardcode value.
> > > > The default value is enable.
> > > Hi ChiaHao
> > >
> > > There is a general dislike for module parameters. What other mechanisms
> > > have you looked at? Would an ethtool private flag work?
> > >
> > >       Andrew
> >
> >
> > Hi Andrew,
> >
> > I can survey other mechanisms, however before I start doing that,
> >
> > could you share more details about what the problem is with using module
> > parameters? thanks.
>
> It is not very user friendly. No two kernel modules use the same
> module parameters. Often you see the same name, but different
> meaning. There is poor documentation, you often need to read the
> kernel sources it figure out what it does, etc.

+1, It is also global parameter to whole system/devices that use this
module, which is rarely what users want.

Thanks

Re: [net-next 1/2] xen-netback: add module parameter to disable ctrl-ring

[Hsu, Chiahao]

Leon Romanovsky 於 2021/3/14 11:04 寫道:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>
>
>
> On Fri, Mar 12, 2021 at 09:36:59PM +0100, Andrew Lunn wrote:
>> On Fri, Mar 12, 2021 at 04:18:02PM +0100, Hsu, Chiahao wrote:
>>> Andrew Lunn 於 2021/3/12 15:52 寫道:
>>>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>>>>
>>>>
>>>>
>>>> On Thu, Mar 11, 2021 at 10:59:44PM +0000, ChiaHao Hsu wrote:
>>>>> In order to support live migration of guests between kernels
>>>>> that do and do not support 'feature-ctrl-ring', we add a
>>>>> module parameter that allows the feature to be disabled
>>>>> at run time, instead of using hardcode value.
>>>>> The default value is enable.
>>>> Hi ChiaHao
>>>>
>>>> There is a general dislike for module parameters. What other mechanisms
>>>> have you looked at? Would an ethtool private flag work?
>>>>
>>>>        Andrew
>>>
>>> Hi Andrew,
>>>
>>> I can survey other mechanisms, however before I start doing that,
>>>
>>> could you share more details about what the problem is with using module
>>> parameters? thanks.
>> It is not very user friendly. No two kernel modules use the same
>> module parameters. Often you see the same name, but different
>> meaning. There is poor documentation, you often need to read the
>> kernel sources it figure out what it does, etc.
> +1, It is also global parameter to whole system/devices that use this
> module, which is rarely what users want.
>
> Thanks

Hi,
I think I would say the current implementation(modparams) isappropriate
after reviewing it again.

We are talking about 'feature leveling', a way to support live 
migrationof guest
between kernels that do and do not support the features. So we want to 
refrain
fromadding the features if guest would be migrated to the kernel which does
not support the feature. Everythingshould be done (in probe function) before
frontend connects, and this is why ethtool is not appropriate for this.

Thanks

Re: [net-next 1/2] xen-netback: add module parameter to disable ctrl-ring

[Leon Romanovsky]

On Tue, Mar 16, 2021 at 04:22:21PM +0100, Hsu, Chiahao wrote:
>
>
> Leon Romanovsky 於 2021/3/14 11:04 寫道:
> > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> >
> >
> >
> > On Fri, Mar 12, 2021 at 09:36:59PM +0100, Andrew Lunn wrote:
> > > On Fri, Mar 12, 2021 at 04:18:02PM +0100, Hsu, Chiahao wrote:
> > > > Andrew Lunn 於 2021/3/12 15:52 寫道:
> > > > > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> > > > >
> > > > >
> > > > >
> > > > > On Thu, Mar 11, 2021 at 10:59:44PM +0000, ChiaHao Hsu wrote:
> > > > > > In order to support live migration of guests between kernels
> > > > > > that do and do not support 'feature-ctrl-ring', we add a
> > > > > > module parameter that allows the feature to be disabled
> > > > > > at run time, instead of using hardcode value.
> > > > > > The default value is enable.
> > > > > Hi ChiaHao
> > > > >
> > > > > There is a general dislike for module parameters. What other mechanisms
> > > > > have you looked at? Would an ethtool private flag work?
> > > > >
> > > > >        Andrew
> > > >
> > > > Hi Andrew,
> > > >
> > > > I can survey other mechanisms, however before I start doing that,
> > > >
> > > > could you share more details about what the problem is with using module
> > > > parameters? thanks.
> > > It is not very user friendly. No two kernel modules use the same
> > > module parameters. Often you see the same name, but different
> > > meaning. There is poor documentation, you often need to read the
> > > kernel sources it figure out what it does, etc.
> > +1, It is also global parameter to whole system/devices that use this
> > module, which is rarely what users want.
> >
> > Thanks
>
> Hi,
> I think I would say the current implementation(modparams) isappropriate
> after reviewing it again.
>
> We are talking about 'feature leveling', a way to support live migrationof
> guest
> between kernels that do and do not support the features. So we want to
> refrain
> fromadding the features if guest would be migrated to the kernel which does
> not support the feature. Everythingshould be done (in probe function) before
> frontend connects, and this is why ethtool is not appropriate for this.

It wouldn't be a surprise to you that feature discovery is not supposed
to be done through module parameters. Instead of asking from user to
randomly disable some feature, the system is expected to be backward
compatible and robust enough to query the list of supported/needed
features.

Thanks

>
> Thanks
>
>

Re: [net-next 1/2] xen-netback: add module parameter to disable ctrl-ring

[Hsu, Chiahao]

Leon Romanovsky 於 2021/3/17 18:22 寫道:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>
>
>
> On Tue, Mar 16, 2021 at 04:22:21PM +0100, Hsu, Chiahao wrote:
>>
>> Leon Romanovsky 於 2021/3/14 11:04 寫道:
>>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>>>
>>>
>>>
>>> On Fri, Mar 12, 2021 at 09:36:59PM +0100, Andrew Lunn wrote:
>>>> On Fri, Mar 12, 2021 at 04:18:02PM +0100, Hsu, Chiahao wrote:
>>>>> Andrew Lunn 於 2021/3/12 15:52 寫道:
>>>>>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Thu, Mar 11, 2021 at 10:59:44PM +0000, ChiaHao Hsu wrote:
>>>>>>> In order to support live migration of guests between kernels
>>>>>>> that do and do not support 'feature-ctrl-ring', we add a
>>>>>>> module parameter that allows the feature to be disabled
>>>>>>> at run time, instead of using hardcode value.
>>>>>>> The default value is enable.
>>>>>> Hi ChiaHao
>>>>>>
>>>>>> There is a general dislike for module parameters. What other mechanisms
>>>>>> have you looked at? Would an ethtool private flag work?
>>>>>>
>>>>>>         Andrew
>>>>> Hi Andrew,
>>>>>
>>>>> I can survey other mechanisms, however before I start doing that,
>>>>>
>>>>> could you share more details about what the problem is with using module
>>>>> parameters? thanks.
>>>> It is not very user friendly. No two kernel modules use the same
>>>> module parameters. Often you see the same name, but different
>>>> meaning. There is poor documentation, you often need to read the
>>>> kernel sources it figure out what it does, etc.
>>> +1, It is also global parameter to whole system/devices that use this
>>> module, which is rarely what users want.
>>>
>>> Thanks
>> Hi,
>> I think I would say the current implementation(modparams) isappropriate
>> after reviewing it again.
>>
>> We are talking about 'feature leveling', a way to support live migrationof
>> guest
>> between kernels that do and do not support the features. So we want to
>> refrain
>> fromadding the features if guest would be migrated to the kernel which does
>> not support the feature. Everythingshould be done (in probe function) before
>> frontend connects, and this is why ethtool is not appropriate for this.
> It wouldn't be a surprise to you that feature discovery is not supposed
> to be done through module parameters. Instead of asking from user to
> randomly disable some feature, the system is expected to be backward
> compatible and robust enough to query the list of supported/needed
> features.
>
> Thanks
>
>> Thanks
>>
>>
Typically there should be one VM running netback on each host,
and having control over what interfaces or features it exposes is also 
important for stability.
How about we create a 'feature flags' modparam, each bits is specified 
for different new features?

Re: [net-next 1/2] xen-netback: add module parameter to disable ctrl-ring

[Leon Romanovsky]

On Sun, Mar 21, 2021 at 05:31:08PM +0100, Hsu, Chiahao wrote:
> 
> 
> Leon Romanovsky 於 2021/3/17 18:22 寫道:
> > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> > 
> > 
> > 
> > On Tue, Mar 16, 2021 at 04:22:21PM +0100, Hsu, Chiahao wrote:
> > > 
> > > Leon Romanovsky 於 2021/3/14 11:04 寫道:
> > > > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> > > > 
> > > > 
> > > > 
> > > > On Fri, Mar 12, 2021 at 09:36:59PM +0100, Andrew Lunn wrote:
> > > > > On Fri, Mar 12, 2021 at 04:18:02PM +0100, Hsu, Chiahao wrote:
> > > > > > Andrew Lunn 於 2021/3/12 15:52 寫道:
> > > > > > > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > On Thu, Mar 11, 2021 at 10:59:44PM +0000, ChiaHao Hsu wrote:
> > > > > > > > In order to support live migration of guests between kernels
> > > > > > > > that do and do not support 'feature-ctrl-ring', we add a
> > > > > > > > module parameter that allows the feature to be disabled
> > > > > > > > at run time, instead of using hardcode value.
> > > > > > > > The default value is enable.
> > > > > > > Hi ChiaHao
> > > > > > > 
> > > > > > > There is a general dislike for module parameters. What other mechanisms
> > > > > > > have you looked at? Would an ethtool private flag work?
> > > > > > > 
> > > > > > >         Andrew
> > > > > > Hi Andrew,
> > > > > > 
> > > > > > I can survey other mechanisms, however before I start doing that,
> > > > > > 
> > > > > > could you share more details about what the problem is with using module
> > > > > > parameters? thanks.
> > > > > It is not very user friendly. No two kernel modules use the same
> > > > > module parameters. Often you see the same name, but different
> > > > > meaning. There is poor documentation, you often need to read the
> > > > > kernel sources it figure out what it does, etc.
> > > > +1, It is also global parameter to whole system/devices that use this
> > > > module, which is rarely what users want.
> > > > 
> > > > Thanks
> > > Hi,
> > > I think I would say the current implementation(modparams) isappropriate
> > > after reviewing it again.
> > > 
> > > We are talking about 'feature leveling', a way to support live migrationof
> > > guest
> > > between kernels that do and do not support the features. So we want to
> > > refrain
> > > fromadding the features if guest would be migrated to the kernel which does
> > > not support the feature. Everythingshould be done (in probe function) before
> > > frontend connects, and this is why ethtool is not appropriate for this.
> > It wouldn't be a surprise to you that feature discovery is not supposed
> > to be done through module parameters. Instead of asking from user to
> > randomly disable some feature, the system is expected to be backward
> > compatible and robust enough to query the list of supported/needed
> > features.
> > 
> > Thanks
> > 
> > > Thanks
> > > 
> > > 
> Typically there should be one VM running netback on each host,
> and having control over what interfaces or features it exposes is also
> important for stability.
> How about we create a 'feature flags' modparam, each bits is specified for
> different new features?

At the end, it will be more granular module parameter that user still
will need to guess.

Thanks

> 

Re: [net-next 1/2] xen-netback: add module parameter to disable ctrl-ring

[Hsu, Chiahao]

Leon Romanovsky 於 2021/3/21 18:22 寫道:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>
>
>
> On Sun, Mar 21, 2021 at 05:31:08PM +0100, Hsu, Chiahao wrote:
>>
>> Leon Romanovsky 於 2021/3/17 18:22 寫道:
>>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>>>
>>>
>>>
>>> On Tue, Mar 16, 2021 at 04:22:21PM +0100, Hsu, Chiahao wrote:
>>>> Leon Romanovsky 於 2021/3/14 11:04 寫道:
>>>>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>>>>>
>>>>>
>>>>>
>>>>> On Fri, Mar 12, 2021 at 09:36:59PM +0100, Andrew Lunn wrote:
>>>>>> On Fri, Mar 12, 2021 at 04:18:02PM +0100, Hsu, Chiahao wrote:
>>>>>>> Andrew Lunn 於 2021/3/12 15:52 寫道:
>>>>>>>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On Thu, Mar 11, 2021 at 10:59:44PM +0000, ChiaHao Hsu wrote:
>>>>>>>>> In order to support live migration of guests between kernels
>>>>>>>>> that do and do not support 'feature-ctrl-ring', we add a
>>>>>>>>> module parameter that allows the feature to be disabled
>>>>>>>>> at run time, instead of using hardcode value.
>>>>>>>>> The default value is enable.
>>>>>>>> Hi ChiaHao
>>>>>>>>
>>>>>>>> There is a general dislike for module parameters. What other mechanisms
>>>>>>>> have you looked at? Would an ethtool private flag work?
>>>>>>>>
>>>>>>>>          Andrew
>>>>>>> Hi Andrew,
>>>>>>>
>>>>>>> I can survey other mechanisms, however before I start doing that,
>>>>>>>
>>>>>>> could you share more details about what the problem is with using module
>>>>>>> parameters? thanks.
>>>>>> It is not very user friendly. No two kernel modules use the same
>>>>>> module parameters. Often you see the same name, but different
>>>>>> meaning. There is poor documentation, you often need to read the
>>>>>> kernel sources it figure out what it does, etc.
>>>>> +1, It is also global parameter to whole system/devices that use this
>>>>> module, which is rarely what users want.
>>>>>
>>>>> Thanks
>>>> Hi,
>>>> I think I would say the current implementation(modparams) isappropriate
>>>> after reviewing it again.
>>>>
>>>> We are talking about 'feature leveling', a way to support live migrationof
>>>> guest
>>>> between kernels that do and do not support the features. So we want to
>>>> refrain
>>>> fromadding the features if guest would be migrated to the kernel which does
>>>> not support the feature. Everythingshould be done (in probe function) before
>>>> frontend connects, and this is why ethtool is not appropriate for this.
>>> It wouldn't be a surprise to you that feature discovery is not supposed
>>> to be done through module parameters. Instead of asking from user to
>>> randomly disable some feature, the system is expected to be backward
>>> compatible and robust enough to query the list of supported/needed
>>> features.
>>>
>>> Thanks
>>>
>>>> Thanks
>>>>
>>>>
>> Typically there should be one VM running netback on each host,
>> and having control over what interfaces or features it exposes is also
>> important for stability.
>> How about we create a 'feature flags' modparam, each bits is specified for
>> different new features?
> At the end, it will be more granular module parameter that user still
> will need to guess.
I believe users always need to know any parameter or any tool's flag 
before they use it.
For example, before user try to set/clear this ctrl_ring_enabled, they 
should already have basic knowledge about this feature,
or else they shouldn't use it (the default value is same as before), and 
that's also why we use the 'ctrl_ring_enabled' as parameter name.

Thanks
> Thanks
>

Re: [net-next 1/2] xen-netback: add module parameter to disable ctrl-ring

[Andrew Lunn]

> > At the end, it will be more granular module parameter that user still
> > will need to guess.
> I believe users always need to know any parameter or any tool's flag before
> they use it.
> For example, before user try to set/clear this ctrl_ring_enabled, they
> should already have basic knowledge about this feature,
> or else they shouldn't use it (the default value is same as before), and
> that's also why we use the 'ctrl_ring_enabled' as parameter name.

To me, it seems you are fixing this problem in the wrong place. As a
VM user in the cloud, i have no idea how the cloud provider needs the
VM configured to allow the cloud provider to migrate the VM to
somewhere else in the bitbarn. As the VM user, it should not be my
problem. I would expect the cloud provider to configure the VM host to
only expose facilities to the VM which allows it to be migrated.

This is a VM host problem, not a VM problem.

     Andrew

Re: [net-next 1/2] xen-netback: add module parameter to disable ctrl-ring

[Hsu, Chiahao]

Andrew Lunn 於 2021/3/21 21:29 寫道:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>
>
>
>>> At the end, it will be more granular module parameter that user still
>>> will need to guess.
>> I believe users always need to know any parameter or any tool's flag before
>> they use it.
>> For example, before user try to set/clear this ctrl_ring_enabled, they
>> should already have basic knowledge about this feature,
>> or else they shouldn't use it (the default value is same as before), and
>> that's also why we use the 'ctrl_ring_enabled' as parameter name.
> To me, it seems you are fixing this problem in the wrong place. As a
> VM user in the cloud, i have no idea how the cloud provider needs the
> VM configured to allow the cloud provider to migrate the VM to
> somewhere else in the bitbarn. As the VM user, it should not be my
> problem. I would expect the cloud provider to configure the VM host to
> only expose facilities to the VM which allows it to be migrated.
'the users' I mentioned it's the cloud provider, not a VM user. Sorry 
for the confusion.
And agree with you, just wondering how the cloud provider can expose the 
facilities to the VM if there's no way to set/clr it at runtime, unless 
reconfigure kernel?  These features are enabled by default.
>
> This is a VM host problem, not a VM problem.
>
>       Andrew


Thanks

Re: [net-next 1/2] xen-netback: add module parameter to disable ctrl-ring

[Leon Romanovsky]

On Sun, Mar 21, 2021 at 06:54:52PM +0100, Hsu, Chiahao wrote:
> 

<...>

> > > Typically there should be one VM running netback on each host,
> > > and having control over what interfaces or features it exposes is also
> > > important for stability.
> > > How about we create a 'feature flags' modparam, each bits is specified for
> > > different new features?
> > At the end, it will be more granular module parameter that user still
> > will need to guess.
> I believe users always need to know any parameter or any tool's flag before
> they use it.
> For example, before user try to set/clear this ctrl_ring_enabled, they
> should already have basic knowledge about this feature,
> or else they shouldn't use it (the default value is same as before), and
> that's also why we use the 'ctrl_ring_enabled' as parameter name.

It solves only forward migration flow. Move from machine A with no
option X to machine B with option X. It doesn't work for backward
flow. Move from machine B to A back will probably break.

In your flow, you want that users will set all module parameters for
every upgrade and keep those parameters differently per-version.

Thanks

> 
> Thanks
> > Thanks
> > 
> 

Re: [net-next 1/2] xen-netback: add module parameter to disable ctrl-ring

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 1619 bytes --]

On 22.03.21 06:39, Leon Romanovsky wrote:
> On Sun, Mar 21, 2021 at 06:54:52PM +0100, Hsu, Chiahao wrote:
>>
> 
> <...>
> 
>>>> Typically there should be one VM running netback on each host,
>>>> and having control over what interfaces or features it exposes is also
>>>> important for stability.
>>>> How about we create a 'feature flags' modparam, each bits is specified for
>>>> different new features?
>>> At the end, it will be more granular module parameter that user still
>>> will need to guess.
>> I believe users always need to know any parameter or any tool's flag before
>> they use it.
>> For example, before user try to set/clear this ctrl_ring_enabled, they
>> should already have basic knowledge about this feature,
>> or else they shouldn't use it (the default value is same as before), and
>> that's also why we use the 'ctrl_ring_enabled' as parameter name.
> 
> It solves only forward migration flow. Move from machine A with no
> option X to machine B with option X. It doesn't work for backward
> flow. Move from machine B to A back will probably break.
> 
> In your flow, you want that users will set all module parameters for
> every upgrade and keep those parameters differently per-version.

I think the flag should be a per guest config item. Adding this item to
the backend Xenstore nodes for netback to consume it should be rather
easy.

Yes, this would need a change in Xen tools, too, but it is the most
flexible way to handle it. And in case of migration the information
would be just migrated to the new host with the guest's config data.


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [net-next 1/2] xen-netback: add module parameter to disable ctrl-ring

[Leon Romanovsky]

On Mon, Mar 22, 2021 at 06:58:34AM +0100, Jürgen Groß wrote:
> On 22.03.21 06:39, Leon Romanovsky wrote:
> > On Sun, Mar 21, 2021 at 06:54:52PM +0100, Hsu, Chiahao wrote:
> > > 
> > 
> > <...>
> > 
> > > > > Typically there should be one VM running netback on each host,
> > > > > and having control over what interfaces or features it exposes is also
> > > > > important for stability.
> > > > > How about we create a 'feature flags' modparam, each bits is specified for
> > > > > different new features?
> > > > At the end, it will be more granular module parameter that user still
> > > > will need to guess.
> > > I believe users always need to know any parameter or any tool's flag before
> > > they use it.
> > > For example, before user try to set/clear this ctrl_ring_enabled, they
> > > should already have basic knowledge about this feature,
> > > or else they shouldn't use it (the default value is same as before), and
> > > that's also why we use the 'ctrl_ring_enabled' as parameter name.
> > 
> > It solves only forward migration flow. Move from machine A with no
> > option X to machine B with option X. It doesn't work for backward
> > flow. Move from machine B to A back will probably break.
> > 
> > In your flow, you want that users will set all module parameters for
> > every upgrade and keep those parameters differently per-version.
> 
> I think the flag should be a per guest config item. Adding this item to
> the backend Xenstore nodes for netback to consume it should be rather
> easy.
> 
> Yes, this would need a change in Xen tools, too, but it is the most
> flexible way to handle it. And in case of migration the information
> would be just migrated to the new host with the guest's config data.

Yes, it will overcome global nature of module parameters, but how does
it solve backward compatibility concern?

Thanks

> 
> 
> Juergen

Re: [net-next 1/2] xen-netback: add module parameter to disable ctrl-ring

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 2787 bytes --]

On 22.03.21 07:48, Leon Romanovsky wrote:
> On Mon, Mar 22, 2021 at 06:58:34AM +0100, Jürgen Groß wrote:
>> On 22.03.21 06:39, Leon Romanovsky wrote:
>>> On Sun, Mar 21, 2021 at 06:54:52PM +0100, Hsu, Chiahao wrote:
>>>>
>>>
>>> <...>
>>>
>>>>>> Typically there should be one VM running netback on each host,
>>>>>> and having control over what interfaces or features it exposes is also
>>>>>> important for stability.
>>>>>> How about we create a 'feature flags' modparam, each bits is specified for
>>>>>> different new features?
>>>>> At the end, it will be more granular module parameter that user still
>>>>> will need to guess.
>>>> I believe users always need to know any parameter or any tool's flag before
>>>> they use it.
>>>> For example, before user try to set/clear this ctrl_ring_enabled, they
>>>> should already have basic knowledge about this feature,
>>>> or else they shouldn't use it (the default value is same as before), and
>>>> that's also why we use the 'ctrl_ring_enabled' as parameter name.
>>>
>>> It solves only forward migration flow. Move from machine A with no
>>> option X to machine B with option X. It doesn't work for backward
>>> flow. Move from machine B to A back will probably break.
>>>
>>> In your flow, you want that users will set all module parameters for
>>> every upgrade and keep those parameters differently per-version.
>>
>> I think the flag should be a per guest config item. Adding this item to
>> the backend Xenstore nodes for netback to consume it should be rather
>> easy.
>>
>> Yes, this would need a change in Xen tools, too, but it is the most
>> flexible way to handle it. And in case of migration the information
>> would be just migrated to the new host with the guest's config data.
> 
> Yes, it will overcome global nature of module parameters, but how does
> it solve backward compatibility concern?

When creating a guest on A the (unknown) feature will not be set to
any value in the guest's config data. A migration stream not having any
value for that feature on B should set it to "false".

When creating a guest on B it will either have the feature value set
explicitly in the guest config (either true or false), or it will get
the server's default (this value should be configurable in a global
config file, default for that global value would be "true").

So with the guest created on B with feature specified as "false" (either
for this guest only, or per global config), it will be migratable to
machine A without problem. Migrating it back to B would work the same
way as above. Trying to migrate a guest with feature set to "true" to
B would not work, but this would be the host admin's fault due to not
configuring the guest correctly.


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [net-next 1/2] xen-netback: add module parameter to disable ctrl-ring

[Leon Romanovsky]

On Mon, Mar 22, 2021 at 08:01:17AM +0100, Jürgen Groß wrote:
> On 22.03.21 07:48, Leon Romanovsky wrote:
> > On Mon, Mar 22, 2021 at 06:58:34AM +0100, Jürgen Groß wrote:
> > > On 22.03.21 06:39, Leon Romanovsky wrote:
> > > > On Sun, Mar 21, 2021 at 06:54:52PM +0100, Hsu, Chiahao wrote:
> > > > > 
> > > > 
> > > > <...>
> > > > 
> > > > > > > Typically there should be one VM running netback on each host,
> > > > > > > and having control over what interfaces or features it exposes is also
> > > > > > > important for stability.
> > > > > > > How about we create a 'feature flags' modparam, each bits is specified for
> > > > > > > different new features?
> > > > > > At the end, it will be more granular module parameter that user still
> > > > > > will need to guess.
> > > > > I believe users always need to know any parameter or any tool's flag before
> > > > > they use it.
> > > > > For example, before user try to set/clear this ctrl_ring_enabled, they
> > > > > should already have basic knowledge about this feature,
> > > > > or else they shouldn't use it (the default value is same as before), and
> > > > > that's also why we use the 'ctrl_ring_enabled' as parameter name.
> > > > 
> > > > It solves only forward migration flow. Move from machine A with no
> > > > option X to machine B with option X. It doesn't work for backward
> > > > flow. Move from machine B to A back will probably break.
> > > > 
> > > > In your flow, you want that users will set all module parameters for
> > > > every upgrade and keep those parameters differently per-version.
> > > 
> > > I think the flag should be a per guest config item. Adding this item to
> > > the backend Xenstore nodes for netback to consume it should be rather
> > > easy.
> > > 
> > > Yes, this would need a change in Xen tools, too, but it is the most
> > > flexible way to handle it. And in case of migration the information
> > > would be just migrated to the new host with the guest's config data.
> > 
> > Yes, it will overcome global nature of module parameters, but how does
> > it solve backward compatibility concern?
> 
> When creating a guest on A the (unknown) feature will not be set to
> any value in the guest's config data. A migration stream not having any
> value for that feature on B should set it to "false".
> 
> When creating a guest on B it will either have the feature value set
> explicitly in the guest config (either true or false), or it will get
> the server's default (this value should be configurable in a global
> config file, default for that global value would be "true").
> 
> So with the guest created on B with feature specified as "false" (either
> for this guest only, or per global config), it will be migratable to
> machine A without problem. Migrating it back to B would work the same
> way as above. Trying to migrate a guest with feature set to "true" to
> B would not work, but this would be the host admin's fault due to not
> configuring the guest correctly.

As long as all new features are disabled by default, it will be ok.

Thanks

> 
> 
> Juergen

Re: [net-next 1/2] xen-netback: add module parameter to disable ctrl-ring

[Hsu, Chiahao]

Leon Romanovsky 於 2021/3/22 08:13 寫道:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>
>
>
> On Mon, Mar 22, 2021 at 08:01:17AM +0100, Jürgen Groß wrote:
>> On 22.03.21 07:48, Leon Romanovsky wrote:
>>> On Mon, Mar 22, 2021 at 06:58:34AM +0100, Jürgen Groß wrote:
>>>> On 22.03.21 06:39, Leon Romanovsky wrote:
>>>>> On Sun, Mar 21, 2021 at 06:54:52PM +0100, Hsu, Chiahao wrote:
>>>>> <...>
>>>>>
>>>>>>>> Typically there should be one VM running netback on each host,
>>>>>>>> and having control over what interfaces or features it exposes is also
>>>>>>>> important for stability.
>>>>>>>> How about we create a 'feature flags' modparam, each bits is specified for
>>>>>>>> different new features?
>>>>>>> At the end, it will be more granular module parameter that user still
>>>>>>> will need to guess.
>>>>>> I believe users always need to know any parameter or any tool's flag before
>>>>>> they use it.
>>>>>> For example, before user try to set/clear this ctrl_ring_enabled, they
>>>>>> should already have basic knowledge about this feature,
>>>>>> or else they shouldn't use it (the default value is same as before), and
>>>>>> that's also why we use the 'ctrl_ring_enabled' as parameter name.
>>>>> It solves only forward migration flow. Move from machine A with no
>>>>> option X to machine B with option X. It doesn't work for backward
>>>>> flow. Move from machine B to A back will probably break.
>>>>>
>>>>> In your flow, you want that users will set all module parameters for
>>>>> every upgrade and keep those parameters differently per-version.
>>>> I think the flag should be a per guest config item. Adding this item to
>>>> the backend Xenstore nodes for netback to consume it should be rather
>>>> easy.
>>>>
>>>> Yes, this would need a change in Xen tools, too, but it is the most
>>>> flexible way to handle it. And in case of migration the information
>>>> would be just migrated to the new host with the guest's config data.
>>> Yes, it will overcome global nature of module parameters, but how does
>>> it solve backward compatibility concern?
>> When creating a guest on A the (unknown) feature will not be set to
>> any value in the guest's config data. A migration stream not having any
>> value for that feature on B should set it to "false".
>>
>> When creating a guest on B it will either have the feature value set
>> explicitly in the guest config (either true or false), or it will get
>> the server's default (this value should be configurable in a global
>> config file, default for that global value would be "true").
>>
>> So with the guest created on B with feature specified as "false" (either
>> for this guest only, or per global config), it will be migratable to
>> machine A without problem. Migrating it back to B would work the same
>> way as above. Trying to migrate a guest with feature set to "true" to
>> B would not work, but this would be the host admin's fault due to not
>> configuring the guest correctly.
so the expected changes would be

1. remove feature-ctrl-ring & feature-dynamic-multicast-control from 
netback_probe( )
2. consume the backend Xenstore nodes in connect( ) to see if Xen tools 
set nodes(true/false) or not(unknown)

Thanks.

Andy
> As long as all new features are disabled by default, it will be ok.
>
> Thanks
>
>>
>> Juergen
>
>
>
>

Re: [net-next 1/2] xen-netback: add module parameter to disable ctrl-ring

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 3640 bytes --]

On 28.03.21 22:46, Hsu, Chiahao wrote:
> 
> 
> Leon Romanovsky 於 2021/3/22 08:13 寫道:
>> CAUTION: This email originated from outside of the organization. Do 
>> not click links or open attachments unless you can confirm the sender 
>> and know the content is safe.
>>
>>
>>
>> On Mon, Mar 22, 2021 at 08:01:17AM +0100, Jürgen Groß wrote:
>>> On 22.03.21 07:48, Leon Romanovsky wrote:
>>>> On Mon, Mar 22, 2021 at 06:58:34AM +0100, Jürgen Groß wrote:
>>>>> On 22.03.21 06:39, Leon Romanovsky wrote:
>>>>>> On Sun, Mar 21, 2021 at 06:54:52PM +0100, Hsu, Chiahao wrote:
>>>>>> <...>
>>>>>>
>>>>>>>>> Typically there should be one VM running netback on each host,
>>>>>>>>> and having control over what interfaces or features it exposes 
>>>>>>>>> is also
>>>>>>>>> important for stability.
>>>>>>>>> How about we create a 'feature flags' modparam, each bits is 
>>>>>>>>> specified for
>>>>>>>>> different new features?
>>>>>>>> At the end, it will be more granular module parameter that user 
>>>>>>>> still
>>>>>>>> will need to guess.
>>>>>>> I believe users always need to know any parameter or any tool's 
>>>>>>> flag before
>>>>>>> they use it.
>>>>>>> For example, before user try to set/clear this ctrl_ring_enabled, 
>>>>>>> they
>>>>>>> should already have basic knowledge about this feature,
>>>>>>> or else they shouldn't use it (the default value is same as 
>>>>>>> before), and
>>>>>>> that's also why we use the 'ctrl_ring_enabled' as parameter name.
>>>>>> It solves only forward migration flow. Move from machine A with no
>>>>>> option X to machine B with option X. It doesn't work for backward
>>>>>> flow. Move from machine B to A back will probably break.
>>>>>>
>>>>>> In your flow, you want that users will set all module parameters for
>>>>>> every upgrade and keep those parameters differently per-version.
>>>>> I think the flag should be a per guest config item. Adding this 
>>>>> item to
>>>>> the backend Xenstore nodes for netback to consume it should be rather
>>>>> easy.
>>>>>
>>>>> Yes, this would need a change in Xen tools, too, but it is the most
>>>>> flexible way to handle it. And in case of migration the information
>>>>> would be just migrated to the new host with the guest's config data.
>>>> Yes, it will overcome global nature of module parameters, but how does
>>>> it solve backward compatibility concern?
>>> When creating a guest on A the (unknown) feature will not be set to
>>> any value in the guest's config data. A migration stream not having any
>>> value for that feature on B should set it to "false".
>>>
>>> When creating a guest on B it will either have the feature value set
>>> explicitly in the guest config (either true or false), or it will get
>>> the server's default (this value should be configurable in a global
>>> config file, default for that global value would be "true").
>>>
>>> So with the guest created on B with feature specified as "false" (either
>>> for this guest only, or per global config), it will be migratable to
>>> machine A without problem. Migrating it back to B would work the same
>>> way as above. Trying to migrate a guest with feature set to "true" to
>>> B would not work, but this would be the host admin's fault due to not
>>> configuring the guest correctly.
> so the expected changes would be
> 
> 1. remove feature-ctrl-ring & feature-dynamic-multicast-control from 
> netback_probe( )
> 2. consume the backend Xenstore nodes in connect( ) to see if Xen tools 
> set nodes(true/false) or not(unknown)

Yes. I think this is the way to go.


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]