| Message ID | 1507815262-33294-1-git-send-email-steven.lin1@broadcom.com |
|---|---|
| Headers | show |
| Series | Adding config get/set to devlink | expand |
On Thu, Oct 12, 2017 at 6:34 AM, Steve Lin <steven.lin1@broadcom.com> wrote: > Adds a devlink command for getting & setting device configuration > parameters, and enumerates a bunch of those parameters as devlink > attributes. Also introduces an attribute that can be set by a > driver to indicate that the config change doesn't take effect > until the next restart (as in the case of the bnxt driver changes > in this patchset, for which all the configuration changes affect NVM > only, and aren't loaded until the next restart.) > > bnxt driver patches make use of these new devlink cmds/attributes. > > Steve Lin (3): > devlink: Add config parameter get/set operations > bnxt: Move generic devlink code to new file > bnxt: Add devlink support for config get/set > Is the goal here to move all ethtool operations to devlink (I saw some attrs related to speed etc). ?. We do need to move ethtool attrs to netlink and devlink is a good place (and of-course leave the current ethtool api around for backward compatibility).
Thu, Oct 12, 2017 at 04:35:10PM CEST, roopa@cumulusnetworks.com wrote: >On Thu, Oct 12, 2017 at 6:34 AM, Steve Lin <steven.lin1@broadcom.com> wrote: >> Adds a devlink command for getting & setting device configuration >> parameters, and enumerates a bunch of those parameters as devlink >> attributes. Also introduces an attribute that can be set by a >> driver to indicate that the config change doesn't take effect >> until the next restart (as in the case of the bnxt driver changes >> in this patchset, for which all the configuration changes affect NVM >> only, and aren't loaded until the next restart.) >> >> bnxt driver patches make use of these new devlink cmds/attributes. >> >> Steve Lin (3): >> devlink: Add config parameter get/set operations >> bnxt: Move generic devlink code to new file >> bnxt: Add devlink support for config get/set >> > >Is the goal here to move all ethtool operations to devlink (I saw some >attrs related to speed etc). ?. >We do need to move ethtool attrs to netlink and devlink is a good >place (and of-course leave the current ethtool api around for backward >compatibility). We need to make sure we are not moving things to devlink which don't belong there. All options that use "netdev" as a handle should go into rtnetlink instead.
Hi Roopa, The attributes added in this patchset are not really the same type as ethtool - these are more device configuration type attributes. The speeds you saw, for example, affect the pre-OS [i.e. PXE boot time] configuration for a port, and aren't run-time speed changes on a given netdev like ethtool configures. As Jiri mentioned, I will add some comments to better describe each of the attributes. So I don't think there's much duplication here with ethtool. That said, there also shouldn't be anything in the patchset that would preclude some future migration of ethtool settings to using devlink or rtnetlink API. Steve On Thu, Oct 12, 2017 at 10:35 AM, Roopa Prabhu <roopa@cumulusnetworks.com> wrote: > On Thu, Oct 12, 2017 at 6:34 AM, Steve Lin <steven.lin1@broadcom.com> wrote: >> Adds a devlink command for getting & setting device configuration >> parameters, and enumerates a bunch of those parameters as devlink >> attributes. Also introduces an attribute that can be set by a >> driver to indicate that the config change doesn't take effect >> until the next restart (as in the case of the bnxt driver changes >> in this patchset, for which all the configuration changes affect NVM >> only, and aren't loaded until the next restart.) >> >> bnxt driver patches make use of these new devlink cmds/attributes. >> >> Steve Lin (3): >> devlink: Add config parameter get/set operations >> bnxt: Move generic devlink code to new file >> bnxt: Add devlink support for config get/set >> > > Is the goal here to move all ethtool operations to devlink (I saw some > attrs related to speed etc). ?. > We do need to move ethtool attrs to netlink and devlink is a good > place (and of-course leave the current ethtool api around for backward > compatibility).
On Thu, Oct 12, 2017 at 7:40 AM, Jiri Pirko <jiri@resnulli.us> wrote: > Thu, Oct 12, 2017 at 04:35:10PM CEST, roopa@cumulusnetworks.com wrote: >>On Thu, Oct 12, 2017 at 6:34 AM, Steve Lin <steven.lin1@broadcom.com> wrote: >>> Adds a devlink command for getting & setting device configuration >>> parameters, and enumerates a bunch of those parameters as devlink >>> attributes. Also introduces an attribute that can be set by a >>> driver to indicate that the config change doesn't take effect >>> until the next restart (as in the case of the bnxt driver changes >>> in this patchset, for which all the configuration changes affect NVM >>> only, and aren't loaded until the next restart.) >>> >>> bnxt driver patches make use of these new devlink cmds/attributes. >>> >>> Steve Lin (3): >>> devlink: Add config parameter get/set operations >>> bnxt: Move generic devlink code to new file >>> bnxt: Add devlink support for config get/set >>> >> >>Is the goal here to move all ethtool operations to devlink (I saw some >>attrs related to speed etc). ?. >>We do need to move ethtool attrs to netlink and devlink is a good >>place (and of-course leave the current ethtool api around for backward >>compatibility). > > We need to make sure we are not moving things to devlink which don't > belong there. All options that use "netdev" as a handle should go into > rtnetlink instead. > Any reason you want to keep that restriction ?. FWIS, devlink is a driver api just like ethtool is. and ethtool needs to move to netlink soon...and It would be better to not put the rtnl_lock burden on ethtool driver operations. Instead of adding yet another driver api, extending devlink seems like a great fit to me.
On Thu, Oct 12, 2017 at 7:45 AM, Steve Lin <steven.lin1@broadcom.com> wrote: > Hi Roopa, > > The attributes added in this patchset are not really the same type as > ethtool - these are more device configuration type attributes. The > speeds you saw, for example, affect the pre-OS [i.e. PXE boot time] > configuration for a port, and aren't run-time speed changes on a given > netdev like ethtool configures. As Jiri mentioned, I will add some > comments to better describe each of the attributes. > > So I don't think there's much duplication here with ethtool. > > That said, there also shouldn't be anything in the patchset that would > preclude some future migration of ethtool settings to using devlink or > rtnetlink API. > ok, ack, thanks for the clarification. Just trying to find a best netlink place for future ethtool migration to netlink.
Thu, Oct 12, 2017 at 04:46:24PM CEST, roopa@cumulusnetworks.com wrote: >On Thu, Oct 12, 2017 at 7:40 AM, Jiri Pirko <jiri@resnulli.us> wrote: >> Thu, Oct 12, 2017 at 04:35:10PM CEST, roopa@cumulusnetworks.com wrote: >>>On Thu, Oct 12, 2017 at 6:34 AM, Steve Lin <steven.lin1@broadcom.com> wrote: >>>> Adds a devlink command for getting & setting device configuration >>>> parameters, and enumerates a bunch of those parameters as devlink >>>> attributes. Also introduces an attribute that can be set by a >>>> driver to indicate that the config change doesn't take effect >>>> until the next restart (as in the case of the bnxt driver changes >>>> in this patchset, for which all the configuration changes affect NVM >>>> only, and aren't loaded until the next restart.) >>>> >>>> bnxt driver patches make use of these new devlink cmds/attributes. >>>> >>>> Steve Lin (3): >>>> devlink: Add config parameter get/set operations >>>> bnxt: Move generic devlink code to new file >>>> bnxt: Add devlink support for config get/set >>>> >>> >>>Is the goal here to move all ethtool operations to devlink (I saw some >>>attrs related to speed etc). ?. >>>We do need to move ethtool attrs to netlink and devlink is a good >>>place (and of-course leave the current ethtool api around for backward >>>compatibility). >> >> We need to make sure we are not moving things to devlink which don't >> belong there. All options that use "netdev" as a handle should go into >> rtnetlink instead. >> > >Any reason you want to keep that restriction ?. >FWIS, devlink is a driver api just like ethtool is. >and ethtool needs to move to netlink soon...and It would be better to >not put the rtnl_lock burden on ethtool driver operations. Instead of >adding yet another driver api, extending devlink seems like a great >fit to me. Hmm, the original purpose of devlink was to obtain iface for things that could not use "netdev" as a handle. I try to stick with it as we already have iface for things that could use "netdev" as a handle - rtnetlink. Not sure we want to go this way and add "netdev"-handle things into devlink. Thoughts?
On Thu, Oct 12, 2017 at 8:04 AM, Jiri Pirko <jiri@resnulli.us> wrote: > Thu, Oct 12, 2017 at 04:46:24PM CEST, roopa@cumulusnetworks.com wrote: >>On Thu, Oct 12, 2017 at 7:40 AM, Jiri Pirko <jiri@resnulli.us> wrote: >>> Thu, Oct 12, 2017 at 04:35:10PM CEST, roopa@cumulusnetworks.com wrote: >>>>On Thu, Oct 12, 2017 at 6:34 AM, Steve Lin <steven.lin1@broadcom.com> wrote: >>>>> Adds a devlink command for getting & setting device configuration >>>>> parameters, and enumerates a bunch of those parameters as devlink >>>>> attributes. Also introduces an attribute that can be set by a >>>>> driver to indicate that the config change doesn't take effect >>>>> until the next restart (as in the case of the bnxt driver changes >>>>> in this patchset, for which all the configuration changes affect NVM >>>>> only, and aren't loaded until the next restart.) >>>>> >>>>> bnxt driver patches make use of these new devlink cmds/attributes. >>>>> >>>>> Steve Lin (3): >>>>> devlink: Add config parameter get/set operations >>>>> bnxt: Move generic devlink code to new file >>>>> bnxt: Add devlink support for config get/set >>>>> >>>> >>>>Is the goal here to move all ethtool operations to devlink (I saw some >>>>attrs related to speed etc). ?. >>>>We do need to move ethtool attrs to netlink and devlink is a good >>>>place (and of-course leave the current ethtool api around for backward >>>>compatibility). >>> >>> We need to make sure we are not moving things to devlink which don't >>> belong there. All options that use "netdev" as a handle should go into >>> rtnetlink instead. >>> >> >>Any reason you want to keep that restriction ?. >>FWIS, devlink is a driver api just like ethtool is. >>and ethtool needs to move to netlink soon...and It would be better to >>not put the rtnl_lock burden on ethtool driver operations. Instead of >>adding yet another driver api, extending devlink seems like a great >>fit to me. > > Hmm, the original purpose of devlink was to obtain iface for things that > could not use "netdev" as a handle. I try to stick with it as we already > have iface for things that could use "netdev" as a handle - rtnetlink. > > Not sure we want to go this way and add "netdev"-handle things into > devlink. Thoughts? > Only motivation for me is to keep all driver/hw api in a single place. and its high time ethtool moved to netlink. I would prefer it be out of rtnetlink if we have a choice. Moving some of the driver ops to rtnetlink and leaving the rest in devlink can be a mess for drivers in the long run. Maybe we can discuss this more at netdev2.2 ?
On October 12, 2017 8:04:19 AM PDT, Jiri Pirko <jiri@resnulli.us> wrote: >Thu, Oct 12, 2017 at 04:46:24PM CEST, roopa@cumulusnetworks.com wrote: >>On Thu, Oct 12, 2017 at 7:40 AM, Jiri Pirko <jiri@resnulli.us> wrote: >>> Thu, Oct 12, 2017 at 04:35:10PM CEST, roopa@cumulusnetworks.com >wrote: >>>>On Thu, Oct 12, 2017 at 6:34 AM, Steve Lin ><steven.lin1@broadcom.com> wrote: >>>>> Adds a devlink command for getting & setting device configuration >>>>> parameters, and enumerates a bunch of those parameters as devlink >>>>> attributes. Also introduces an attribute that can be set by a >>>>> driver to indicate that the config change doesn't take effect >>>>> until the next restart (as in the case of the bnxt driver changes >>>>> in this patchset, for which all the configuration changes affect >NVM >>>>> only, and aren't loaded until the next restart.) >>>>> >>>>> bnxt driver patches make use of these new devlink cmds/attributes. >>>>> >>>>> Steve Lin (3): >>>>> devlink: Add config parameter get/set operations >>>>> bnxt: Move generic devlink code to new file >>>>> bnxt: Add devlink support for config get/set >>>>> >>>> >>>>Is the goal here to move all ethtool operations to devlink (I saw >some >>>>attrs related to speed etc). ?. >>>>We do need to move ethtool attrs to netlink and devlink is a good >>>>place (and of-course leave the current ethtool api around for >backward >>>>compatibility). >>> >>> We need to make sure we are not moving things to devlink which don't >>> belong there. All options that use "netdev" as a handle should go >into >>> rtnetlink instead. >>> >> >>Any reason you want to keep that restriction ?. >>FWIS, devlink is a driver api just like ethtool is. >>and ethtool needs to move to netlink soon...and It would be better to >>not put the rtnl_lock burden on ethtool driver operations. Instead of >>adding yet another driver api, extending devlink seems like a great >>fit to me. > >Hmm, the original purpose of devlink was to obtain iface for things >that >could not use "netdev" as a handle. I try to stick with it as we >already >have iface for things that could use "netdev" as a handle - rtnetlink. > >Not sure we want to go this way and add "netdev"-handle things into >devlink. Thoughts? In the current situation where we have ethtool and devlink operating separately on different objects as entry points to the kernel, I agree with that design. Once we move ethtool (or however we name its successor) over to netlink there is an opportunity for accessing objects that do and do not have a netdevice representor today (e.g: management ports on switches) with the same interface, and devlink could be used for that. In terms of compatibility though we should have a pseudo generic layer that can take ethtool ioctl() and transform that into a netlink message and then call back down to drivers with the existing ethtool_ops that are implemented. It is reasonably simple to use coccinelle to update these ethtool_ops with possibly updated signatures to support netlink attributes and whatnot, but forcing drivers to quit doing ethtool_ops entitely and implement new sets of "ethtool over netlink" ops is a non starter IMHO.
On Thu, Oct 12, 2017 at 8:43 AM, Florian Fainelli <f.fainelli@gmail.com> wrote: > On October 12, 2017 8:04:19 AM PDT, Jiri Pirko <jiri@resnulli.us> wrote: >>Thu, Oct 12, 2017 at 04:46:24PM CEST, roopa@cumulusnetworks.com wrote: >>>On Thu, Oct 12, 2017 at 7:40 AM, Jiri Pirko <jiri@resnulli.us> wrote: >>>> Thu, Oct 12, 2017 at 04:35:10PM CEST, roopa@cumulusnetworks.com >>wrote: >>>>>On Thu, Oct 12, 2017 at 6:34 AM, Steve Lin >><steven.lin1@broadcom.com> wrote: >>>>>> Adds a devlink command for getting & setting device configuration >>>>>> parameters, and enumerates a bunch of those parameters as devlink >>>>>> attributes. Also introduces an attribute that can be set by a >>>>>> driver to indicate that the config change doesn't take effect >>>>>> until the next restart (as in the case of the bnxt driver changes >>>>>> in this patchset, for which all the configuration changes affect >>NVM >>>>>> only, and aren't loaded until the next restart.) >>>>>> >>>>>> bnxt driver patches make use of these new devlink cmds/attributes. >>>>>> >>>>>> Steve Lin (3): >>>>>> devlink: Add config parameter get/set operations >>>>>> bnxt: Move generic devlink code to new file >>>>>> bnxt: Add devlink support for config get/set >>>>>> >>>>> >>>>>Is the goal here to move all ethtool operations to devlink (I saw >>some >>>>>attrs related to speed etc). ?. >>>>>We do need to move ethtool attrs to netlink and devlink is a good >>>>>place (and of-course leave the current ethtool api around for >>backward >>>>>compatibility). >>>> >>>> We need to make sure we are not moving things to devlink which don't >>>> belong there. All options that use "netdev" as a handle should go >>into >>>> rtnetlink instead. >>>> >>> >>>Any reason you want to keep that restriction ?. >>>FWIS, devlink is a driver api just like ethtool is. >>>and ethtool needs to move to netlink soon...and It would be better to >>>not put the rtnl_lock burden on ethtool driver operations. Instead of >>>adding yet another driver api, extending devlink seems like a great >>>fit to me. >> >>Hmm, the original purpose of devlink was to obtain iface for things >>that >>could not use "netdev" as a handle. I try to stick with it as we >>already >>have iface for things that could use "netdev" as a handle - rtnetlink. >> >>Not sure we want to go this way and add "netdev"-handle things into >>devlink. Thoughts? > > In the current situation where we have ethtool and devlink operating separately on different objects as entry points to the kernel, I agree with that design. > > Once we move ethtool (or however we name its successor) over to netlink there is an opportunity for accessing objects that do and do not have a netdevice representor today (e.g: management ports on switches) with the same interface, and devlink could be used for that. > > In terms of compatibility though we should have a pseudo generic layer that can take ethtool ioctl() and transform that into a netlink message and then call back down to drivers with the existing ethtool_ops that are implemented. It is reasonably simple to use coccinelle to update these ethtool_ops with possibly updated signatures to support netlink attributes and whatnot, ack, that sounds like a good approach. > but forcing drivers to quit doing ethtool_ops entitely and implement new sets of "ethtool over netlink" ops is a non starter IMHO. correct, nobody disagrees with that point.
From: Jiri Pirko <jiri@resnulli.us> Date: Thu, 12 Oct 2017 16:40:32 +0200 > Thu, Oct 12, 2017 at 04:35:10PM CEST, roopa@cumulusnetworks.com wrote: >>On Thu, Oct 12, 2017 at 6:34 AM, Steve Lin <steven.lin1@broadcom.com> wrote: >>> Adds a devlink command for getting & setting device configuration >>> parameters, and enumerates a bunch of those parameters as devlink >>> attributes. Also introduces an attribute that can be set by a >>> driver to indicate that the config change doesn't take effect >>> until the next restart (as in the case of the bnxt driver changes >>> in this patchset, for which all the configuration changes affect NVM >>> only, and aren't loaded until the next restart.) >>> >>> bnxt driver patches make use of these new devlink cmds/attributes. >>> >>> Steve Lin (3): >>> devlink: Add config parameter get/set operations >>> bnxt: Move generic devlink code to new file >>> bnxt: Add devlink support for config get/set >>> >> >>Is the goal here to move all ethtool operations to devlink (I saw some >>attrs related to speed etc). ?. >>We do need to move ethtool attrs to netlink and devlink is a good >>place (and of-course leave the current ethtool api around for backward >>compatibility). > > We need to make sure we are not moving things to devlink which don't > belong there. All options that use "netdev" as a handle should go into > rtnetlink instead. I agree. Let's keep devlink what is was created for, which is situations where we don't have a netdev as the object to work upon.
From: Roopa Prabhu <roopa@cumulusnetworks.com> Date: Thu, 12 Oct 2017 07:46:24 -0700 > FWIS, devlink is a driver api just like ethtool is. Devlink a driver API for doing operations where we don't have a specific 'netdev' object to work upon. > and ethtool needs to move to netlink soon...and It would be better to > not put the rtnl_lock burden on ethtool driver operations. Instead of > adding yet another driver api, extending devlink seems like a great > fit to me. You can use genetlink and avoid RTNL. Also, Florian Westphal has been pushing the RTNL lock down into the actual rtnetlink operation implementations. So one does not have to avoid rtnetlink to avoid the RTNL mutex at all.
From: Jiri Pirko <jiri@resnulli.us> Date: Thu, 12 Oct 2017 17:04:19 +0200 > Not sure we want to go this way and add "netdev"-handle things into > devlink. Thoughts? I think we should avoid this and keep devlink to it's designed purpose.
From: Florian Fainelli <f.fainelli@gmail.com> Date: Thu, 12 Oct 2017 08:43:59 -0700 > Once we move ethtool (or however we name its successor) over to > netlink there is an opportunity for accessing objects that do and do > not have a netdevice representor today (e.g: management ports on > switches) with the same interface, and devlink could be used for > that. That is an interesting angle for including this in devlink. I'm not so sure what to do about this. One suggestion is that devlink is used for getting ethtool stats for objects lacking netdev representor's, and a new genetlink family is used for netdev based ethtool. I think it's important that we don't expand the scope of devlink beyond what it was originally designed for.
On 10/12/2017 12:06 PM, David Miller wrote: > From: Florian Fainelli <f.fainelli@gmail.com> > Date: Thu, 12 Oct 2017 08:43:59 -0700 > >> Once we move ethtool (or however we name its successor) over to >> netlink there is an opportunity for accessing objects that do and do >> not have a netdevice representor today (e.g: management ports on >> switches) with the same interface, and devlink could be used for >> that. > > That is an interesting angle for including this in devlink. > > I'm not so sure what to do about this. > > One suggestion is that devlink is used for getting ethtool stats for > objects lacking netdev representor's, and a new genetlink family is > used for netdev based ethtool. Right, I was also thinking along those lines that we we would have a new generic netlink family for ethtool to support ethtool over netlink. > > I think it's important that we don't expand the scope of devlink > beyond what it was originally designed for. It seems to me like devlink is well defined in what it is not for: it is not meant to be used for any object that is/has a net_device, but it is not well defined for what it can offer to these non network devices. For instance, we have a tremendous amount of operations that are extremely specific to its single user(s) such as mlx5 and mlxsw. For instance, I am not sure how the buffer reservation scheme can be generalized, and this is always the tricky part with a single user facility in that you try to generalize the best you can based on the HW you know. This is not a criticism or meant to be anything negative, this just happens to be the case, and we did not have anything better. So maybe the first thing is to clarify what devlink operations can and should be and what they are absolutely not allowed to cover. We should also clarify whether a generic set/get that Steven is proposing is something that we tolerate, or whether there should be specific function pointers implemented for each attribute, which would be more in line with what has been done thus far.
On Thu, Oct 12, 2017 at 3:20 PM, Florian Fainelli <f.fainelli@gmail.com> wrote: > On 10/12/2017 12:06 PM, David Miller wrote: >> From: Florian Fainelli <f.fainelli@gmail.com> >> Date: Thu, 12 Oct 2017 08:43:59 -0700 >> >>> Once we move ethtool (or however we name its successor) over to >>> netlink there is an opportunity for accessing objects that do and do >>> not have a netdevice representor today (e.g: management ports on >>> switches) with the same interface, and devlink could be used for >>> that. >> >> That is an interesting angle for including this in devlink. >> >> I'm not so sure what to do about this. >> >> One suggestion is that devlink is used for getting ethtool stats for >> objects lacking netdev representor's, and a new genetlink family is >> used for netdev based ethtool. > > Right, I was also thinking along those lines that we we would have a new > generic netlink family for ethtool to support ethtool over netlink. > >> >> I think it's important that we don't expand the scope of devlink >> beyond what it was originally designed for. > > It seems to me like devlink is well defined in what it is not for: it is > not meant to be used for any object that is/has a net_device, but it is > not well defined for what it can offer to these non network devices. For > instance, we have a tremendous amount of operations that are extremely > specific to its single user(s) such as mlx5 and mlxsw. > > For instance, I am not sure how the buffer reservation scheme can be > generalized, and this is always the tricky part with a single user > facility in that you try to generalize the best you can based on the HW > you know. This is not a criticism or meant to be anything negative, this > just happens to be the case, and we did not have anything better. > > So maybe the first thing is to clarify what devlink operations can and > should be and what they are absolutely not allowed to cover. We should > also clarify whether a generic set/get that Steven is proposing is > something that we tolerate, or whether there should be specific function > pointers implemented for each attribute, which would be more in line > with what has been done thus far. Hi Florian, Some of this is subjective, of course, but just to clarify, it did seem like implementing a new devlink_op function pointer per attribute might be more consistent with what's been done so far. But for code reuse purposes - i.e. to avoid replicating essentially the same function for each of the 30+ config attributes - I elected to just implement a single generic get and set devlink_op. Steve
On Thu, Oct 12, 2017 at 12:20:07PM -0700, Florian Fainelli wrote: > On 10/12/2017 12:06 PM, David Miller wrote: > > > > One suggestion is that devlink is used for getting ethtool stats for > > objects lacking netdev representor's, and a new genetlink family is > > used for netdev based ethtool. > > Right, I was also thinking along those lines that we we would have a new > generic netlink family for ethtool to support ethtool over netlink. This is what I plan to work on on next SUSE Hackweek in November. But I'm, of course, open to suggestions and I don't insist on this approach. Michal Kubecek
On Thu, Oct 12, 2017 at 12:20 PM, Florian Fainelli <f.fainelli@gmail.com> wrote: > On 10/12/2017 12:06 PM, David Miller wrote: >> From: Florian Fainelli <f.fainelli@gmail.com> >> Date: Thu, 12 Oct 2017 08:43:59 -0700 >> >>> Once we move ethtool (or however we name its successor) over to >>> netlink there is an opportunity for accessing objects that do and do >>> not have a netdevice representor today (e.g: management ports on >>> switches) with the same interface, and devlink could be used for >>> that. >> >> That is an interesting angle for including this in devlink. >> >> I'm not so sure what to do about this. >> >> One suggestion is that devlink is used for getting ethtool stats for >> objects lacking netdev representor's, and a new genetlink family is >> used for netdev based ethtool. > > Right, I was also thinking along those lines that we we would have a new > generic netlink family for ethtool to support ethtool over netlink. new api is fine by me. The reason for suggesting devlink was because some of the devlink port_* ops are close to ethtool ops that can operate on a port/netdev. eg split_port could be a netdev operation unless you want to split before the netdev is created. There are some ops in devlink which are global hw parameters and not specific to a port, those fit perfectly with devlinks original goal. > >> >> I think it's important that we don't expand the scope of devlink >> beyond what it was originally designed for. > > It seems to me like devlink is well defined in what it is not for: it is > not meant to be used for any object that is/has a net_device, but it is > not well defined for what it can offer to these non network devices. For > instance, we have a tremendous amount of operations that are extremely > specific to its single user(s) such as mlx5 and mlxsw. > > For instance, I am not sure how the buffer reservation scheme can be > generalized, and this is always the tricky part with a single user > facility in that you try to generalize the best you can based on the HW > you know. This is not a criticism or meant to be anything negative, this > just happens to be the case, and we did not have anything better. > > So maybe the first thing is to clarify what devlink operations can and > should be and what they are absolutely not allowed to cover. We should > also clarify whether a generic set/get that Steven is proposing is > something that we tolerate, or whether there should be specific function > pointers implemented for each attribute, which would be more in line > with what has been done thus far. > -- > Florian
Thu, Oct 12, 2017 at 10:12:31PM CEST, steven.lin1@broadcom.com wrote: >On Thu, Oct 12, 2017 at 3:20 PM, Florian Fainelli <f.fainelli@gmail.com> wrote: >> On 10/12/2017 12:06 PM, David Miller wrote: >>> From: Florian Fainelli <f.fainelli@gmail.com> >>> Date: Thu, 12 Oct 2017 08:43:59 -0700 >>> >>>> Once we move ethtool (or however we name its successor) over to >>>> netlink there is an opportunity for accessing objects that do and do >>>> not have a netdevice representor today (e.g: management ports on >>>> switches) with the same interface, and devlink could be used for >>>> that. >>> >>> That is an interesting angle for including this in devlink. >>> >>> I'm not so sure what to do about this. >>> >>> One suggestion is that devlink is used for getting ethtool stats for >>> objects lacking netdev representor's, and a new genetlink family is >>> used for netdev based ethtool. >> >> Right, I was also thinking along those lines that we we would have a new >> generic netlink family for ethtool to support ethtool over netlink. >> >>> >>> I think it's important that we don't expand the scope of devlink >>> beyond what it was originally designed for. >> >> It seems to me like devlink is well defined in what it is not for: it is >> not meant to be used for any object that is/has a net_device, but it is >> not well defined for what it can offer to these non network devices. For >> instance, we have a tremendous amount of operations that are extremely >> specific to its single user(s) such as mlx5 and mlxsw. >> >> For instance, I am not sure how the buffer reservation scheme can be >> generalized, and this is always the tricky part with a single user >> facility in that you try to generalize the best you can based on the HW >> you know. This is not a criticism or meant to be anything negative, this >> just happens to be the case, and we did not have anything better. >> >> So maybe the first thing is to clarify what devlink operations can and >> should be and what they are absolutely not allowed to cover. We should >> also clarify whether a generic set/get that Steven is proposing is >> something that we tolerate, or whether there should be specific function >> pointers implemented for each attribute, which would be more in line >> with what has been done thus far. > >Hi Florian, > >Some of this is subjective, of course, but just to clarify, it did >seem like implementing a new devlink_op function pointer per attribute >might be more consistent with what's been done so far. But for code >reuse purposes - i.e. to avoid replicating essentially the same >function for each of the 30+ config attributes - I elected to just >implement a single generic get and set devlink_op. Also, it this case, unlike any existing cmds, the config options are all permanent, written in hw. I think it is fine to have one set of get/set cmd to handle them all at once. Same family.
Thu, Oct 12, 2017 at 11:53:56PM CEST, roopa@cumulusnetworks.com wrote: >On Thu, Oct 12, 2017 at 12:20 PM, Florian Fainelli <f.fainelli@gmail.com> wrote: >> On 10/12/2017 12:06 PM, David Miller wrote: >>> From: Florian Fainelli <f.fainelli@gmail.com> >>> Date: Thu, 12 Oct 2017 08:43:59 -0700 >>> >>>> Once we move ethtool (or however we name its successor) over to >>>> netlink there is an opportunity for accessing objects that do and do >>>> not have a netdevice representor today (e.g: management ports on >>>> switches) with the same interface, and devlink could be used for >>>> that. >>> >>> That is an interesting angle for including this in devlink. >>> >>> I'm not so sure what to do about this. >>> >>> One suggestion is that devlink is used for getting ethtool stats for >>> objects lacking netdev representor's, and a new genetlink family is >>> used for netdev based ethtool. >> >> Right, I was also thinking along those lines that we we would have a new >> generic netlink family for ethtool to support ethtool over netlink. > >new api is fine by me. The reason for suggesting devlink was because >some of the devlink >port_* ops are close to ethtool ops that can operate on a port/netdev. >eg split_port could be a netdev operation >unless you want to split before the netdev is created. Let me correct you. The split is always devlink_port operation. In some cases however when there is a mapping between devlink_port and netdev, userspace part could translate netdev->devlink_port. > >There are some ops in devlink which are global hw parameters and not >specific to a port, those fit perfectly with >devlinks original goal. There are 2 handles from the very beginning: 1) devlink - asic-wide handle 2) devlink_port - port handle
On Fri, Oct 13, 2017 at 12:11 AM, Jiri Pirko <jiri@resnulli.us> wrote: > Thu, Oct 12, 2017 at 11:53:56PM CEST, roopa@cumulusnetworks.com wrote: >>On Thu, Oct 12, 2017 at 12:20 PM, Florian Fainelli <f.fainelli@gmail.com> wrote: >>> On 10/12/2017 12:06 PM, David Miller wrote: >>>> From: Florian Fainelli <f.fainelli@gmail.com> >>>> Date: Thu, 12 Oct 2017 08:43:59 -0700 >>>> >>>>> Once we move ethtool (or however we name its successor) over to >>>>> netlink there is an opportunity for accessing objects that do and do >>>>> not have a netdevice representor today (e.g: management ports on >>>>> switches) with the same interface, and devlink could be used for >>>>> that. >>>> >>>> That is an interesting angle for including this in devlink. >>>> >>>> I'm not so sure what to do about this. >>>> >>>> One suggestion is that devlink is used for getting ethtool stats for >>>> objects lacking netdev representor's, and a new genetlink family is >>>> used for netdev based ethtool. >>> >>> Right, I was also thinking along those lines that we we would have a new >>> generic netlink family for ethtool to support ethtool over netlink. >> >>new api is fine by me. The reason for suggesting devlink was because >>some of the devlink >>port_* ops are close to ethtool ops that can operate on a port/netdev. >>eg split_port could be a netdev operation >>unless you want to split before the netdev is created. > > Let me correct you. The split is always devlink_port operation. In some > cases however when there is a mapping between devlink_port and netdev, > userspace part could translate netdev->devlink_port. yes, thats what i was trying to hint..that in some cases devlink_port can already be mapped to a netdev. > > >> >>There are some ops in devlink which are global hw parameters and not >>specific to a port, those fit perfectly with >>devlinks original goal. > > There are 2 handles from the very beginning: > 1) devlink - asic-wide handle > 2) devlink_port - port handle yep, i know that...and i was not trying to say that is a bad thing. I think we will end up with devlink_port operations that could also be done on a netdev down the lane. And, we may have to then argue where an attribute will go. Hence my suggestion on classifying the api by the target (driver in this case vs kernel networking for rtnetlink). If you take netdev out of the picture, the port attributes that devlink tries to set are similar to the ethtool port attributes today. Also, it seemed like the new port attributes set api (proposed in this thread) was close to the ethtool attributes set. Having all link hw attributes in the same tool/api has advantages. I have no plans to move anything yet...so if the general preference is to keep devlink netdev free for now, thats fine.