Message ID | 1547795385-12354-1-git-send-email-vasundhara-v.volam@broadcom.com |
---|---|
Headers | show |
Series | devlink: Add configuration parameters support for devlink_port | expand |
On Fri, Jan 18, 2019 at 12:39:37PM +0530, Vasundhara Volam wrote: > There is difference of opinion on adding WOL parameter to devlink, between > Jakub Kicinski and Michael Chan. > > Quote from Jakud Kicinski: > ******** > As explained previously I think it's a very bad idea to add existing > configuration options to devlink, just because devlink has the ability > to persist the setting in NVM. Especially that for WoL you have to get > the link up so you potentially have all link config stuff as well. And > that n-tuple filters are one of the WoL options, meaning we'd need the > ability to persist n-tuple filters via devlink. > > The effort would be far better spent helping with migrating ethtool to > netlink, and allowing persisting there. > > I have not heard any reason why devlink is a better fit. I can imagine > you're just doing it here because it's less effort for you since > ethtool is not yet migrated. > ******** > > Quote from Michael Chan: > ******** > The devlink's WoL parameter is a persistent WoL parameter stored in the > NIC's NVRAM. It is different from ethtool's WoL parameter in a number of > ways. ethtool WoL is not persistent over AC power cycle and is considered > OS-present WoL. As such, ethtool WoL can use a more sophisticated pattern > including n-tuple with IP address in addition to the more basic types > (e.g. magic packet). Whereas OS-absent power up WoL should only include > magic packet and other simple types. If I understand correctly, it's that way now. I'm not sure there is a technical reason preventing more complex WoL types in the OS-absent case in the future. Also, even with traditional ethtool WoL setting, most NICs only support some of the types (I'm not sure if there is a NIC which would support all of them.) > The devlink WoL setting does not have to match the ethtool WoL > setting. IMHO this is not really a problem. We can either use an additional flag telling kernel/driver if we are setting runtime or persistent WoL mask or we can pass (up to) two bitmaps. > The card will autoneg up to the speed supported by Vaux so no special > devlink link setting is needed. > ******** Like Jakub, I'm not convinced there is a strong technical reason to have each of the WoL settings handled through a different interface. I don't say, though, that ethtool is necessarily the right one. If there is a consensus that it better fits into devlink, I can imagine that both could be accessible through devlink (for start, in drivers which choose so, e.g. because they want to implement the persistent setting). Michal Kubecek
On Fri, 18 Jan 2019 15:33:19 +0100, Michal Kubecek wrote: > On Fri, Jan 18, 2019 at 12:39:37PM +0530, Vasundhara Volam wrote: > > There is difference of opinion on adding WOL parameter to devlink, between > > Jakub Kicinski and Michael Chan. > > > > Quote from Jakud Kicinski: > > ******** > > As explained previously I think it's a very bad idea to add existing > > configuration options to devlink, just because devlink has the ability > > to persist the setting in NVM. Especially that for WoL you have to get > > the link up so you potentially have all link config stuff as well. And > > that n-tuple filters are one of the WoL options, meaning we'd need the > > ability to persist n-tuple filters via devlink. > > > > The effort would be far better spent helping with migrating ethtool to > > netlink, and allowing persisting there. > > > > I have not heard any reason why devlink is a better fit. I can imagine > > you're just doing it here because it's less effort for you since > > ethtool is not yet migrated. > > ******** > > > > Quote from Michael Chan: > > ******** > > The devlink's WoL parameter is a persistent WoL parameter stored in the > > NIC's NVRAM. It is different from ethtool's WoL parameter in a number of > > ways. ethtool WoL is not persistent over AC power cycle and is considered > > OS-present WoL. As such, ethtool WoL can use a more sophisticated pattern > > including n-tuple with IP address in addition to the more basic types > > (e.g. magic packet). Whereas OS-absent power up WoL should only include > > magic packet and other simple types. > > If I understand correctly, it's that way now. I'm not sure there is a > technical reason preventing more complex WoL types in the OS-absent case > in the future. Also, even with traditional ethtool WoL setting, most > NICs only support some of the types (I'm not sure if there is a NIC > which would support all of them.) > > > The devlink WoL setting does not have to match the ethtool WoL > > setting. > > IMHO this is not really a problem. We can either use an additional flag > telling kernel/driver if we are setting runtime or persistent WoL mask > or we can pass (up to) two bitmaps. I think reusing new netlink ethtool with a special flag would be a nice, complete solution. We could also address link settings this way (which are a pre-requisite for WoL). I have no strong preference on the mechanism, but for ease of setting as well as dumping would it be workable to use a nesting, like this: Run time settings: [ETHTOOL_SETTINGS_BLA] [ETHTOOL_BLA_VAL_1] [ETHTOOL_BLA_VAL_2] ... Persistent: [ETHTOOL_PERSISTENT] [ETHTOOL_SETTINGS_BLA] [ETHTOOL_BLA_VAL_1] [ETHTOOL_BLA_VAL_2] IOW encapsulate settings into a "persistent" attribute? How does that look to you, Michal? > > The card will autoneg up to the speed supported by Vaux so no special > > devlink link setting is needed. > > ******** > > Like Jakub, I'm not convinced there is a strong technical reason to have > each of the WoL settings handled through a different interface. I don't > say, though, that ethtool is necessarily the right one. If there is > a consensus that it better fits into devlink, I can imagine that both > could be accessible through devlink (for start, in drivers which choose > so, e.g. because they want to implement the persistent setting). > > Michal Kubecek
On Fri, Jan 18, 2019 at 8:03 PM Michal Kubecek <mkubecek@suse.cz> wrote: > > On Fri, Jan 18, 2019 at 12:39:37PM +0530, Vasundhara Volam wrote: > > There is difference of opinion on adding WOL parameter to devlink, between > > Jakub Kicinski and Michael Chan. > > > > Quote from Jakud Kicinski: > > ******** > > As explained previously I think it's a very bad idea to add existing > > configuration options to devlink, just because devlink has the ability > > to persist the setting in NVM. Especially that for WoL you have to get > > the link up so you potentially have all link config stuff as well. And > > that n-tuple filters are one of the WoL options, meaning we'd need the > > ability to persist n-tuple filters via devlink. > > > > The effort would be far better spent helping with migrating ethtool to > > netlink, and allowing persisting there. > > > > I have not heard any reason why devlink is a better fit. I can imagine > > you're just doing it here because it's less effort for you since > > ethtool is not yet migrated. > > ******** > > > > Quote from Michael Chan: > > ******** > > The devlink's WoL parameter is a persistent WoL parameter stored in the > > NIC's NVRAM. It is different from ethtool's WoL parameter in a number of > > ways. ethtool WoL is not persistent over AC power cycle and is considered > > OS-present WoL. As such, ethtool WoL can use a more sophisticated pattern > > including n-tuple with IP address in addition to the more basic types > > (e.g. magic packet). Whereas OS-absent power up WoL should only include > > magic packet and other simple types. > > If I understand correctly, it's that way now. I'm not sure there is a > technical reason preventing more complex WoL types in the OS-absent case > in the future. Also, even with traditional ethtool WoL setting, most > NICs only support some of the types (I'm not sure if there is a NIC > which would support all of them.) Agreed. This can be extended in future for other types of WoL. > > > The devlink WoL setting does not have to match the ethtool WoL > > setting. > > IMHO this is not really a problem. We can either use an additional flag > telling kernel/driver if we are setting runtime or persistent WoL mask > or we can pass (up to) two bitmaps. This already supports as persistent or runtime WoL. All devlink parameters have this option to be either persistent or runtime which is defined by the driver when defining the parameter. > > > The card will autoneg up to the speed supported by Vaux so no special > > devlink link setting is needed. > > ******** > > Like Jakub, I'm not convinced there is a strong technical reason to have > each of the WoL settings handled through a different interface. I don't > say, though, that ethtool is necessarily the right one. If there is > a consensus that it better fits into devlink, I can imagine that both > could be accessible through devlink (for start, in drivers which choose > so, e.g. because they want to implement the persistent setting). Devlink supports both persistent and runtime WoL setting. It depends on driver's use case. > > Michal Kubecek
On Wed, Jan 23, 2019 at 3:48 AM Jakub Kicinski <jakub.kicinski@netronome.com> wrote: > > On Fri, 18 Jan 2019 15:33:19 +0100, Michal Kubecek wrote: > > On Fri, Jan 18, 2019 at 12:39:37PM +0530, Vasundhara Volam wrote: > > > There is difference of opinion on adding WOL parameter to devlink, between > > > Jakub Kicinski and Michael Chan. > > > > > > Quote from Jakud Kicinski: > > > ******** > > > As explained previously I think it's a very bad idea to add existing > > > configuration options to devlink, just because devlink has the ability > > > to persist the setting in NVM. Especially that for WoL you have to get > > > the link up so you potentially have all link config stuff as well. And > > > that n-tuple filters are one of the WoL options, meaning we'd need the > > > ability to persist n-tuple filters via devlink. > > > > > > The effort would be far better spent helping with migrating ethtool to > > > netlink, and allowing persisting there. > > > > > > I have not heard any reason why devlink is a better fit. I can imagine > > > you're just doing it here because it's less effort for you since > > > ethtool is not yet migrated. > > > ******** > > > > > > Quote from Michael Chan: > > > ******** > > > The devlink's WoL parameter is a persistent WoL parameter stored in the > > > NIC's NVRAM. It is different from ethtool's WoL parameter in a number of > > > ways. ethtool WoL is not persistent over AC power cycle and is considered > > > OS-present WoL. As such, ethtool WoL can use a more sophisticated pattern > > > including n-tuple with IP address in addition to the more basic types > > > (e.g. magic packet). Whereas OS-absent power up WoL should only include > > > magic packet and other simple types. > > > > If I understand correctly, it's that way now. I'm not sure there is a > > technical reason preventing more complex WoL types in the OS-absent case > > in the future. Also, even with traditional ethtool WoL setting, most > > NICs only support some of the types (I'm not sure if there is a NIC > > which would support all of them.) > > > > > The devlink WoL setting does not have to match the ethtool WoL > > > setting. > > > > IMHO this is not really a problem. We can either use an additional flag > > telling kernel/driver if we are setting runtime or persistent WoL mask > > or we can pass (up to) two bitmaps. > > I think reusing new netlink ethtool with a special flag would be a nice, > complete solution. We could also address link settings this way (which > are a pre-requisite for WoL). > > I have no strong preference on the mechanism, but for ease of setting > as well as dumping would it be workable to use a nesting, like this: > > Run time settings: > [ETHTOOL_SETTINGS_BLA] > [ETHTOOL_BLA_VAL_1] > [ETHTOOL_BLA_VAL_2] > ... > > Persistent: > [ETHTOOL_PERSISTENT] > [ETHTOOL_SETTINGS_BLA] > [ETHTOOL_BLA_VAL_1] > [ETHTOOL_BLA_VAL_2] > > IOW encapsulate settings into a "persistent" attribute? Not sure if current devlink framework allows to encapsulate additional settings now. But we can think of extending it to support this when there is a requirement. > > How does that look to you, Michal? > > > > The card will autoneg up to the speed supported by Vaux so no special > > > devlink link setting is needed. > > > ******** > > > > Like Jakub, I'm not convinced there is a strong technical reason to have > > each of the WoL settings handled through a different interface. I don't > > say, though, that ethtool is necessarily the right one. If there is > > a consensus that it better fits into devlink, I can imagine that both > > could be accessible through devlink (for start, in drivers which choose > > so, e.g. because they want to implement the persistent setting). > > > > Michal Kubecek
On Thu, 24 Jan 2019 15:16:27 +0530, Vasundhara Volam wrote: > > > > The devlink WoL setting does not have to match the ethtool WoL > > > > setting. > > > > > > IMHO this is not really a problem. We can either use an additional flag > > > telling kernel/driver if we are setting runtime or persistent WoL mask > > > or we can pass (up to) two bitmaps. > > > > I think reusing new netlink ethtool with a special flag would be a nice, > > complete solution. We could also address link settings this way (which > > are a pre-requisite for WoL). > > > > I have no strong preference on the mechanism, but for ease of setting > > as well as dumping would it be workable to use a nesting, like this: > > > > Run time settings: > > [ETHTOOL_SETTINGS_BLA] > > [ETHTOOL_BLA_VAL_1] > > [ETHTOOL_BLA_VAL_2] > > ... > > > > Persistent: > > [ETHTOOL_PERSISTENT] > > [ETHTOOL_SETTINGS_BLA] > > [ETHTOOL_BLA_VAL_1] > > [ETHTOOL_BLA_VAL_2] > > > > IOW encapsulate settings into a "persistent" attribute? > Not sure if current devlink framework allows to encapsulate additional > settings now. > But we can think of extending it to support this when there is a requirement. To be clear the question was to Michal and about ethtool netlink, where this configuration belongs.
On Tue, Jan 22, 2019 at 02:18:42PM -0800, Jakub Kicinski wrote: > > I think reusing new netlink ethtool with a special flag would be a nice, > complete solution. We could also address link settings this way (which > are a pre-requisite for WoL). > > I have no strong preference on the mechanism, but for ease of setting > as well as dumping would it be workable to use a nesting, like this: > > Run time settings: > [ETHTOOL_SETTINGS_BLA] > [ETHTOOL_BLA_VAL_1] > [ETHTOOL_BLA_VAL_2] > ... > > Persistent: > [ETHTOOL_PERSISTENT] > [ETHTOOL_SETTINGS_BLA] > [ETHTOOL_BLA_VAL_1] > [ETHTOOL_BLA_VAL_2] > > IOW encapsulate settings into a "persistent" attribute? > > How does that look to you, Michal? It's certainly one of the options worth considering. What I like is that this approach would not require knowing (or rather guessing) which attributes are going to allow persistent settings in the future. Michal
From: Michal Kubecek <mkubecek@suse.cz> Date: Mon, 28 Jan 2019 00:07:30 +0100 > What I like is that this approach would not require knowing (or > rather guessing) which attributes are going to allow persistent > settings in the future. +1
On Wed, Jan 23, 2019 at 3:48 AM Jakub Kicinski <jakub.kicinski@netronome.com> wrote: > > On Fri, 18 Jan 2019 15:33:19 +0100, Michal Kubecek wrote: > > On Fri, Jan 18, 2019 at 12:39:37PM +0530, Vasundhara Volam wrote: > > > There is difference of opinion on adding WOL parameter to devlink, between > > > Jakub Kicinski and Michael Chan. > > > > > > Quote from Jakud Kicinski: > > > ******** > > > As explained previously I think it's a very bad idea to add existing > > > configuration options to devlink, just because devlink has the ability > > > to persist the setting in NVM. Especially that for WoL you have to get > > > the link up so you potentially have all link config stuff as well. And > > > that n-tuple filters are one of the WoL options, meaning we'd need the > > > ability to persist n-tuple filters via devlink. > > > > > > The effort would be far better spent helping with migrating ethtool to > > > netlink, and allowing persisting there. > > > > > > I have not heard any reason why devlink is a better fit. I can imagine > > > you're just doing it here because it's less effort for you since > > > ethtool is not yet migrated. > > > ******** > > > > > > Quote from Michael Chan: > > > ******** > > > The devlink's WoL parameter is a persistent WoL parameter stored in the > > > NIC's NVRAM. It is different from ethtool's WoL parameter in a number of > > > ways. ethtool WoL is not persistent over AC power cycle and is considered > > > OS-present WoL. As such, ethtool WoL can use a more sophisticated pattern > > > including n-tuple with IP address in addition to the more basic types > > > (e.g. magic packet). Whereas OS-absent power up WoL should only include > > > magic packet and other simple types. > > > > If I understand correctly, it's that way now. I'm not sure there is a > > technical reason preventing more complex WoL types in the OS-absent case > > in the future. Also, even with traditional ethtool WoL setting, most > > NICs only support some of the types (I'm not sure if there is a NIC > > which would support all of them.) > > > > > The devlink WoL setting does not have to match the ethtool WoL > > > setting. > > > > IMHO this is not really a problem. We can either use an additional flag > > telling kernel/driver if we are setting runtime or persistent WoL mask > > or we can pass (up to) two bitmaps. Jakub, I will add another two bitmask parameters for WoL named as wake_on_lan_runtime and wake_on_lan_persistent. This will give information about what types are runtime and what types are persistent, does the device support for any given WoL types. Does that sound good? > > I think reusing new netlink ethtool with a special flag would be a nice, > complete solution. We could also address link settings this way (which > are a pre-requisite for WoL). > > I have no strong preference on the mechanism, but for ease of setting > as well as dumping would it be workable to use a nesting, like this: > > Run time settings: > [ETHTOOL_SETTINGS_BLA] > [ETHTOOL_BLA_VAL_1] > [ETHTOOL_BLA_VAL_2] > ... > > Persistent: > [ETHTOOL_PERSISTENT] > [ETHTOOL_SETTINGS_BLA] > [ETHTOOL_BLA_VAL_1] > [ETHTOOL_BLA_VAL_2] > > IOW encapsulate settings into a "persistent" attribute? > > How does that look to you, Michal? > > > > The card will autoneg up to the speed supported by Vaux so no special > > > devlink link setting is needed. > > > ******** > > > > Like Jakub, I'm not convinced there is a strong technical reason to have > > each of the WoL settings handled through a different interface. I don't > > say, though, that ethtool is necessarily the right one. If there is > > a consensus that it better fits into devlink, I can imagine that both > > could be accessible through devlink (for start, in drivers which choose > > so, e.g. because they want to implement the persistent setting). > > > > Michal Kubecek
On Mon, 4 Feb 2019 12:25:13 +0530, Vasundhara Volam wrote: > > > IMHO this is not really a problem. We can either use an additional flag > > > telling kernel/driver if we are setting runtime or persistent WoL mask > > > or we can pass (up to) two bitmaps. > Jakub, I will add another two bitmask parameters for WoL named as > wake_on_lan_runtime > and wake_on_lan_persistent. This will give information about what > types are runtime and > what types are persistent, does the device support for any given WoL types. > > Does that sound good? No? We were talking about using the soon-too-come ethtool netlink API with additional indication that given configuration request is supposed to be persisted. Adding more devlink parameters is exactly the opposite of what you should be doing.
On Tue, Feb 5, 2019 at 8:26 AM Jakub Kicinski <jakub.kicinski@netronome.com> wrote: > > On Mon, 4 Feb 2019 12:25:13 +0530, Vasundhara Volam wrote: > > > > IMHO this is not really a problem. We can either use an additional flag > > > > telling kernel/driver if we are setting runtime or persistent WoL mask > > > > or we can pass (up to) two bitmaps. > > Jakub, I will add another two bitmask parameters for WoL named as > > wake_on_lan_runtime > > and wake_on_lan_persistent. This will give information about what > > types are runtime and > > what types are persistent, does the device support for any given WoL types. > > > > Does that sound good? > > No? We were talking about using the soon-too-come ethtool netlink > API with additional indication that given configuration request is > supposed to be persisted. Adding more devlink parameters is exactly > the opposite of what you should be doing. Okay. So, till then can we have the devlink wake_on_lan parameter or you want this to be removed? Could you please clarify? Once ethtool netlink API is available with persisted support, I can remove this wake_on_lan parameter from devlink. Thanks.
On Tue, Feb 05, 2019 at 09:53:26AM +0530, Vasundhara Volam wrote: > On Tue, Feb 5, 2019 at 8:26 AM Jakub Kicinski > > > > No? We were talking about using the soon-too-come ethtool netlink > > API with additional indication that given configuration request is > > supposed to be persisted. Adding more devlink parameters is exactly > > the opposite of what you should be doing. > > Okay. So, till then can we have the devlink wake_on_lan parameter or > you want this to be removed? Could you please clarify? > > Once ethtool netlink API is available with persisted support, I can remove > this wake_on_lan parameter from devlink. Thanks. Once you provide an interface for userspace and applications start using it, it's hard to get rid of it. As an extreme example, the legacy ioctl interface used by ifconfig has been declared obsolete since kernel 2.2.0 (January 1999, i.e. 20 years ago) and we still have to maintain it. Michal Kubecek
On Tue, Feb 5, 2019 at 10:21 PM Michal Kubecek <mkubecek@suse.cz> wrote: > > On Tue, Feb 05, 2019 at 09:53:26AM +0530, Vasundhara Volam wrote: > > On Tue, Feb 5, 2019 at 8:26 AM Jakub Kicinski > > > > > > No? We were talking about using the soon-too-come ethtool netlink > > > API with additional indication that given configuration request is > > > supposed to be persisted. Adding more devlink parameters is exactly > > > the opposite of what you should be doing. > > > > Okay. So, till then can we have the devlink wake_on_lan parameter or > > you want this to be removed? Could you please clarify? > > > > Once ethtool netlink API is available with persisted support, I can remove > > this wake_on_lan parameter from devlink. Thanks. > > Once you provide an interface for userspace and applications start using > it, it's hard to get rid of it. As an extreme example, the legacy ioctl > interface used by ifconfig has been declared obsolete since kernel 2.2.0 > (January 1999, i.e. 20 years ago) and we still have to maintain it. > Okay Got it. I will revert only the wake_on_lan parameter and send the patch. We will wait for soon-too-come ethtool netlink API. Thank you. > Michal Kubecek