| Message ID | 20210311225944.24198-1-andyhsu@amazon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [net-next,1/2] xen-netback: add module parameter to disable ctrl-ring | expand |
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>
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
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
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
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
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
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 > >
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?
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 >
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 >
> > 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
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
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 > > >
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
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
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
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
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 > > > >
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
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);
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(-)