Re: [PATCH v3 4/4] tcp: Update data retransmission timeout

[David Gibson <david@gibson.dropbear.id.au>]

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

On Tue, Oct 21, 2025 at 01:20:46AM +0200, Stefano Brivio wrote:
> On Mon, 20 Oct 2025 18:57:45 +0800
> Yumei Huang <yuhuang@redhat.com> wrote:
> > On Sat, Oct 18, 2025 at 2:28 AM Stefano Brivio <sbrivio@redhat.com> wrote:
[snip]
> > > because we can surely move faster than that, but three versions in a
> > > day obviously before I get any chance to have a look means a
> > > substantial overhead for me, and I might miss the meaning and context of
> > > comments of other reviewers (David in this case). There are no
> > > changelogs in cover letters either.
> > >
> > > I plan to skip to v6 but don't expect a review soon, because of that
> > > overhead I just mentioned.  
> > 
> > Sorry for the overhead I brought. It's just so different from what we
> > do with MRs or PRs(at least within our team), which we are supposed to
> > update as soon as possible, so reviewers could review again at any
> > time they are available. And it's always the latest code (with less
> > "problematic" code) there for review, not the outdated ones.

A lot of the differences from the workflow on a forge are a question
of degree, rather than qualitative.  If I see v4, v5 & v6 in my inbox
and I haven't looked at any yet, I'll delete v4 & v5 and go straight
to v6.


It gets trickier when there's already a discussion thread started on
one of the earlier versions (by my or someone else), or if I've
already started reviewing an earlier version.

The higher latencies of an email flow make those sorts of collision
more likely.  Although not as much as you might think, because most of
the latency is not from the technology, but from when people are awake
/ available.

> Oh, I see now.
> 
> I also have some experience with contributing via git forges, and I
> think it's a serious limitation (at least on GitHub) coming from the
> fact that you don't have (proper) threading. You have it on discussions
> and issues/tickets but not on code reviews.
> 
> You lose one dimension of discussion there, because it becomes entirely
> "linear", and while you can see differences between revisions, it's not
> really practical to review or discuss them. There's also no space to
> record and describe changes, if you just force push a branch.
> 
> I think code quality suffers because if the author of the change and
> just one reviewer are fast enough, the point of view of everybody else
> will be ignored.
> 
> Other points of view can be re-evaluated later, but in this case you'll
> waste more time writing yet another revision, which might now ignore a
> previous comment (that you addressed, previously) because it's not
> visible anymore.

I largely agree.  I find there are things to like about the forge
method.  Tracking comments according to the source line they are
relevant to can be nice in terms of automatically invalidating them
once the code is updated.  But it can also be a trap if the comment is
a broader point / discussion that was just attached to that source
line for want of a better place.

I find the forges sometimes encourage review at the level of a the
total diff of the MR, rather than patch-by-patch.  Patch-by-patch
often makes it easier to follow - or at least gives the contributor
more scope to make the change clearer by the way they split it up.



>  * * *
> 
> Let's pick this practical example here: we were in the middle of a
> discussion about whether we need to properly size a buffer to read out
> sysctl values (David's idea), or if we can go for a larger buffer in any
> case to keep things simpler (my proposal).
> 
> Before I had the chance to follow up with the discussion, you posted
> another revision. And then another one.
> 
> On GitHub, it would be impossible for me to re-open that discussion, so
> I would start a new one, and now David might miss the fact it's the
> same discussion. Maybe he was right, but it doesn't matter anymore.
> 
> With email, I can do that because we have threading and persistence, but
> if the outcome of the discussion now changes, you wasted time with
> another revision.
> 
> Or maybe I see that you're at v7 now and I forget that that discussion
> was still open, so my previous point, even if valid, is now effectively
> ignored and forgotten by everybody.
> 
> The workflow you have on GitHub works well if you have one author and
> one reviewer, or more reviewers who are always right and always agree
> between each other, but that's a quite unrealistic expectation.
> 
> I guess it also works well if code quantity is more important than
> quality, because it's merged faster that way, and because it's harder
> to discuss about it (no real threading). But here we're trying to have
> less code and less bugs, not more.
> 
> > I thought
> > it's the same with patches in emails, that outdated versions are no
> > longer useful.
> 
> They are, but they're not so practical to have a discussion about, so
> not so useful as the current one, which is why discussions should have
> a chance to complete.
> 
> You'll just be busy writing new revisions otherwise, instead of having
> time for something else in parallel.

Btw, it's perfectly ok to apply some fixes from review in your local
tree for things that are simple, but wait a while before re-posting
for longer discussions to complete or for additional reviewers to
contribute.  I do this all the time.

There's not a lot of hard and fast rules here, it's a question of
weighing the reasons to post early against the reasons to wait for the
next spin.  Taking a while to tune that is expected for any new
developer.

> And reviewers have other stuff to review too, so we don't really gain
> time if you re-post fast.
> 
> It's different if we have a critical issue affecting many users and we
> want to fix it fast for them. But usually it's a small patch/series in
> that case and we don't care so much about discussing the best approach
> as long as it's fixed and released quickly.
> 
> > Apparently I got it wrong. I will keep it in mind and
> > not send too many versions in a short time, and add changelogs in
> > cover letters when necessary.
> 
> It's not always necessary I think, and sometimes you can keep things
> short if they're obvious to everybody.

Right.  As a reviewer, a detailed changelog is very useful.  But as a
developer I know that maintaining that changelog can be a lot of work.
So again, it's a matter of finding a balance.

> These are the biggest series
> ever posted for passt, in terms of number of patches:
> 
>   [PATCH v2 00/32] Use dual stack sockets to listen for inbound
>   TCP connections
>   https://archives.passt.top/passt-dev/20221117055908.2782981-1-david@gibson.dropbear.id.au/
> 
>   [PATCH v11 00/30] Introduce discontiguous frames management
>   https://archives.passt.top/passt-dev/20250902075253.990038-1-lvivier@redhat.com/
> 
> ...you'll see that, for some revisions, changes are very briefly
> summarised. That's enough, especially if there was a single reviewer
> for a given revision.
> 
> But with this series it's doable and there are a few specific changes
> between each revision, so I think you should, because it helps
> reviewers to understand what you're doing.
> 
> -- 
> Stefano
> 

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

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