Various changes "backportability"

Various changes "backportability"

[Michael Tokarev]

[Added some more active patch reviewers to Cc]

Hi!

Yesterday I wrote email about picking up changes from master
for previous stable release(s).  What's interesting is that
yesterday, basically in a single day, we've faced numerous
examples of subsystem changes which makes such backporting
significantly more difficult than might be.

For example, recent tpm bugfix, which is trivial by its own,
uses RETRY_ON_EINTR helper which were introduced recently and
which is now used everywhere.  coroutine_fn et al markers is
another example, translator_io_start is yet another, and so
on and so on.

When adding such subsystems/helpers which are to be used widely,
please split the initial implementation patch out of a single
"introduce foo; convert everything to use it" change.  Instead,
add the feature in a small patch first, and convert all users
tree-wide to it in a second, subsequent patch, maybe removing
the old version in that second patch too.  Where it makes sense
ofcourse, - sometimes it is not possible or just complicated to
do that, like when old and new implementations can't be supported
in parallel.

Just by splitting "introduce" from "convert", especially for
something simple which will be used all around, you'll greatly
simplify stable trees maintenance.

Thank you for consideration! :)

/mjt

Re: Various changes "backportability"

[Stefan Hajnoczi]

On Wed, 13 Sept 2023 at 04:13, Michael Tokarev <mjt@tls.msk.ru> wrote:
>
> [Added some more active patch reviewers to Cc]
>
> Hi!
>
> Yesterday I wrote email about picking up changes from master
> for previous stable release(s).  What's interesting is that
> yesterday, basically in a single day, we've faced numerous
> examples of subsystem changes which makes such backporting
> significantly more difficult than might be.
>
> For example, recent tpm bugfix, which is trivial by its own,
> uses RETRY_ON_EINTR helper which were introduced recently and
> which is now used everywhere.  coroutine_fn et al markers is
> another example, translator_io_start is yet another, and so
> on and so on.
>
> When adding such subsystems/helpers which are to be used widely,
> please split the initial implementation patch out of a single
> "introduce foo; convert everything to use it" change.  Instead,
> add the feature in a small patch first, and convert all users
> tree-wide to it in a second, subsequent patch, maybe removing
> the old version in that second patch too.  Where it makes sense
> ofcourse, - sometimes it is not possible or just complicated to
> do that, like when old and new implementations can't be supported
> in parallel.
>
> Just by splitting "introduce" from "convert", especially for
> something simple which will be used all around, you'll greatly
> simplify stable trees maintenance.

The general concept makes sense to me but I'm not sure what the
specific issue with adding (?) coroutine_fn was. Can you link to the
patch that caused difficulties so I can review it?

Thanks,
Stefan

Re: Various changes "backportability"

[Michael Tokarev]

13.09.2023 17:27, Stefan Hajnoczi wrote:
...
>> For example, recent tpm bugfix, which is trivial by its own,
>> uses RETRY_ON_EINTR helper which were introduced recently and
>> which is now used everywhere.  coroutine_fn et al markers is
>> another example, translator_io_start is yet another, and so
>> on and so on.

> The general concept makes sense to me but I'm not sure what the
> specific issue with adding (?) coroutine_fn was. Can you link to the
> patch that caused difficulties so I can review it?

There's nothing really exciting here, and coroutine_fn example isn't
a best one really.  I'm talking about this:

https://gitlab.com/mjt0k/qemu/-/commit/c5034f827726f5876234bf4c6a0fab648fd8b020

which is a current back-port of 92e2e6a867334a990f8d29f07ca34e3162fdd6ec
"virtio: Drop out of coroutine context in virtio_load()":

https://gitlab.com/mjt0k/qemu/-/commit/92e2e6a867334a990f8d29f07ca34e3162fdd6ec

This is a bugfix which I tried to cherry-pick (btw, I dunno yet if it should
go to 8.0 or 7.2 to begin with, asked this in another email, but it still
serves as an example).  Original patch adds coroutine_mixed_fn to some existing
functions and to a newly added function.

The patch introducing coroutine_mixed_fn marker is v7.2.0-909-g0f3de970 .
This is actually a very good example of a way how things are done best,
an excellent example of what I'm talking here, - this 0f3de970 only introduces
the new concept (to be widely used), not converting everything to it
right away.  So it's a good example of how things can be done right.

