In D15910#338114, @alc wrote:

In D15910#338113, @alc wrote:

In D15910#338097, @markj wrote:

Isn't this invalid if both the old and new mappings are wired? Or should it not be possible

... for that situation to arise when the mappings are to different physical pages?

To be clear, I'm assuming that the problem that Kostik is worried about is having "spurious" faults on mlock()ed memory. However, when we wire mappings (and the underlying physical pages) during mlock(), we simulate the COW faults. So, in the normal case of mlock(), I assert that there is not a problem. I speculate that setting a breakpoint in the code of an mlockall()ed application might be the one scenario where a temporarily zeroed PTE could occur. Do I need to argue that a spurious page fault in that scenario isn't really of concern. :-)

And for completeness, in the case of a fork of an mlockall()ed application, we preemptively copy all writeable data. (During the fork, other threads are paused from executing, right?)

In D15910#338067, @alc wrote:

In D15910#337632, @kib wrote:

In D15910#337582, @alc wrote:

1. In regards to pmap_enter(), we should aim to kill two birds with one stone. Recall the copy-on-write mapping bug that Kostik worked around in vm_fault(). I say worked around because the root cause is here in pmap_enter(). When the physical page mapped at va is changing, pmap_enter() should destroy the old mapping before creating the new one. Once pmap_enter() is restructured in this way, you can simply recycle the old mapping's PV entry.

Can you elaborate more, please ? What do you mean by destroying the old mapping ? In particular, do you mean installing the pte with clear PG_V into the changing PTE ?

Yes, I mean destroying the PTE, including TLB shootdown. In effect, briefly the PTE will be 0. Then, installing the new PTE is just a pte_store(). All of the stuff that we currently perform under "if ((origpte & PG_V) != 0) {" will already have been performed when we destroyed the PTE.

As I said I believe it is fine functionally.

This is fine.

It looks fine to me, I will wait some time for John opinion.

In D15910#337582, @alc wrote:

1. In regards to pmap_enter(), we should aim to kill two birds with one stone. Recall the copy-on-write mapping bug that Kostik worked around in vm_fault(). I say worked around because the root cause is here in pmap_enter(). When the physical page mapped at va is changing, pmap_enter() should destroy the old mapping before creating the new one. Once pmap_enter() is restructured in this way, you can simply recycle the old mapping's PV entry.

Other than two notes I put inline, the patch looks fine to me.

You can create symbols which are exported but not linkable, since they do not provide a default version. Such symbol can be only created by asm '@' syntax, it should be removed from the version map. Also I do not see a sense in leaving the private symbols around.

In D15857#336986, @yanko.yankulov_gmail.com wrote:

I think John' idea was to move the block which sets the has_ptrace_fork variable to true, down to the code which acts on its true value. De-fact, eliminating the var, and removing one if().

OK, but as far as I can tell we will need p1's PROC_LOCK or the proctree_lock in order to check the PTRACE_FORK flag. Am i missing something obvious again :) ?

In D15857#336018, @yanko.yankulov_gmail.com wrote:

Address 2 of the jhb comments.

Have no idea about moving other code around.

This is somewhat orthogonal, but since you are makes the init the proper child of the proc0, shouldn't proc0 get the P_TREE_REAPER flag ? Otherwise, dying init would confuse the reaping code. We do allow init(8) to die sometimes, without inducing the panic.

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.

This should be fine.

In D15570#335994, @bz wrote:

In D15570#335980, @kib wrote:

In D15570#335888, @bz wrote:

In D15570#328824, @kib wrote:

grep for FOREACH_PROC_IN_SYSTEM() for the start. In fact these places are already visible in the patch because they require allproc_lock.

Yeah I got that bit. I was more wondering about ..

But the more important question still stands. Some of the uses of the global lists should go, but some, esp. for VM, probably must stay. As I noted, this requires preliminary architectural discussion.

.. exactly one of these esp for VM, where it must stay. Can you just point out one specific example.

In the network stack we have VNET_FOREACH() as an outside iterator over all virtual (network stack) instances and then can iterate over all per-virtual-instance lists. I am wondering if the same could be done here..

In D15570#335888, @bz wrote:

