In D15706#336213, @micchie.gml_gmail.com wrote:

In D15706#335628, @rwatson wrote:

In D15706#334870, @micchie.gml_gmail.com wrote:

In D15706#334711, @jtl wrote:

In D15706#334605, @rwatson wrote:

Didn't quite catch this before it was committed. This isn't really a destructor, it's a close notification. Rather than confuse matters, as sockets have UMA destructors as well, this should probably be so_notify_close.

Yes, I caught that when I asked the submitter about the placement of the "destructor". But, once I figured out it was a close notification, I should have changed the name. Mea cupla.

Before changing this, let me see if I can confirm what the Linux implementation does.

Thanks, it is intended to be equivalent to sk_destruct() callback in struct sock in Linux.

Is the Linux sk_destruct called on socket close, or on socket destruction? If we want an actual socket destructor, we can add one of those [instead / as well], called from the socket destructor function.

It is literally destructor, which is called when the final reference count drops to zero. So probably we should define so_dtor() as destructor (so keep the name) and call it after SOCK_UNLOCK() in sofree(). (in Linux it is invoked without locking socket).

In D15706#334605, @rwatson wrote:

Didn't quite catch this before it was committed. This isn't really a destructor, it's a close notification. Rather than confuse matters, as sockets have UMA destructors as well, this should probably be so_notify_close.

I've spent some time thinking about this a bit, and I have the following comments. (Some may seem contradictory, but please bear with me. :-) )

Address @markj's feedback by always defining the vmd_kernel_rwx_arena member of the vm_domain struct.

Update the zone(9) manpage.

Okay, really without the cruft this time. (Hopefully...)

I'll try to look at this review this week.

Get the correct version of sys/vm/vm_kern.c (with the troubleshooting stuff removed).

Numerous updates:

• Address @alc's concerns by adding a new arena for allocations with non-standard permissions. Import a 2MB-aligned address block at a time into the arena. Release space back to the parent arena when able. But, only do this for architectures with superpages.
• Plumb M_EXEC through malloc(9).
• Fix BPF by reverting most of rS317072.
• Add a note to the manpage that not all architectures will enforce execution permissions.

This passes my "sniff test", but it would be better to get @pkelsey to review it.

I think there are more changes needed.

The submitter spoke to me in person at BSDCan and answered my questions.

In D15691#331801, @alc wrote:

Overall, I think that this is a good idea, but the implementation has the following problem. The allocation of one executable page will block the promotion of the surrounding pages to a superpage mapping.

Incorporate review feedback from @jhb.

I added a few basic comments while I ponder the rest...

I don't know ISA well. I'm open to switching this to be an option to always panic on any NMI, instead of picking NMI_TIMER2 for special treatment.

I'm sorry it took me so long to review this.

In D15386#330028, @brooks wrote:

Anyone on transport care about this?

In D11003#328091, @sbruno wrote:

In D11003#327485, @jtl wrote:

In D11003#327482, @sbruno wrote:

Is everyone on this review fine with me committing this? With the inheritance code removed, I believe there are no further objections or questions.

I've been meaning to review this, but have been delayed. I would appreciate a few extra days to look it over.

@jtl I'd like to commit this at some point this week, but I also want your feedback on it. Should we hold off until next week to drop this into head?

In D11003#327482, @sbruno wrote:

Is everyone on this review fine with me committing this? With the inheritance code removed, I believe there are no further objections or questions.

Having stared at this a bit, I generally think its fine. In fact, in some aspects it is an improvement over what it replaces.

In D15483#326763, @mjg wrote:

First a minor note is that I took the liberty of s/bzero/bcmp, which I presume was intended.

By the way, measurements were taken on an Intel E5-2697A v4 (32-core Broadwell).

Yes, see rS302894, rS302997, rS302998, rS303037, rS303096, and rS303098.

Sorry it took me a while to look at this. See comments in-line.

Given the fact that users appear to depend on this (we have two PRs on it), this appears to have been an unintended change in stable/11, and it seems to violate POLA by making this change on a stable branch, I intend to commit this today unless someone objects.

@glebius : Any progress on reviewing this? IIRC, you didn't like overloading the SS_ISDISCONNECTED flag to indicate listen sockets that were shutdown. Do you have an alternate proposal?

Is everyone OK with this going in? Or, to ask it differently, is anyone not OK? The 11.2 release is underway, so it would be good to get this in soon, if we agree to fix it in stable/11.

BTW, it looks like this was introduced in r331214. Since it is so new, my "not widely used" comment is probably both true and somewhat irrelevant.

In D15337#323278, @mmacy wrote:

In D15337#323183, @jtl wrote:

How does this interact with the low-latency, high-precision timestamp option being discussed at the IETF?

See https://tools.ietf.org/html/draft-wang-tcpm-low-latency-opt-00 and https://www.ietf.org/proceedings/97/slides/slides-97-tcpm-tcp-options-for-low-latency-00.pdf .

@jtl I was under the impression that that discussion had stalled. The RFC you point at expired on December 10, 2017. Is there a mailing list where this is being discussed or do I need to mail them individually?

How does this interact with the low-latency, high-precision timestamp option being discussed at the IETF?

In D14993#320674, @tuexen wrote:

I'm not sure it is a good idea to have a sysctl for the default value. I would prefer to have the default value always be 0 (not changeable by a sysctl) and required the application to set a non-default value via the socket option. Why is there a need to change the default from "off" on a system wide base?

In D15021#319135, @emaste wrote:

Presumably we need to agree on and commit D15019 first, and commit this as a MFC in spirit after the change in HEAD.

If we're going to get this into 11.2, it would be nice if someone reviewed this soonish.

Looks good. Please check on two things before committing:

1. I don't see a svn:keywords property change for head/share/pgpkeys/thj.key. I expected to see one, although this could just be a Phabricator display issue. Can you confirm you've followed the instructions in the Note at the start of 5.3.7 of the Committer's Guide ("Adding and Removing Files")? If not, please do so and then svn remove and svn add the file again. You can check that the property was correctly applied by running this command: svn propget svn:keywords thj.key. You should see FreeBSD=%H.
2. Please check on the indentation in news.xml. (See in-line note.)

See nits inline.