The summary of this review is lacking detail

Is there any concern or additional work needed for ipV6?

To all those harping about it not being foo or bar or zed, this *is* an underlying frame work that is very flexible and allows one to implement what ever foo, bar or zed configuration you want on top of it. This leaves json, xml, or any other implementation out of the base layer code and those can be implemented in a layer above this. This has been discussed for a some time extensively by those that attend the semi-monthly bhyve call.

Approving as mentor only.

In D25876#574103, @chuck wrote:

...
Your observation that the data units values restart at zero is very valid. My tentative plan for this is to optionally save the current values when the VM is powered down using the new configuration file format. This saved device state could then be used on a subsequent restart to provide the continuity you describe.

Reflowing sentences and removal of .Tn seems like busy work to me. Much easier to just make mandoc ignore .Tn and igor silent on its usage. These macros DO make the groff man pages output look different, and IMHO, nicer.

I still see request changes markers from emaste and jhb/bhyve, what are these changes?

In D25833#572269, @emaste wrote:

In D25833#572248, @rgrimes wrote:

Could we please get stances in /etc/sshd/ssh{,d}_config commented out that show how to enable these if they are needed?

For example,

Ciphers +aes128-cbc,aes192-cbc,aes256-cbc

I'd rather not deviate from upstream, but if we must I'd rather do so in sshd_config, not in the source.

Could we please get stances in /etc/sshd/ssh{,d}_config commented out that show how to enable these if they are needed?

In D25793#572090, @rajesh1.kumar_amd.com wrote:

Can we add an appropriate SPDX tag to the source files as well - e.g. /* SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */ or similar?

Need a clarification here please. The license text in the source files has additional text (part of it give below) apart from the normal BSD-3-Clause text. If this is not an issue, we can use /* SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */. Please clarify.

I have no objection to it, but it is still not totally factual, more than likely it was "intergrated" into FreeBSD Current/Head when it was at 12 and merged back to stable/11 and first appeared in a FreeBSD release at 11.1. I still do not see any added value to the history of mandoc by this change, and it just makes FreeBSD have a local diff to the mandoc from witch it was derived. Speaking of which, shouldn't this be on a vendor branch?

I see a fair amount of white space/formatting only changes in this, it MAY make since to break those out as 0 effort review.
I also have concerns over any performance claims, though I see the old code is probably at least sub optimal in that it often checks for UDP, then for TCP when the volume of traffic should almost always be mostly TCP. Also modern compilers should just "handle this" without rewritting code unless something odd is being done. If you really want to micro optimize I suspect rewriting this as "branchless" code that results in cmovl or other instructions might be interesting. I have heard of 40x code speed improvements using such tenchiques, but that is mostly in tight loops.

I was asked to look at this and see if I could help get the Makefile issues moved forward. I have commented inline.

In D25601#566423, @imp wrote:

In D25601#566410, @markj wrote:

In D25601#566409, @imp wrote:

I don't think this is a good idea....

Setting sysctl values multiple times may be unwise.o

I concur on this point. Not all sysctl are idempotent.

This is a good fix, it is wrong for the kernel to try and guess that something is wrong here. "Highly Probably != For Certain" and having this kind of stuff get in the way when your trying to make something work is very annoying for the end user. Now if we could just get the hard coded interface loopback route creating/removal out of the kernel.

I do not believe we normally document this type of information in the HISTORY section of manual pages. The history sections of these man pages are already rich with information, especially when you add the history included in the Authors section.

Is it possible to do cgn without rfc1918? I think the changes should allow for that configuration.

Though much better than the prior iteration I believe it leaves some detail out.

This has been in process for 15 months and should be evolving in tree rather than in a review at this point, IMHO. If no one objects to it being committed in 7 days I am asking that Richard go ahead and commit it.

So, is FreeBSD getting close to a proper Cubic yet?

In D25401#560437, @tuexen wrote:

The in_pcb_setup code is that called independent of L4? Ie for all sockets? Or have i lost my way in the twisty maze of IP layers?

TCP calls it: tcp_usrreq.c:1566. The logic is independent from L4. They are also rewriting INADDR_BROADCAST, which doesn't make sense for TCP, but we are catching this now...

In D25401#560408, @tuexen wrote:

In D25401#560404, @rscheff wrote:

Should this

• If the destination address is INADDR_ANY,
• use the primary local address.

