Page MenuHomeFreeBSD

iflib: unify prefetching & enable for non-x86
ClosedPublic

Authored by gallatin on Sat, Oct 10, 10:43 PM.
Tags
None
Referenced Files
F175724883: D60602.diff
Mon, Oct 12, 7:49 PM
F175671550: D60602.diff
Mon, Oct 12, 10:17 AM
F175627990: D60602.id189424.diff
Mon, Oct 12, 3:48 AM
F175625281: D60602.id189422.diff
Mon, Oct 12, 3:23 AM
Unknown Object (File)
Sun, Oct 11, 11:30 PM
Unknown Object (File)
Sun, Oct 11, 11:21 PM
Unknown Object (File)
Sun, Oct 11, 8:21 PM
Unknown Object (File)
Sun, Oct 11, 4:28 PM

Details

Summary

This change makes all prefetching use the iflib prefetch wrappers, rather
than using prefetch() in some places and __builtin_prefetch() in others.

Note that __builtin_prefetch() does not work in the kernel on amd64 as clang
disables it when -mnosse is specified,, so we must use the asm directives there,
and builtin_prefetch() elsewhere.

This change enables prefetching on non-x86 (notably arm64), and it enables
prefetching in netmap that has been there since day one, but which has never worked.

This is a no-op on non-netmap amd64 and i386

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

It looks like rrs added a sys/kern_prefetch.h... I would say use/move the definitions there if you must maintain wrappers (and drop the amd64 case in it).

Otherwise (eventually) drop that file and use __builtin_prefetch directly (my preference). The 2lines variant would be dead code after simpletx so it can remain as some short term hack.

gallatin edited the summary of this revision. (Show Details)
gallatin added a reviewer: vmaffione.

It looks like rrs added a sys/kern_prefetch.h... I would say use/move the definitions there if you must maintain wrappers (and drop the amd64 case in it).

Otherwise (eventually) drop that file and use __builtin_prefetch directly (my preference). The 2lines variant would be dead code after simpletx so it can remain as some short term hack.

My intention is to just get prefetching working for arm64 in iflib. His kern_prefetch uses a different level assembly instruction (nt0 in iflib, vs nt1 in rrs code).
The difference is that nt1 skips l1 so as to not evict stuff. So that would be a performance change. Its arguably better, but I'd want to measure it.

Otherwise (eventually) drop that file and use __builtin_prefetch directly (my preference). The 2lines variant would be dead code after simpletx so it can remain as some short term hack.

My initial stupid test didn't catch the fact that __builtin_prefetch doesn't work on x86 because clang skips prefetch if you compile with -mno-sse. So we need the wrapper.
For now, I just want to get prefetching working on iflib for arm64. I'll try to run some tests on nt0 vs nt1, which we'd want to do before using kern_prefetch

Otherwise (eventually) drop that file and use __builtin_prefetch directly (my preference). The 2lines variant would be dead code after simpletx so it can remain as some short term hack.

My initial stupid test didn't catch the fact that __builtin_prefetch doesn't work on x86 because clang skips prefetch if you compile with -mno-sse. So we need the wrapper.
For now, I just want to get prefetching working on iflib for arm64. I'll try to run some tests on nt0 vs nt1, which we'd want to do before using kern_prefetch

builtin_prefetch can do locality with 3 args. I wonder if there is some way to overcome this mno-sse side-effect, that seems like an awkward behavior by the toolchain. Otherwise I'd nudge toward kern_prefetch (and it naturally namespaces instead of claiming bare 'prefetch' keyword which could cause issues later on).. adding variants or varadics if we want locality control.

But there is certainly nothing wrong as proposed because you're just moving the existing code and fixing up the definitions.

This revision is now accepted and ready to land.Sun, Oct 11, 12:09 AM
FreeBSD/sys/net/iflib.c
919 ↗(On Diff #189424)

You don't need underscores before the C99 inline keyword in kernel compilation units. It is needed only in the public headers installed in /usr/include to stay compatible with relic compilers.

My intention is to just get prefetching working for arm64 in iflib. His kern_prefetch uses a different level assembly instruction (nt0 in iflib, vs nt1 in rrs code).

Yep, and Randall's intention was also to just get his things working in HPTS :) And before that Macy's intention was also to get his things working in iflib (but not on arm64). It would be really great if we don't need a fourth person to achieve his intentions before we get some standardized KPI for that. :)

My intention is to just get prefetching working for arm64 in iflib. His kern_prefetch uses a different level assembly instruction (nt0 in iflib, vs nt1 in rrs code).

Yep, and Randall's intention was also to just get his things working in HPTS :) And before that Macy's intention was also to get his things working in iflib (but not on arm64). It would be really great if we don't need a fourth person to achieve his intentions before we get some standardized KPI for that. :)

Yeah, this is turning into a can of worms. The problem is that I cannot find any __builtin_prefetch() args that are a win for Randall's prefetcher with TCP on arm64, so I want to leave it alone. We probably want to test disabling it for amd64 as well. The iflib case is much more clear cut. The default builtin_prefetch() args generate the same prefetchnt0 that iflib already uses and generate a clear performance win.

This revision was automatically updated to reflect the committed changes.