But this 0f3de970 change is based on earlier change which split things up
and moved stuff from one place to another, and which is too large to
backport.  So even if 0f3de970 did an excellent job, it is still of no
use in this context.

I decided to drop coroutine_mixed_fn markings in the fix for 7.2 in this
context, - again, if this particular fix is needed there to begin with,
which is a question unrelated to this topic.


A better example is a trivial thing with RETRY_ON_EINTR introduction.
A trivial macro which replaced TFR in

commit 37b0b24e933c18269dddbf6b83f91823cacf8105
Author: Nikita Ivanov <nivanov@cloudlinux.com>
Date:   Sun Oct 23 12:04:22 2022 +0300

     error handling: Use RETRY_ON_EINTR() macro where applicable

if this change were split into two, first introducing the new macro
and second converting existing code & removing old macro, it'd be
possible to just cherry-pick the first part and thered' be no need
to modify further cherry-picks which uses RETRY_ON_EINTR.

But once again, this all is definitely not as important as getting
good code into main :)

Thanks,

/mjt

Re: Various changes "backportability"

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2921 bytes --]

On Wed, Sep 13, 2023 at 05:44:38PM +0300, Michael Tokarev wrote:
> 13.09.2023 17:27, Stefan Hajnoczi wrote:
> ...
> > > For example, recent tpm bugfix, which is trivial by its own,
> > > uses RETRY_ON_EINTR helper which were introduced recently and
> > > which is now used everywhere.  coroutine_fn et al markers is
> > > another example, translator_io_start is yet another, and so
> > > on and so on.
> 
> > The general concept makes sense to me but I'm not sure what the
> > specific issue with adding (?) coroutine_fn was. Can you link to the
> > patch that caused difficulties so I can review it?
> 
> There's nothing really exciting here, and coroutine_fn example isn't
> a best one really.  I'm talking about this:
> 
> https://gitlab.com/mjt0k/qemu/-/commit/c5034f827726f5876234bf4c6a0fab648fd8b020
> 
> which is a current back-port of 92e2e6a867334a990f8d29f07ca34e3162fdd6ec
> "virtio: Drop out of coroutine context in virtio_load()":
> 
> https://gitlab.com/mjt0k/qemu/-/commit/92e2e6a867334a990f8d29f07ca34e3162fdd6ec
> 
> This is a bugfix which I tried to cherry-pick (btw, I dunno yet if it should
> go to 8.0 or 7.2 to begin with, asked this in another email, but it still
> serves as an example).  Original patch adds coroutine_mixed_fn to some existing
> functions and to a newly added function.
> 
> The patch introducing coroutine_mixed_fn marker is v7.2.0-909-g0f3de970 .
> This is actually a very good example of a way how things are done best,
> an excellent example of what I'm talking here, - this 0f3de970 only introduces
> the new concept (to be widely used), not converting everything to it
> right away.  So it's a good example of how things can be done right.
> 
> But this 0f3de970 change is based on earlier change which split things up
> and moved stuff from one place to another, and which is too large to
> backport.  So even if 0f3de970 did an excellent job, it is still of no
> use in this context.
> 
> I decided to drop coroutine_mixed_fn markings in the fix for 7.2 in this
> context, - again, if this particular fix is needed there to begin with,
> which is a question unrelated to this topic.
> 
> 
> A better example is a trivial thing with RETRY_ON_EINTR introduction.
> A trivial macro which replaced TFR in
> 
> commit 37b0b24e933c18269dddbf6b83f91823cacf8105
> Author: Nikita Ivanov <nivanov@cloudlinux.com>
> Date:   Sun Oct 23 12:04:22 2022 +0300
> 
>     error handling: Use RETRY_ON_EINTR() macro where applicable
> 
> if this change were split into two, first introducing the new macro
> and second converting existing code & removing old macro, it'd be
> possible to just cherry-pick the first part and thered' be no need
> to modify further cherry-picks which uses RETRY_ON_EINTR.
> 
> But once again, this all is definitely not as important as getting
> good code into main :)

I see, thank you!

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]