It took me a few "brain loops" to realize you mean the "local loopback interface address" or is there actually some concept of a "primary local IP address."?

Thank you

In D25312#559159, @imp wrote:

Summary so far:

1. there's overwhelming support for this, at least in theory
2. the devil is in the details.

... Much sound analysis removed ...

I find this change debatable, and as presented here incomplete as I believe this has effects on some other parts of style(9), nor does it provide a proper sample (it leaves the example unchanged) Lets not RUSH to slam this in the tree!

If you have clients that need to do this type of thing I would recommend they are running behind DSCP and they should use 2 DSCP's to seperate there DCTCP like traffic from there TCP/reno like traffic. Using ECT(1) as a traffic classifier in advance of IETF clear decision may land them in a very hard to resolve possition in the future.

No, the point was to reduce the amount of change

Just a couple nits, and I can be ignore on my line breaking issues.

I'll commit this today

Approving as mentor, only a visual code read was done.

Works for me

In D24596#548776, @emaste wrote:

In D24596#547800, @pi wrote:

These types of greps on a linux system return an error message for every directory in the directory you ran the grep in

So if we modify grep to not try to read directories and not spew errors, would that be a solution ?

It wouldn't even take much modification, just changing the default to be -d skip.

Or is there some POSIX or similar rule that requires those errors 8-} ?

POSIX allows implementations to do what they want with read() on a directory and grep's behaviour follows from that, so I think it's within the spirit for us to show errors or not, as we see fit.

In D24596#547795, @kevans wrote:

In D24596#547790, @rgrimes wrote:

Now that I have had time to go play with this on a "Linux" system, I am afraid I object to this change. Your doing it because you don't like the occasional splat of "foo is a binary file" when you run a grep *, you should see the results when EISDIR is returned. These types of greps on a linux system return an error message for every directory in the directory you ran the grep in, far more noise than your small issue, and for many of us this is gona be a PITA/POLA change. I hadnt thought much about it cause I alias 'grep' 'grep -d skip' and do not see your issue or my issue normally on linux.

I think to rectifier your issue you should probably do the same I have done...

I'm afraid I have no idea what you're talking about here...grep isn't my problem, my problems are:

1. The security implications of being able to read(2) a dirfd,
2. Consistency with how most modern kernels behave,

2a) the amount of time I waste hunting down application bugs because of that

There was probably a 3, but it's late and a bit worn down from recent discussions.

In D24596#547205, @imp wrote:

In D24596#547104, @mateusz_serveraptor.com wrote:

Probably grep(1) should be updated after this change. It has -d option to specify which action to to take on processing directories. From manpage:

-d ACTION, --directories=ACTION
       If an input file is a directory, use ACTION to process it.  By
       default, ACTION is read, which means that directories are read
       just as if they were ordinary files.  If ACTION is skip,
       directories are silently skipped.  If ACTION is recurse, grep
       reads all files under each directory, recursively; this is
       equivalent to the -r option.

grep doesn't actually read directories for anything useful (and can't for ZFS), so this change is a nop as far as GREP is concerned.

Now that I have had time to go play with this on a "Linux" system, I am afraid I object to this change. Your doing it because you don't like the occasional splat of "foo is a binary file" when you run a grep *, you should see the results when EISDIR is returned. These types of greps on a linux system return an error message for every directory in the directory you ran the grep in, far more noise than your small issue, and for many of us this is gona be a PITA/POLA change. I hadnt thought much about it cause I alias 'grep' 'grep -d skip' and do not see your issue or my issue normally on linux.

I do not like the undocumented (per summary anyway) KASSERT added for writes.

In D24021#546333, @neel_neelc.org wrote:

In this patch, "me4" is IPv4-only and "me" is dual-stack. It uses kernel opcodes, however.

I know there well be flak over using .Tn in a man page fix, can you submit a new diff without the .Tn's?

I just fixed a couple typos in the summary, looks ok.

Can we get some more eyes on this please?

Thank you for the comment, that helps me, small english nit

That reads much better, cleaned it up a fair bit more than I thought it would.

Adding bz, as it is his XXX-BZ comment being removed about implementing this.

Request for a comment, non blocking.

Thank you, nice additions beyond what I stumbled on in the PR

If you only knew how long I fought with white space issue, Ar and Cm trying to get that to format correct for cpus=. Thank you!