In D15570#328824, @kib wrote:

I have no idea about the global plan and most of the implementation details, so I am replying from the common sense PoV. I am completely not sure about the split of the allproc_lock and proctree_lock into per-VPS locks. For often need to iterate over all processes in the tree, e.g. to make a decision about the global memory subsystem state. In this case, the global process list and the global allproc_lock are right, but per-VPS lists and locks are not. Of course, unless you also partition the physical memory (but I doubt that it makes sense).

In other words, there still must be a global lock, and there probably should be per-container locks. But this requires initial architectural discussion and agreement on the major points, which would allow to make such decisions later without much reconsiderations.

Can you give me a good example where we currently do this? The iterations over all processes won't really work anymore given there's no single all-processes list anymore.
I'd like to have a look at a good example (as you mention the memory subsystem maybe there) so I can give you a more informed description for discussion.

grep for FOREACH_PROC_IN_SYSTEM() for the start. In fact these places are already visible in the patch because they require allproc_lock.

I was not able to imagine a case which is broken by this simplification. Also, the ptrace_test works with the patch.

In D15823#335084, @yanko.yankulov_gmail.com wrote:

I checked the rest of the RFSTOPPED use points and applying the change seems trivial. So if the general idea is acceptable I can apply the changes to all the places and upload a new patch.

Right now I think that this is working approach.

Please generate large diff context when you upload diff into phab, e.g. for svn it would be svn diff -x -U999999, for git diff -U999999.

In D15648#334613, @hselasky wrote:

Just make sure buildworld WITH_OFED -jXXX does not break.

Tinderbox and installworld tests passed with WITH_OFED=yes. I think that the patch is ready to go.

Remove SYSPROTO for them all, instead of patching ? It is not useful for current syscalls, and obviously even less so for compat.

I believe that stubs which return ENOSYS are fine, without redirecting to the syscalls. More, the stubs do not need to provide the default version, so that linking with the symbols will be impossible any more.

Fix libpcap dependency on libmlx5

In D15809#334292, @mjg wrote:

What is the purpose of this code to begin with? It looks like it should just be removed. If it is needed (what for?), it probably has to run after all initial forking is finished.

Code makes consistent early processes start time vs rusage.

Handle Brian' notes.

Add optional start:count for pciconf -D.
Update pciio(4) and pciconf(8) man pages.

Peter, could you, please, test this patch ? I do not think that it is feasible to try to reproduce the original problem, but the generic testing would be useful.

Remove use of vm_page_next(). Add comment.

Fixed bug with the iteration without lock. Implement other suggestions too.

Swap the patch for the swap_pager.c patch.

In D15293#333512, @alc wrote:

If we delay the allocation of the ahead pages until after the buffer is allocated, in other words, after we have dropped and reacquired the object lock, then I believe that we can still use the maxahead result from the swap_pager_haspages() call that was performed before the object lock was dropped. If any of that swap space was invalidated while the object was unlocked, there should now be a resident page at that pindex that stops us from attempting to read from the now invalid swap space.

In D15293#333432, @alc wrote:

For what it's worth, I believe that this race is fallout from the pager interface changes for sendfile. Before those changes, we would have adjusted readbehind and performed the page allocations before calling the pager and releasing the object lock.

I suspect it might be useful to get an ack from the interrupt thread that it sees the IH_SUSP flag, so that device power down does not occur while the handler still run ?

I do not have any important notes about this change except the error code when raced.

In D15293#333070, @alc wrote:

To be clear, the clipping of ahead and behind after the object lock is reacquired should likely remain, but we should also clip behind before the object lock is released. It's also worth considering whether this new clipping should occur in vm_fault_hold(). Currently, I believe that only the swap pager is vulnerable, but it still might be argued that vm_fault_hold() should perform the new clipping.

It could also be argued that vm_object_split() should wait out the paging-in-progress indication on the top-level object by vm_fault_hold().

In D15293#333055, @alc wrote:

The change tightly alias the worker wakeup to the period ticks. It might result at least in the cosmetics issues with the load average.

In D15293#332786, @alc wrote:

Can't we simply modify the restart so that it doesn't find_least from the original pindex, but instead at the busy page's pindex? In other words, ...