Getting way above my pay grade

@allanjude I know your work load is huge as well, if you need me to push a new diff I can do that, but then you have to agree to commit it or I have to pester bde/phk for an approval.

Concur, but not sure how to force it closed.

@jhb, reopening a commited phab review messes with things, as now when you make a second commit the default top patch in this review well be the one you commit, and one has to go digging in the review history to find the prior patch that was in the first commit. It is fine to reopen a review for a revert, but I think one should start a new review for a new change, which is what this is.

Minor nits, and a major win, good work!

Any more little gold mines like this sitting around?

Ok to commit once blank lines are added per jhb in person

I thought this had already been committed?

I would like to see this in next weeks 11.3-PRERELEASE build, this looks to be a low risk barrier addition, @jhb does that seem to be a reasonable target, or would you like to see a longer ^head test cycle? Does anyone see any risk in this change?

This may be a fix for https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=231117, the person is testing.

Thanks for the quick spin on this Patrick

Ping

@jhb it would be good to not have this issue in 11.3

You can mark all my comments as done, I am fine with your answers, I added bz and requested his review on the one change.

In D20065#433092, @kib wrote:

In D20065#433088, @markj wrote:

In D20065#432936, @jhb wrote:

In D20065#432918, @markj wrote:

In D20065#432917, @jhb wrote:

To be clear, the limit isn't being applied today? From the existing code it seems like it should be already in force if I'm reading the diff to vm_map_wire correctly?

The diff is being applied on top of D19908. Without that change, the limit does not get applied to bhyve.

Ok.

Also, if bhyve is allowed to ignore the limit, the amount of wired memory it uses does mean that small requests by, say, gpg will then fail on the host, yes?

Yes, that's right (and true in head today). In the original version of this diff, where bhyve was doing system wiring, that was not the case since system-wired pages are not counted towards the limit.

Ok. I still don't really know how I feel. I think I probably want all long-term wirings to honor the limit as kib@ says and that users will just have to up the limit when using wired VMs with bhyve. If we can get a suitably unique errno value back in whatever creates the wired memory for bhyve and emit a useful error message to the user, that is probably sufficient.

Ok. I guess we can just try and see if there's any fallout. I do think it means that D19908 is not MFCable.

If you add a knob to disable accounting for bhyve requests, it can be merged.

In D19908#433193, @rgrimes wrote:

Do we have to add _user_ to the variables, I only see a rename here, and not a second value used in place of where _users_ went in, or am I totally missing something?

Do we have to add _user_ to the variables, I only see a rename here, and not a second value used in place of where _users_ went in, or am I totally missing something?

I have made one quick pass over this, now that I have read all the bits I would like to make a second pass, my comments now are nits mostly, and can be safely ignored. However I do, as others, have some pretty big concerns about sharing the rcvif with tags, is there some great cost in not doing that?

Well this in any way help with the fact we do not apply any pressure to the zfs arc_max and end up in a memory shortfall that causes high paging? I suspect not as most people do not wire there VM's in memory unless they know that is what they need to do to keep zfs from killing your box with OOM

In D20065#431805, @markj wrote:

In D20065#431692, @rgrimes wrote:

I can see both sides of this, can I ask that the bhyve developers group which meets every 2 weeks be given a chance to discuss this and provide feedback on it? Thanks. Many of us well be at BSDCan a day early for bhyvecon if you wish to discuss it in person.

Why can't the discussion happen here?

Some of it is, but we meet on a cyclic basis to have high bandwidth low delay discussions and I was just asking for the opportunity to allow that discussion to occur. Or if you referring to the in person option that was offered up "if you wish".

I can see both sides of this, can I ask that the bhyve developers group which meets every 2 weeks be given a chance to discuss this and provide feedback on it? Thanks. Many of us well be at BSDCan a day early for bhyvecon if you wish to discuss it in person.

In D19993#431685, @asomers wrote:

The symlink is a new addition for 13.0 (and hopefully 12.1). I would be ok with deleting it for 13.0. But how about I commit this change to head first, and then delete the symlink? That will make MFCs cleaner.

This makes it much clearer to me, thanks jhb

We only need to maintain "backwards" compatibility in the stable/ branches (12 and 11) as far as I am concerned head can just nuke the compatibilty link, and have a 13.0 release notes item that is "fuse has been renamed to fusefs".

A question to @jhb, should the order of the enum and the code updated to be in alphabetic order, in a seperate review, it would only obfuscate the change to do it here.

Looks good, other than a lot of extra blank lines.

In D19587#427247, @kevans wrote:

Would iflib like any kind of UPDATING or relnote mention for the generated 'stable' MAC address likely changing (since we're restricting the range now to a subset of the FF OUI), or is it fine without explicit mention?

You can mark all my comments as done, you have addressed them to my satisfaction.

In D18670#425449, @crees wrote:

In D18670#425443, @rgrimes wrote:

I have performed a visual only review.

Greatly appreciated. Can I take that as approval by a src developer?

I have performed a visual only review.

Thank you, that is 100x clearer as to what is in the constant

Confirmation of a working Windows 2019 boot after this patch: https://lists.freebsd.org/pipermail/freebsd-virtualization/2019-April/007333.html

Can we get a review to cause bhyve -b to emit a deprecation warning notice (gone_in(13)) if someone tries to use it? That could be merged to stable/12, then the code in head killed.
This would also apply to the bvmconsole driver, is there someway to emit a warning during config(8) when someone uses a feature being deprecated? Doing this provides us feedback
sooner rather than later that some feature was in use some place.

Looks like someone may of removed a condition at some time in the past and not re-dented the code to produce a minimal diff.

Does this variable get used by default? I saw this here and just ignored it as I thought it would only be invoked if I asked for a DEBUG build from a level above. If we are always adding DEBUG_FLAGS to our compiles perhaps it should be called something else?

Please have a discussion on arch@ or filesystems@, this review needs more discussion

Can I get some final accept on this, I updated the open issues

I like it, just nits, that for the most part you can ignore, other than $FreeBSD: vs $FreeBSD$

This appears to be used to deal with a core hang on specific CPU's, we need to see that we have mitigated that in the host, and the decide what the appropriate action here is, I am uncomfortable with just making this silently disappear. Deferning to jhb.

Though this does fix your issue by silencing the log, it does little to improve the situation. I think what we may want to do is to add yet another option to the bhyve command that along with -w to ignore invalid MSR access to do so silently so that we do not have to add code like this everything a new msr comes into play. John, Patrick your thoughts on this approach?

In D19426#417642, @emaste wrote:

I would be comfortable with simply "https://bugs.freebsd.org/12344" as that can likely work forever.

Note that https://bugs.freebsd.org/12344 does work - we should accept either https://bugs.freebsd.org/bugzilla/show_bug.cgi?id= or https://bugs.freebsd.org/[0-9]+

I believe what your wanting is web server re-write rules, that would rewrite the URL in and out of the web pages to have the preffered form. Beyond my area of expertise to implement, but I know it can be done and that is probably the easiest place to do it (ie, no modifying bugzilla should be required.)

I removed a redundant "to" in the description. Patch looks good to me. Copyright over to jhb@

I'll review if you ack your are done updating John