[RFC net-next] iavf: refactor plan proposal

[RFC net-next] iavf: refactor plan proposal

[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.

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@intel.com/

Re: [RFC net-next] iavf: refactor plan proposal

[Leon Romanovsky]

On Mon, Mar 08, 2021 at 04:28:58PM -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.
> 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.

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.

>
> 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@intel.com/

Please don't introduce module parameters in new code.

Thanks

Re: [RFC net-next] iavf: refactor plan proposal

[Jakub Kicinski]

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

> 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.

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.

Re: [RFC net-next] iavf: refactor plan proposal

[Jesse Brandeburg]

Leon Romanovsky wrote:

> > 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.
> 
> 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.

> > [1]
> > https://lore.kernel.org/netdev/20200824173306.3178343-1-anthony.l.nguyen@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

Re: [RFC net-next] iavf: refactor plan proposal

[Jesse Brandeburg]

Jakub Kicinski wrote:

> 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

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.

> > 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

Re: [RFC net-next] iavf: refactor plan proposal

[Leon Romanovsky]

On Tue, Mar 09, 2021 at 09:11:46PM -0800, Jesse Brandeburg wrote:
> Leon Romanovsky wrote:
>
> > > 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.
> >
> > 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.

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.

>
> 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.

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.

>
> > > [1]
> > > https://lore.kernel.org/netdev/20200824173306.3178343-1-anthony.l.nguyen@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.

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

>
> Thanks,
>  Jesse