Style & comments

I don't think there's any real reason to diverge from how Linux computes its own version numbers, just to be "more native" FreeBSD. I don't know if these numbers are exposed in any userspace interface, but if they are obviously they need to match Linux, not our own made-up scheme.

I note that arm64 has this bug as well. I'll fix that identically if the current diff is ok.

In D15910#337586, @markj wrote:

In D15910#337582, @alc wrote:

I have two comments.

1. The change to reserve_pv_entries() is unnecessary. That function is only called during demotion. At that point, the only PV entry will be for the superpage mapping, and reclaim_pv_chunk() already skips superpage mappings.

Indeed, I noted this in the review description. The comment above reclaim_pv_chunk() suggests that it skips superpage mappings only to avoid worsening an ongoing PV entry shortage, but in fact this is required for correctness of the code. To me it seemed fragile to leave reserve_pv_entries() as it was since the handling of reclaim_pv_chunk() wrt superpage mappings might change in the future (e.g., by searching for and reclaiming all 4KB page mappings when a superpage mapping is discovered) and introduce this subtle bug.

do you need a committer? :)

• Only recount after a reclaim.

Could you update the diff using arc-diff?

In D15952#337588, @cem wrote:

Why not leave the macro alone and compute v using it instead?

v = LINUX_KERNVER(v0, v1, v2);

(As I noted in the bug, the macro matches the way Linux defines kernel version numbers for a given major, minor, and patch, so it makes the most sense to leave it that way.)

In D15911#336915, @kib wrote:

In D15911#336740, @markj wrote:

In D15911#336659, @kib wrote:

This should be fine.

Can we return an indicator if the page freed from the locked pmap, and retry only in this case ?

We can. I implemented it this way originally, but found it a bit ugly, and strictly speaking we only need to retry if the chunk was freed from the locked pmap *and* it contained free entries. That is, even with your suggestion we may still retry when it is not necessary. If you prefer that approach I'll implement it instead, but I mildly prefer this patch because it's simpler than the alternative.

Sure I do not object against adding the additional check for the chunk to contain a free entry, and it is cheap. My motivation for the suggestion is that we should not penalize the situation when get_pv_entry() failed to allocate a page, too much.

But I do not object to the patch in its current form.

Why not leave the macro alone and compute v using it instead?

In D15910#337582, @alc wrote:

I have two comments.

1. The change to reserve_pv_entries() is unnecessary. That function is only called during demotion. At that point, the only PV entry will be for the superpage mapping, and reclaim_pv_chunk() already skips superpage mappings.

I have two comments.

I will compile commit this review.

Update the test case for acct_success

Committed as r335490.

Approved

Approved

I had been trying to keep all patches to gemspec files named simply "patch-gemspec" so that they were easier to find when updating gem, because that sometimes requires updating all the gemspec patches. So I think that's why this file was named the way it was. Would be nice to keep that, but not a huge issue, it does cause portlint warnings.

Approved

Move NO_ARCH down and included full diff.

Approved

[v3] Fix output of linprocfs stat

Updated diff to full diff.

Approved

Updated diff.

Added PR 229209 to track the osrelease issue and posted a proposed fix in https://reviews.freebsd.org/D15952

In D15858#336639, @chuck wrote:

In D15858#336162, @emaste wrote:

See linux_map_osrel

This probably fits better with the existing code, and I'm inclined to move that direction. Is using linux_kernver() the way to access this value?

Could you use devel/arcanist, or at least generate a diff with full context like it does, with svn diff -x -U9999 or git diff -U9999.

Could you use devel/arcanist, or at least generate a diff with full context like it does, with svn diff -x -U9999 or git diff -U9999.

Moved NO_ARCH and updated diff.

Updated diff as asked.

add context

Could you use devel/arcanist, or at least generate a diff with full context like it does, with svn diff -x -U9999 or git diff -U9999.

All the NO_ARCH lines should go wondown a bit.

This approach is wrong. The problem I was trying to solve is the interrupt handler accessing a bad sc. The proper fix will be to instead make the interrupt handler use a sc, instead of a consdev, and move more state data into the sc.