In D23517#516688, @markj wrote:

In D23517#516336, @jtl wrote:

In D23517#516299, @markj wrote:

So, the problem manifests as the laundry queue steadily growing without any swapping in response?

We noticed this when we tried enabling encrypted swap. On the console, we see a string of processes killed due to the server being out of memory. Then, eventually, the watchdog timer fires and kills the system. I don't know what triggers this cycle. It seems to happen on a small percentage of systems hours to days after boot. To the best of my knowledge, we have not been able to observe a system descend into this naturally.
We tried to recreate the problem by artificially creating memory pressure. We ran a program that allocates a lot of memory (equal to the sum of the free and inactive sizes) and sequentially writes to each page in a loop. When we did this, we saw:

1. The laundry size is growing.
2. Processes are continually killed due to low memory.
3. Finally, the watchdog kicks in and reboots the system.

Now, we may have recreated a different problem that has similar symptoms. But, at minimum, it seems like this is showing *a* bug.

I've done tests like this in the past. Indeed, if the program is dirtying pages quickly enough we might give up an attempt an OOM kill, but we should definitely be targetting the runaway process first. It's possible that we may have swapped its kernel stack out, in which case I believe we have to swap it back in to reclaim anything, since reclamation happens during SIGKILL-triggered process exit. (I don't see offhand why another thread couldn't call pmap_remove_pages() on the target process before it is swapped back in though.)
I have not tried testing with a GELI-backed swap device though. Presumably you were using one? Do you see a difference in behaviour when swap is unencrypted?

In D23517#516299, @markj wrote:

Note that the page daemon uses vmd_free_target as the PID controller set point, but its target may be larger than the instantaneous difference vmd_free_target - vmd_free_count. So if it manages to free enough pages to satisfy vm_paging_target(), but not enough to satisfy the PID controller target, it'll trigger the laundry thread's shortfall mode, which then does nothing (unless pageout_deficit happens to be bigger than the negative difference). This suggests to me that the page daemon should be storing the value of page_shortage in the vmd_shortage field. In other words, the page daemon has failed to meet its target by page_shortage pages, and the laundry thread should try and make up that difference.

In D23517#516299, @markj wrote:

So, the problem manifests as the laundry queue steadily growing without any swapping in response?

@bz will commit.

Basically commited by @bz .

In D21840#477432, @rscheff_gmx.at wrote:

TCP_INFO is not portable afaik; however, I wonder why the linux varian has a flag for "SYN_DATA" (which may get delivered to the appliacation if the TFO is present), but no flag for TFO, while this change is signaling the presence of TFO, but no SYN_DATA... Just curious...

In D19622#476255, @bz wrote:

In a later call I think your suggestion was along the lines of:
(a) if ifnet goes away nuke the recvif pointer from the queued mbufs
(b) if reassembly times out do as outlined above and skip sending ICMP error/per-IF statistics/.. if the ifnet pointer was nuked
(c) if another fragment arrives (or the last fragment to complete the packet arrives) use that ones recvif pointer as that interface is expected to still be there to pass the packet on
Now there is a gray area between (b) and (c) in which (b) could be extended to "if we cannot find an ifnet pointer in the expected place, scan the fragments of the packet in question for any ifnet pointer and use that one" for error handling. It'd be a one-time slightly more expensive operation. If there's an attack however that is kind-of the extra work you'd want to avoid. Without the extra work, you may not find out as easily what kind of problem you are running into though as you are lacking statistics. Trade-off...

In D14387#476034, @bz wrote:

This may sound like a pain but as you already say in your message, this is two changes .. First .. Second ..
Can you split them up into such? First should be really easy to review and second should then be straight forward as well by itself.

Switch to using callout_init_mtx to let the callout system acquire the pause lock.

An alternative, better, version was implemented by @mjg in rS339449.

This was largely committed by mmacy last year.

