(src.conf.5 to be regenerated)

IMO other bits should be also moved to specialreg.h

In D43213#1269045, @rscheff wrote:

Personally I would like a TCP session NOT fail because it has ECN negotiated over a virtio driver, who then throws an error when a TSO superframe with CWR is handed down...

I agree.
I think we should talk about this on next transport meeting.

Personally I would like a TCP session NOT fail because it has ECN negotiated over a virtio driver, who then throws an error when a TSO superframe with CWR is handed down...

LGTM

No. lets abandon it.

Are you waiting for Andrew's feedback from vendor?
Could you please set the planned change state for this revision?

Is this still relevant?
If that's the case, could you please set the planned change state?

Please address Richard's comment.

The reason I am suggesting to include FIN WAIT 1/2 is that we stopped sending, but the
peer can send us data for an unlimited amount of time.. Shouldn't this data transfer towards
us be able to use ECN? That would mean we should send ACKs based on the received ECN markings.
Am I wrong with this? Why shouldn't half closed TCP connections not use ECN for the
direction data is still sent?

Are you still working on this? @glebius
if that's the case, Could you please set the planned change state for this revision?

Are you still working on this? @zlei
Could you please set the planned change state for this revision?

I also agree with Randall and Tuexen.
I hate phabricator dashboard since it's filled with many revisions, and I have to rely solely on my emails to see what I need to review.
Which is not fair to older but still relevant revisions that need reviews.
Therefore, could you please abondon this revision if that's the case?

Why cdialog and not libdialog in the message?

Ok - I went with the current version as I've been testing with it for a while, but have staged a switch to zstdcat in my local tree since I think it clearly matches our intention.

Is this revision still relevant?

In D55460#1268987, @rrs wrote:

In D55460#1268967, @tuexen wrote:

In D55460#1268958, @rscheff wrote:

LGTM

I think CLOSE WAIT should be replaced by FIN WAIT 1 and FIN WAIT 2. My mistake.

Actually no. I don't think so. The idea here is the main stack *not* ecn should be controlling what
acks are sent when we are closing. In FIN WAIT1/2 we are doing the 4-way handshake dance and
ECN (and any CC behind it) needs to not be involved with it. The stack in theory should still
react the correct way and go through the proper handshake sending acks finacks and anything
else it needs. We just don't want ECN toggling in TH_ACKNOW in the middle since it is
unaware of the 4 way shutdown taking place..

This revision is committed. Shouldn't be open.

LGTM, any concern related to this patch is revealing a design issue and IMHO, that's not something that would block this revision.

LGTM

In D55398#1267129, @ae wrote:

When you issue ioctl that does some changes in softc, another thread can do transmission in the same time and use some data from softc.
To avoid access to not yet fully modified or already freed data you need to block transmission.
Or you can allocate new gre_priv structure for each modification and free old one using NET_EPOCH_CALL(). This allows avoid blocking of transmit thread and it uses always stable data while it works in net_epoch.

I understand, I mean, personaly, I'd prefer to block transmission since reconfiguration of gre interface is rare in comparison to transmission.
This is what all other interfaces do. IMHO, it's not worth the extra complexity.

I think that's how it happened historically, for some interfaces we have different files of INET and INET6 implementation.

When I was working on implementing netlink for GRE, I noticed that the GRE implementation has too many layers of indirection due to its historical context.
It's not important and just an idea, but would you like to merge them under sys/net? I can work on it if you want.

In D55316#1265083, @glebius wrote:

Immediately caught in bridge(4):

NET_EPOCH_ENTER(et);

callout_drain(&sc->sc_brcallout);

In D55460#1268967, @tuexen wrote:

In D55460#1268958, @rscheff wrote:

LGTM

I think CLOSE WAIT should be replaced by FIN WAIT 1 and FIN WAIT 2. My mistake.

And I have found the case where we are getting the initial ack that causes the ack-war beginning.. not the
src of the continued ack-war (thats in tcp_timewait) but the initial extra ack :)

Wanted to know how ashafer thinks before accepting. Now it LGTM.

It happens if anything other than __need_size_t is defined. Some headers include stddef.h but don't want the whole thing, so they define __need_*. The header can also be included multiple times with different __need_* macros defined.
This is all internal of course, simply including stddef.h with nothing defined gives you the whole thing.

In D55430#1268961, @kib wrote:

Perhaps the defines should go into x86/include/specialreg.h together with all other CPUID bits definitions.

@emaste good point. I tweaked everything a bit too.

switch nvidia to uppercase, thanks @ashafer!

In D55460#1268958, @rscheff wrote:

LGTM

Thanks, this looks good. The only thing I think we should add is a warning about the consequence of doing so -- i.e., that the user will not be able to update via base system packages.

remove de-pkgbasify
remove src removal. unregister everything, then managing their source
tree is part of their source build phase.

Perhaps the defines should go into x86/include/specialreg.h together with all other CPUID bits definitions.

LGTM

Reducing the number of challenge ACKs when closing out the connection makes sense.

Also use SIZE_TYPE instead of size_t since it's not guaranteed to be defined.

In D55460#1268552, @rrs wrote:

In D55460#1268502, @tuexen wrote:

What is with BBR?

Hmm well for BBR its a non-issue since BBRv1 does *not* support ECN. And yes thinking on it

OK.

being in CLOSE_WAIT should still allow ack's to go out. I will change that...

In D55460#1268933, @rrs wrote:

Ok I have added the CLOSE_WAIT stage Michael suggested. However I am not
sure that is a wise thing. It may mean an extra ack in the closing state i.e. while you
are waiting for your user to close.. but on the other hand that may take a *very* long
time.. so I am ambivalent about it

Remove redundant errno save

LGTM

In D55455#1268471, @arrowd wrote:

If I understand it correctly, we need "7" to be set on podman_service_rundir to allow listing its contents. Client programs usually first check for socket existence which requires the ability to list dir's contents.

Ok I have added the CLOSE_WAIT stage Michael suggested. However I am not
sure that is a wise thing. It may mean an extra ack in the closing state i.e. while you
are waiting for your user to close.. but on the other hand that may take a *very* long
time.. so I am ambivalent about it

Sounds good, thanks.

@emaste We had a miscommunication with QA team, but it is finally being tested. I'll commit the patch as soon as I get confirmation that we didn't break anything while porting from out-of-tree.

Владлен,

I asked @tuukka.pasanen_ilmi.fi to put this one in review to facilitate discussion.

In D55460#1268502, @tuexen wrote:

What is with BBR?

I added a script which show the behavior before the patch. I also cleaned up the formatting of the script showing the behavior after the script. I am wondering why the RACK stack is now sending and additional ACK (the last segment)?