Super helpful review, John. I just opened a new review (https://reviews.freebsd.org/D47583) for the simplest suggested change. Will work on your other suggestions.

Address Kib's feedbackj

In D47294#1078779, @markj wrote:

In D47294#1078776, @gallatin wrote:

Why do we want or need a hardcoded list? Why can't this function be more like lagg_capabilities()? If we do want a hardcoded list, what about IFCAP_TXTLS*

That's a good question. if_bridge could probably be smarter, indeed.

Why exactly does if_bridge need to care about IFCAP_TXTLS*?

Why do we want or need a hardcoded list? Why can't this function be more like lagg_capabilities()? If we do want a hardcoded list, what about IFCAP_TXTLS*

I'd personally want to keep these messages with bootverbose.. I can imagine it might be handy to see them at times...

Fix style issue pointed out by Mark

Why is this re-surfacing?

The a/b results were not surprising (boring as David likes to say). Just slightly higher CPU on the canary (due to the increased irq rate). But no clear streaming quality changes.
All in all, it seems to work and do no real harm, but we'll not use it due to the increased CPU

Yeah, my ideal irq rate/queue is < 1000 . We mostly use Chelsio and Mellanox NICs that can do super aggressive irq coalescing without freaking out TCP due to using RX timestamps. Super aggressive coalescing like this lets us build packet trains in excess of 1000 packets to feed to lro via RSS assisted LRO, and we actually have useful LRO on internet workloads with tens of thousands of TCP connections per-queue. That reminds me that I should port RSS assisted LRO to iflib (eg, lro_queue_mbuf()).

In D30155#1074005, @kbowling wrote:

In D30155#1073987, @gallatin wrote:

In D30155#1073639, @kbowling wrote:

@imp @gallatin if you are able to test your workload, setting this to 1 and 2 would be new behavior versus where you are currently:

I can pull this into our tree and make an image for @dhw to run on the A/B cluster. However, we're not using this hardware very much any more, and there is only 1 pair of machines using it in the A/B cluster. Lmk if you're still interested, and I'll try to build the image tomorrow so that David can test it at his leisure.

Sure, it sounds like that is only enough for one experiment so I would focus on the default algorithm the patch will boot with sysctl dev.ix.<N>.enable_aim=1

In D30155#1073639, @kbowling wrote:

In D45950#1073880, @jhb wrote:

I don't think we need the taskqueue. It's probably just a design copied from the Intel drivers, and I don't think it makes much sense for those either. The other thing that can be nice to do though when making this change is to instead build a temporary list of packets linked via m_nextpkt (mbufq works for this) and pass an entire batch to if_input. This lets you avoid dropping the lock as often.

In D46761#1070797, @kib wrote:

No, the lock cannot be sleepable because the processing occurs in the context of the interrupt thread.

I would implemented something with blockcount_t or even epoch, but then I realized that it would not help. blockcount cannot be used because rx memory is not type-stable. Driver-private epoch might work, but then note that the second reported backtrace in the PR 281368 shows ip stack acquiring sleepable lock. So even if I try to fix driver, the stack still tries to sleep in ip_input().

I suspect you (Netflix) did not see the deadlock because you either do not use ipv6 or use it in situation with static network configuration. The problems are visible when multicast group membership is changed, at least this is what I see in the PR.

Would it be better to call mb_unmapped_to_ext() here ?

Ah, OK, I understand now.

I'm very afraid there will be performance implications due to new cache misses here from queing mbufs twice. On tens of thounsands of interfaces running over 8 years, we've never hit a deadlock from this lock, and I don't think fixing this is important enough to hurt performance for.

I'm confused.. if we are marking non-writable M_EXTPG mufs as M_RDONLY, why can't we simply remove the M_EXTPG check from M_WRITABLE? Why do we need a new macro?

This passes basic sanity testing at netflix. Sorry for the delayed approval; we had a few integration issues with this and a local Netflix feature that made it look like splice was not working. It only just now became obvious that it was due to our local feature & how to fix it.

In D45950#1048214, @markj wrote:

In D45950#1048085, @gallatin wrote:

Is this safe? I think so, but I confess that I don't know the low level details in this driver very well.

I believe so, from what I see, the lock exists to synchronize with the taskqueue and to protect some non-atomic counters.

When I read a network driver, I view a lock around rx processing as an indicator there is room for improvement in the design. The reason the lock seems to exist is to serialize rx ring servicing between the ithread (the normal path) and the taskqueue (which is woken if we continually find more packets to process... or maybe if interrupts don't work..?). I don't really understand code at the bottom of vtnet_rx_vq_process(). It seems like interrupts should be disabled if switching to the taskqueue, and enabled if returning from it. It probably has something to do with the "race" mentioned in that code..

I don't quite understand that either. The comment above the definition of VTNET_INTR_DISABLE_RETRIES suggests to me that the idea is:

• vtnet_rxq_eof() returns 0, so more == 0, i.e., there were no more descriptors to process.
• vtnet_rxq_enable_intr() returned 1, meaning that we found some completed descriptors on the queue when enabling interrupts.
• We should call vtnet_rxq_eof() again to collect those newly completed descriptors instead of deferring.

I'm not sure I understand the purpose of the taskqueue at all though. Why can't we handle all packets in the ithread context?

Is this safe? I think so, but I confess that I don't know the low level details in this driver very well.

I was concerned at first about isal, but then I remembered that @jhb had moved it from plugging in at the ktls layer to plugging in at the ocf layer

In D45224#1031599, @alc wrote:

@gallatin @markj Could you please test this patch? I've also tested this patch on EC2 VMs with both 4K and 16K base pages, but their device mappings don't trigger any L1 or L2C demotions in the direct map.

In D45042#1029022, @alc wrote:

In D45042#1028354, @gallatin wrote:

In D45042#1028058, @alc wrote:

In D45042#1027957, @markj wrote:

Do we have any idea what the downsides of the change are? If we make the default 64KB, then I'd expect memory usage to increase; do we have any idea what the looks like? It'd be nice to, for example, compare memory usage on a newly booted system with and without this change.

I had the same question. It will clearly impact a lot of page granularity counters, at the very least causing some confusion for people who look at those counters, e.g.,

./include/jemalloc/internal/arena_inlines_b.h-          arena_stats_add_u64(tsdn, &arena->stats,
./include/jemalloc/internal/arena_inlines_b.h-              &arena->decay_dirty.stats->nmadvise, 1);
./include/jemalloc/internal/arena_inlines_b.h-          arena_stats_add_u64(tsdn, &arena->stats,
./include/jemalloc/internal/arena_inlines_b.h:              &arena->decay_dirty.stats->purged, extent_size >> LG_PAGE);
./include/jemalloc/internal/arena_inlines_b.h-          arena_stats_sub_zu(tsdn, &arena->stats, &arena->stats.mapped,
./include/jemalloc/internal/arena_inlines_b.h-              extent_size);

However, it's not so obvious what the effect on the memory footprint will be. For example, the madvise(MADV_FREE) calls will have coarser granularity. If we set the page size to 64KB, then one in-use 4KB page within a 64KB region will be enough to block the application of madvise(MADV_FREE) to the other 15 pages. Quantifying the impact that this coarsening has will be hard.

This does, however, seem to be the intended workaround: https://github.com/jemalloc/jemalloc/issues/467

Buried in that issue is the claim that Firefox's builtin derivative version of jemalloc eliminated the statically compiled page size.

What direction does the kernel grow the vm map? They apparently reverted support for lg page size values larger than the runtime page size because it caused fragmentation when the kernel grows the vm map downwards..

Typically, existing map entries are only extended in an upward direction. For downward growing regions, e.g., stacks, new entries are created. Do you have a pointer to where this is discussed? I'm puzzled as to why the direction would be a factor.

In D45042#1028058, @alc wrote:

In D45042#1027957, @markj wrote:

Do we have any idea what the downsides of the change are? If we make the default 64KB, then I'd expect memory usage to increase; do we have any idea what the looks like? It'd be nice to, for example, compare memory usage on a newly booted system with and without this change.

I had the same question. It will clearly impact a lot of page granularity counters, at the very least causing some confusion for people who look at those counters, e.g.,

./include/jemalloc/internal/arena_inlines_b.h-          arena_stats_add_u64(tsdn, &arena->stats,
./include/jemalloc/internal/arena_inlines_b.h-              &arena->decay_dirty.stats->nmadvise, 1);
./include/jemalloc/internal/arena_inlines_b.h-          arena_stats_add_u64(tsdn, &arena->stats,
./include/jemalloc/internal/arena_inlines_b.h:              &arena->decay_dirty.stats->purged, extent_size >> LG_PAGE);
./include/jemalloc/internal/arena_inlines_b.h-          arena_stats_sub_zu(tsdn, &arena->stats, &arena->stats.mapped,
./include/jemalloc/internal/arena_inlines_b.h-              extent_size);

However, it's not so obvious what the effect on the memory footprint will be. For example, the madvise(MADV_FREE) calls will have coarser granularity. If we set the page size to 64KB, then one in-use 4KB page within a 64KB region will be enough to block the application of madvise(MADV_FREE) to the other 15 pages. Quantifying the impact that this coarsening has will be hard.

This does, however, seem to be the intended workaround: https://github.com/jemalloc/jemalloc/issues/467

Buried in that issue is the claim that Firefox's builtin derivative version of jemalloc eliminated the statically compiled page size.

In D45042#1027404, @andrew wrote:

I've been thinking about adding PAGE_SIZE_MAX/PAGE_SHIFT_MAX or similar to arm64 to define the largest page size the kernel could support. We could then use that here if it's defined.

In D40676#1027002, @markj wrote:

In D40676#1027000, @gallatin wrote:

After this change, ktrace output is littered with 'CAP system call not allowed: $SYSCALL' on systems w/o capsicum enabled, which is confusing and distracting. Can this please be reverted to behave without CAP output for systems w/o capsicum ?

This was done already in commit f239db4800ee9e7ff8485f96b7a68e6c38178c3b.

After this change, ktrace output is littered with 'CAP system call not allowed: $SYSCALL' on systems w/o capsicum enabled, which is confusing and distracting. Can this please be reverted to behave without CAP output for systems w/o capsicum ?

I consulted with @imp, and after a trip down the rabbit hole, we concluded that a header file consisting only of the definition of MAXPHYS is not creative (as this is the only way to express this in C) so it can't have copyright protection, and should simply be public domain.

• Update diff to avoid cutting/pasting MAXPHYS definition as per @kib's suggestion

I just tripped over this again when trying to use some of the 16K changes I have in my Netflix tree on a personal machine running a GENERIC kernel, so let's try this again in a different way.

Thank you for adding that option.

In D43504#1017265, @markj wrote:

In D43504#1017242, @gallatin wrote:

Below are the results from my testing. I'm sorry that it took so long.. I had to re-do testing from the start b/c the new machine was not exactly identical to the old (different BIOS rev) and was giving slightly different results.
The results are from 92Gb/s of traffic over a one hour period with 45-47K TCP connections established/

No SDT probes: 56.4%
normal SDT 57.5%
new IP SDTs 57.9%
new IP SDT+ 56.6%
zero-cost

Just to be clear, "SDT+" is with the patch I supplied to provide new asm goto-based SDT probes? I'm not sure what the "zero-cost" line means.

This is just measuring CPU usage as reported by the scheduler?

I made some progress on the hot-patching implementation last week. I hope to have it ready fairly soon.

In D43504#1017265, @markj wrote:

In D43504#1017242, @gallatin wrote:

Below are the results from my testing. I'm sorry that it took so long.. I had to re-do testing from the start b/c the new machine was not exactly identical to the old (different BIOS rev) and was giving slightly different results.
The results are from 92Gb/s of traffic over a one hour period with 45-47K TCP connections established/

No SDT probes: 56.4%
normal SDT 57.5%
new IP SDTs 57.9%
new IP SDT+ 56.6%
zero-cost

Just to be clear, "SDT+" is with the patch I supplied to provide new asm goto-based SDT probes? I'm not sure what the "zero-cost" line means.

This is just measuring CPU usage as reported by the scheduler?

I made some progress on the hot-patching implementation last week. I hope to have it ready fairly soon.

Below are the results from my testing. I'm sorry that it took so long.. I had to re-do testing from the start b/c the new machine was not exactly identical to the old (different BIOS rev) and was giving slightly different results.
The results are from 92Gb/s of traffic over a one hour period with 45-47K TCP connections established/

In D43504#1014924, @gallatin wrote:

OK, starting with an unpatched kernel & working my way through the patches. I'll report percent busy for unpatched and various patches on our original 100G server (based around Xeon E5-2697A v4, which tends to be a poster-child for cache misses, as it runs very close to the limits of its memory bandwidth. I'll be disabling powerd and using TCP RACK TCP's DGP pacing.

This will take several days, as it takes a while to load up a server, get a few hours of steady-state, and unload it gently,.

OK, starting with an unpatched kernel & working my way through the patches. I'll report percent busy for unpatched and various patches on our original 100G server (based around Xeon E5-2697A v4, which tends to be a poster-child for cache misses, as it runs very close to the limits of its memory bandwidth. I'll be disabling powerd and using TCP RACK TCP's DGP pacing.

In D43504#1014487, @markj wrote:

Regarding SDT hotpatching, the implementation[1] was written a long time ago, before we had "asm goto" in LLVM. It required a custom toolchain program[2].

Since then, "asm goto" support appeared in LLVM. It makes for a much simpler implementation. I hacked up part of it and posted a patch[3]. In particular, the patch makes use of asm goto to remove the branch and data access. (The probe site is moved to the end of the function in an unreachable block.) The actual hot-patching part isn't implemented and will take some more work, but this is enough to do some benchmarking to verify that the overhead really is minimal. @gallatin would you be able to verify this?

I would also appreciate any comments on the approach taken in the patch, keeping in mind that the MD bits are not yet implemented.

[1] https://people.freebsd.org/~markj/patches/sdt-zerocost/
[2] https://github.com/markjdb/sdtpatch
[3] https://reviews.freebsd.org/D44483

In D43504#1013980, @tuexen wrote:

I agree, that introducing these probes should not have a performance hit. Since they exist in Solaris, I was assuming that there is no substantial performance hit. I would really like to see zero cost probes going into the tree.

In D43504#1013965, @gallatin wrote:

Guys, this is crazy. Every SDT probe does a test on a global variable. If this lands, it will cause a noticeable performance impact, especially in high packet rate workloads. Can we shelve this until / unless SDT is modified to insert nops rather than do tests on a global variable? Or put this under its own options EXTRA_IP_PROBES or something?

Guys, this is crazy. Every SDT probe does a test on a global variable. If this lands, it will cause a noticeable performance impact, especially in high packet rate workloads. Can we shelve this until / unless SDT is modified to insert nops rather than do tests on a global variable? Or put this under its own options EXTRA_IP_PROBES or something?