iavf: refactor plan proposal
Subsystems (2)
Hello,
We plan to refactor the iavf module and would appreciate community and maintainer feedback on our plans. We want to do this to realize the usefulness of the common code module for multiple drivers. This proposal aims to avoid disrupting current users.
The steps we plan are something like:
1) Continue upstreaming of the iecm module (common module) and the initial feature set for the idpf driver[1] utilizing iecm.
2) Introduce the refactored iavf code as a "new" iavf driver with the same device ID, but Kconfig default to =n to enable testing. a. Make this exclusive so if someone opts in to "new" iavf, then it disables the original iavf (?)
b. If we do make it exclusive in Kconfig can we use the same
name?
3) Plan is to make the "new" iavf driver the default iavf once extensive regression testing can be completed.
a. Current proposal is to make CONFIG_IAVF have a sub-option
CONFIG_IAVF_V2 that lets the user adopt the new code, without changing the config for existing users or breaking them.
We are looking to make sure that the mode of our refactoring will meet the community's expectations. Any advice or feedback is appreciated.
Thanks,
Jesse, Alice, Alan
[1]
https://lore.kernel.org/netdev/20200824173306.3178343-1-anthony.l.nguyen at intel.com/
Quoted 7 lines · Jesse Brandeburg
Hello,
We plan to refactor the iavf module and would appreciate community and maintainer feedback on our plans. We want to do this to realize the usefulness of the common code module for multiple drivers. This proposal aims to avoid disrupting current users.
Quoted 15 lines
The steps we plan are something like:
1) Continue upstreaming of the iecm module (common module) and the initial feature set for the idpf driver[1] utilizing iecm.
2) Introduce the refactored iavf code as a "new" iavf driver with the same device ID, butKconfigdefault to =n to enable testing. a. Make this exclusive so if someone opts in to "new" iavf, then it disables the original iavf (?)
b. If we do make it exclusive inKconfigcan we use the same
name?
3) Plan is to make the "new" iavf driver the default iavf once extensive regression testing can be completed.
a. Current proposal is to makeCONFIG_IAVFhave a sub-option
CONFIG_IAVF_V2that lets the user adopt the new code, without changing the config for existing users or breaking them.
I don't think that .config options are considered ABIs, so it is unclear what do you mean by saying "disrupting current users". Instead of the complication wrote above, do like any other driver does: perform your testing, submit the code and switch to the new code at the same time.
Quoted 9 lines
We are looking to make sure that the mode of our refactoring will meet the community's expectations. Any advice or feedback is appreciated.
Thanks,
Jesse, Alice, Alan[1]
https://lore.kernel.org/netdev/20200824173306.3178343-1-anthony.l.nguyen at intel.com/
Please don't introduce module parameters in new code.
Thanks
Jakub Kicinski
maintainerQuoted 10 lines · On Mon, 8 Mar 2021 16:28:58 -0800 Jesse Brandeburg wrote:
Hello,
We plan to refactor the iavf module and would appreciate community and maintainer feedback on our plans. We want to do this to realize the usefulness of the common code module for multiple drivers. This proposal aims to avoid disrupting current users.
The steps we plan are something like:
1) Continue upstreaming of the iecm module (common module) and the initial feature set for the idpf driver[1] utilizing iecm.
Oh, that's still going? there wasn't any revision for such a long time I deleted my notes :-o
Quoted 13 lines
2) Introduce the refactored iavf code as a "new" iavf driver with the same device ID, but
Kconfigdefault to =n to enable testing. a. Make this exclusive so if someone opts in to "new" iavf, then it disables the original iavf (?)
b. If we do make it exclusive inKconfigcan we use the same
name?
3) Plan is to make the "new" iavf driver the default iavf once extensive regression testing can be completed.
a. Current proposal is to makeCONFIG_IAVFhave a sub-option
CONFIG_IAVF_V2that lets the user adopt the new code, without changing the config for existing users or breaking them.
Quoted 2 lines
We are looking to make sure that the mode of our refactoring will meet the community's expectations. Any advice or feedback is appreciated.
Sounds like a slow, drawn out process painful to everyone involved.
The driver is upstream. My humble preference is that Intel sends small logical changes we can review, and preserve a meaningful git history.
Quoted 11 lines · Leon Romanovsky
Quoted 6 lines
3) Plan is to make the "new" iavf driver the default iavf once extensive regression testing can be completed.
a. Current proposal is to makeCONFIG_IAVFhave a sub-option
CONFIG_IAVF_V2that lets the user adopt the new code, without changing the config for existing users or breaking them.I don't think that .config options are considered ABIs, so it is unclear what do you mean by saying "disrupting current users". Instead of the complication wrote above, do like any other driver does: perform your testing, submit the code and switch to the new code at the same time.
Because this VF driver runs on multiple hardware PFs (they all expose the same VF device ID) the testing matrix is quite huge and will take us a while to get through it. We aim to avoid making users's life hard by having CONFIG_IAVF=m become a surprise new code base behind the back of the user.
I've always thought that the .config options *are* a sort of ABI, because when you do "make oldconfig" it tries to pick up your previous configuration and if, for instance, a driver changes it's Kconfig name, it will not pick up the old value of the old driver Kconfig name for the new build, and with either default or ask the user. The way we're proposing I think will allow the old driver to stay default until the user answers Y to the "new option" for the new, iecm based code.
Quoted 4 lines
Quoted 2 lines
[1]
https://lore.kernel.org/netdev/20200824173306.3178343-1-anthony.l.nguyen at intel.com/Please don't introduce module parameters in new code.
Thanks, we certainly won't. :-)
I'm not sure why you commented about module parameters, but the above link is to the previous submission for a new driver that uses some common code as a module (iecm) for a new device driver (idpf) we had sent. The point of this email was to solicit feedback and give notice about doing a complicated refactor/replace where we end up re-using iecm for the new version of the iavf code, with the intent to be up front and working with the community throughout the process. Because of the complexity, we want do the right thing the first time so we can to avoid a restart/redesign.
Thanks,
Jesse
Quoted 8 lines · Jakub Kicinski
Quoted 7 lines · On Mon, 8 Mar 2021 16:28:58 -0800 Jesse Brandeburg wrote:
Hello,
We plan to refactor the iavf module and would appreciate community and maintainer feedback on our plans. We want to do this to realize the usefulness of the common code module for multiple drivers. This proposal aims to avoid disrupting current users.
Quoted 6 lines
Quoted 3 lines
The steps we plan are something like:
1) Continue upstreaming of the iecm module (common module) and the initial feature set for the idpf driver[1] utilizing iecm.Oh, that's still going? there wasn't any revision for such a long time I deleted my notes :-o
Argh! sorry about the delay. These proposed driver changes impacted progress on this patch series, we should have done a better job communicating what was going on.
Quoted 7 lines
Quoted 2 lines
We are looking to make sure that the mode of our refactoring will meet the community's expectations. Any advice or feedback is appreciated.
Sounds like a slow, drawn out process painful to everyone involved.
The driver is upstream. My humble preference is that Intel sends small logical changes we can review, and preserve a meaningful git history.
We are attempting to make it as painless and quick as possible. With that said, I see your point and am driving some internal discussions to see what we can do differently.
The primary reason for the plan proposed is the code reuse model we've chosen. With the change to the common module, the new iavf is significantly different and replacing the old avf base with the new would take many unnecessary intermediate steps that would be thrown away at the end. The end design will use the code from the common module with hooks to get device specific implementation where necessary. After putting in place the new-avf code we can update the iavf with new functionality which is already present in the common module.
Thanks,
Jesse
Quoted 14 lines · Jesse Brandeburg
Quoted 11 lines · Leon Romanovsky
Quoted 6 lines
3) Plan is to make the "new" iavf driver the default iavf once extensive regression testing can be completed.
a. Current proposal is to makeCONFIG_IAVFhave a sub-option
CONFIG_IAVF_V2that lets the user adopt the new code, without changing the config for existing users or breaking them.I don't think that .config options are considered ABIs, so it is unclear what do you mean by saying "disrupting current users". Instead of the complication wrote above, do like any other driver does: perform your testing, submit the code and switch to the new code at the same time.
Quoted 5 lines
Because this VF driver runs on multiple hardware PFs (they all expose the same VF device ID) the testing matrix is quite huge and will take us a while to get through it. We aim to avoid making users's life hard by having
CONFIG_IAVF=mbecome a surprise new code base behind the back of the user.
Don't you already test your patches against that testing DB?
Like Jakub said, do incremental changes and it will be much saner for everyone.
Quoted 8 lines
I've always thought that the .config options *are* a sort of ABI, because when you do "make oldconfig" it tries to pick up your previous configuration and if, for instance, a driver changes it's
Kconfigname, it will not pick up the old value of the old driverKconfigname for the new build, and with either default or ask the user. The way we're proposing I think will allow the old driver to stay default until the user answers Y to the "new option" for the new, iecm based code.
I understand the rationale, but no - .config is not ABI at all. There are three types of "users" who are messing with configs:
1. Distro people
2. Kernel developers
3. "Experts" who wants/needs rebuild kernel
All of them are expected to be proficient enough to handle changes in CONFIG_* land. In your proposal you are trying to solve non-existent problem of having users who are building their own kernel, but dumb enough do not understand what they are doing.
We are removing/adding/renaming CONFIG_* all the time, this is no different.
Quoted 6 lines
Quoted 5 lines
Quoted 3 lines
[1]
https://lore.kernel.org/netdev/20200824173306.3178343-1-anthony.l.nguyen at intel.com/Please don't introduce module parameters in new code.
Quoted 10 lines
Thanks, we certainly won't. :-)
I'm not sure why you commented about module parameters, but the above link is to the previous submission for a new driver that uses some common code as a module (iecm) for a new device driver (idpf) we had sent. The point of this email was to solicit feedback and give notice about doing a complicated refactor/replace where we end up re-using iecm for the new version of the iavf code, with the intent to be up front and working with the community throughout the process. Because of the complexity, we want do the right thing the first time so we can to avoid a restart/redesign.
I commented simply because it jumped in front of my eyes when I looked on the patches in that link. It was general enough to write it here, rest of my comments are too specific and better to be posted as a reply to the patches itself.
Thanks
Quoted 3 lines
Thanks,
Jesse