In D26548#591453, @markj wrote:

Indeed, there seems to be a race in ng_l2tp_seq_xack_timeout() as well.

In D26548#591451, @lutz_donnerhacke.de wrote:

My problem is, that I do not understand, how this race condition can happen at all.

Thanks for writing tests.

In D26548#591438, @lutz_donnerhacke.de wrote:

In D26548#591373, @markj wrote:

In D26548#591305, @lutz_donnerhacke.de wrote:

Can you please point me to the bug report?

The PR that Aleksandr noted, https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=241133

A pfsense developer also reported seeing a null pointer dereference in ng_l2tp_seq_rack_timeout(), apparently because seq->xwin[0] == 0. Apparently the panic was observed even after this commit: https://svnweb.freebsd.org/base?view=revision&revision=353027 .

Thank you for the info.

We are running this node in a large scale production environment (with remote LAC at different carriers) and did not observe such an issue before.

How common are retransmits in this environment?

We had no problems with the L2TP tunnel since PR146082 was solved. There we run into long time stability problems with L2TP sequence numbers.
Yes we have retransmits, but I did not set up an special monitoring for this, because we use sequence numbers in L2TP only for the control channel, but not for the data channel.
Typically we see a permanent difference in sequence numbers of more than one (always sending data). May be the heavy traffic hides the problem, because we usually do not run into any timeout.
Of course, we have loss of connectivity between LAC and LNS, but never had a panic with a similar stack trace.

In D26548#591401, @lutz_donnerhacke.de wrote:

Is the bug reproducible in a test environment?

In D26548#591305, @lutz_donnerhacke.de wrote:

Can you please point me to the bug report?

Fix lock leak.

In D26538#590975, @roy_marples.name wrote:

In D26538#590877, @markj wrote:

Looks ok to me, thanks. This seems like it would be useful for programs that run under Capsicum.

Note that pledge does not allow SIOCGIFDATA - at all.

Could you explain why using a single TCP connection improves performance? I find it easy to believe that it does, but I'm fairly ignorant of TCP internals and would like to understand the reason for it.

Would you be willing to add some simple regression tests for these fixes based on the examples in the code review description? There are some tests already in usr.bin/col/tests/col_test.sh. The atf-sh-api man page has info on the shell functions you can use for writing tests, and there are lots of examples in the tree.

I'll give other folks a couple of days to have a look, and will commit if there are no objections.

In D26538#590896, @emaste wrote:

So AFAICT NetBSD populates ifdr->ifdr_data and is properly _IOWR, OpenBSD matches this patch (and is unnecessarily _IOWR), and DragonFlyBSD has added ifru_data to struct ifreq and matches the NetBSD approach other than not having a separate struct ifdatareq

Looks ok to me, thanks. This seems like it would be useful for programs that run under Capsicum.

Looks ok to me, my comments are just nitpicking.

Any objections to this change? I'd like to commit soon if not.

Any objections to this change? I'd like to commit soon if not.

Committed in r365976, I omitted the phabricator link by accident.

In D26041#590584, @arichardson wrote:

In D26041#590583, @markj wrote:

I have no objection to the change, it brings install(1)'s logic closer to that used in cp(1).

Do you have a benchmark which demonstrates a difference when read() is used instead of mmap() for small files?

No I don't and I doubt I'll have time to test this in the near future. My guess is that it probably doesn't make much of a difference for small files.

I have no objection to the change, it brings install(1)'s logic closer to that used in cp(1).

Looks ok to me aside from the man pages.

Assert PMAP_HAS_LARGEPAGES in vm_fault_populate().

Seems reasonable to me.

In D26467#589671, @kib wrote:

In D26467#589636, @markj wrote:

In D26467#589425, @kib wrote:

I did not do it this way because my feel was that requires asserts in other pmap_enter() that PMAP_ENTER_LARGEPAGE is not set. It might be fine as is.