This looks like it does what is described. As I understand, the use of delivered_data will be covered by a separate review. It looks like this will slightly change the way sacked_bytes is calculated. The change is probably a good thing, but it is worth verifying (and I haven't done this yet) that the updated calculation will work correctly.

In my local tree, I added functions to generate fake machine check records and exercise the logic now found in mca_process_records(). It appears to work correctly. I *think* this is now ready to land.

Changes based on review:

• Switched to using STAILQ_CONCAT to refill the free list.
• Dropped redundant calls to resize the free list.
• Centralized the logging logic to reduce code duplication.

Incorporate feedback from @jhb.

Try 2:
Maintain the last N records in the mca_records list. N is user-configurable and defaults to -1 (unlimited; the current behavior).

In D20482#442176, @jhb wrote:

So I have had tools in the past that parsed the list from the kernel. See https://github.com/freebsd/freebsd/compare/master...bsdjhb:libsmbios_ecc. At the very least I think there should perhaps be a tunable/sysctl to control the behavior.

In D20109#437264, @hselasky wrote:

@glebius: Multicast destruction is deferred. When we destroy a multicast address we need to call the if_ioctl of the belonging network interface to remove any multicast addresses. That's the problem. I think draining is a good way to implement a safe solution instead of using refcounts. Then we ensure that the ifnet is in a certain state when the multicast destruction callbacks are invoked.

In the interests of avoiding discussion fragmentation, I am adding the feedback from the transport working group meeting today.

Accepting the inp change as transport role to unblock the review.

In D19960#429052, @bz wrote:

That said, there are vendors who list it (maybe not on the forwarding plane) but as RFC compliance supporting it:
https://www.juniper.net/documentation/en_US/junos/topics/reference/standards/ipv6.html

I agree with @bz about ideal process. I also agree with @kristof about the practical implications of this feature. :-)

LGTM

I'm confused how a RST could have tripped this assert. In that case, len should have been 0 and ((th_flags) & (TH_SYN | TH_FIN)) == 0 should have been true (i.e. neither SYN nor FIN was set). In other words, it looks to me as if a RST should already pass the assert without tripping it. Can you explain further what I'm missing?

Thanks!

Approved.

(FYI, the revision looks incorrect. It looks like it should have been r342597.)

Approved

Please feel free to commit with "Approved by: jtl".

Please feel free to commit with "Approved by: jtl".

@jonathan , @pjd : Does this change look okay to you? You both committed some of the bits being reverted (although, its been 7-8 years since then).

I assume this is the stable/12 version of D19150?

Feel free to commit with "Approved by: jtl"

Feel free to commit with "Approved by: jtl".

Please commit with "Approved by: jtl"

Has this landed yet?

Please commit with Approved by: jtl

By the way, in the commit message, please make it clear that this simple "allows" you to more easily use arc if you want your changes to be reviewed. It doesn't require it in any way, shape, or form. And, we're not changing any expectations regarding when someone will get review.

LGTM

I'm OK with importing the code as an option for developers to use. I'd be wary of specifying that there is a "plan" to convert existing test cases unless there is general buy in to do that.

Regardless of what the group says, it seems like we'll probably want to support both for a transitional period. Given that, this change seems OK. @emaste , do you agree?

Since make clean runs first, I'm assuming the only things left will be things with flags set? If so, what you propose is probably correct (and we don't need the rm/chflags/rm "dance" optimization).

LGTM; would appreciate also hearing @rrs's opinion.

In D17914#382797, @markj wrote:

I'm writing a regression test for this to stick under tests/sys/netinet. I'll see if I can add test cases for D17922 as well.

Yeah, I saw this shortly before seeing this review. I agree we should fix it.

If alignment is important, we can (theoretically) do something more intelligent to maintain it. However, I think this is both safe and good enough for an emergency fix.

In D17609#375841, @mjg wrote:

I think this is a very questionable idea. In my experience clang does *mostly* not re-read. For all cases you are adding a function call.
See vfs_ref/vfs_rel or fsetown as examples.
Can you show me a specific function which compiles to multiple reads of gs just to get curthread?

In D17604#375736, @kib wrote:

I definitely agree with the genoffset.sh changes.
On the other hand, I am not sure that we need to restore volatile qualifiers in the thread_lite, instead of removing them in the struct thread. The manipulations of the critnest level and pin count should use barriers, and I believe that the thread_lite patch and some of its follow-ups just did that. Volatile relied on the compiler-specific semantic to get similar effect.