Why does the current scheme not require such assertions? I don't mind adding them to this diff, or simply extending the ifdef in kern_shm_open2(). I just prefer having a central place to indicate PMAP_ENTER_LARGEPAGE support.

I am not advocating keeping the current scheme, but for me its advantage was that there is simple place where the support for LARGEPAGE is specified per-arch. With PMAP_HAS_LARGEPAGE I need to inspect all <arch>/include/vmparam.h.

If switching to this scheme, we should provide a pattern for pmap_enter() where somebody copying current implementation to a new architecture clearly see that the flag is handled or not. My scheme makes it correct without any action from the porter.

Assert that ATTR_DESCR_VALID is set if va < VM_MAX_USER_ADDRESS.

Add comments.

In D26507#589974, @allanjude wrote:

This needs to go to stable/12 then be merged to releng/12.2 (requires approval from re@)

Address feedback:

• Fix assertion message.
• Correct wired page accounting.

Address feedback:

• When mapping a large page, don't look up the PTP unless we need it.
• Simplify pmap wired count updates.
• Unconditionally issue dsb after updating a large page PTE.

In D26467#589425, @kib wrote:

I did not do it this way because my feel was that requires asserts in other pmap_enter() that PMAP_ENTER_LARGEPAGE is not set. It might be fine as is.

Address feedback:

• Use pmap_unuse_pt().
• Fix resident page accounting, second attempt.

Seems reasonable to me.

Address review feedback.

In D26463#589164, @kib wrote:

Hm, unless I miss something, does pmap_copy() break MINHERIT_ZERO ?

• Adjust assertion messages.
• Implement pmap_copy() support.

Drop a test printf().

Implement handling for 1G pages in pmap_copy().

I plan to commit this later today if there are no objections.

In D26453#589070, @markj wrote:

It looks like library targets depend on .PHONY, which might explain this. I'm no make wizard though.

In D26453#588916, @bdragon wrote:

Regarding installing it multiple times, I suspect that something is going wrong in the target up to date logic for the library.

Poking through a (meta-mode against already-built world to reduce number of rebuilt targets, but I assume the same problem exists in a "normal" build) build with "-d m" to dump out logic, I see the following:

Examining libgcc_s.so.1.full...
modified 19:54:21 Sep 16, 2020...out-of-date
Building /usr/obj/root/bdragon28-freebsd/powerpc.powerpc64/lib/libgcc_s/libgcc_s.so.1.full
--- libgcc_s.so.1.full ---
building shared library libgcc_s.so.1
Make_Update: libgcc_s.so.1.full
 recheck(libgcc_s.so.1.full): current update time: 19:55:47 Sep 16, 2020
inspect parent libgcc_s.so.1: flags 9, type b020001, made 1, unmade 1 - unmade children
inspect parent libgcc_s.so.1.debug: flags 9, type b020001, made 1, unmade 0 - libgcc_s.so.1.full made, schedule libgcc_s.so.1.debug (made 1)

....

Examining libgcc_s.so.1.full...
modified 19:55:47 Sep 16, 2020...out-of-date
Building /usr/obj/root/bdragon28-freebsd/powerpc.powerpc64/lib/libgcc_s/libgcc_s.so.1.full
--- libgcc_s.so.1.full ---
building shared library libgcc_s.so.1
Make_Update: libgcc_s.so.1.full
 recheck(libgcc_s.so.1.full): current update time: 19:55:53 Sep 16, 2020
inspect parent libgcc_s.so.1: flags 9, type b020001, made 1, unmade 1 - unmade children
inspect parent libgcc_s.so.1.debug: flags 9, type b020001, made 1, unmade 0 - libgcc_s.so.1.full made, schedule libgcc_s.so.1.debug (made 1)

NOTE: This is without this phab, I have not retested things to see if there are any differences with it.

so I wonder if make itself is getting confused about the state of the target.

Not that I understand much about how make works internally. It does make me wonder though.