* [PATCH net-next] vhost: use "checked" versions of get_user() and put_user()
@ 2025-11-13 0:55 Jon Kohler
2025-11-13 1:09 ` Jason Wang
2025-11-14 18:54 ` David Laight
0 siblings, 2 replies; 27+ messages in thread
From: Jon Kohler @ 2025-11-13 0:55 UTC (permalink / raw)
To: Michael S. Tsirkin, Jason Wang, Eugenio Pérez, kvm,
virtualization, netdev, linux-kernel
Cc: Jon Kohler, Linus Torvalds, Borislav Petkov, Sean Christopherson
vhost_get_user and vhost_put_user leverage __get_user and __put_user,
respectively, which were both added in 2016 by commit 6b1e6cc7855b
("vhost: new device IOTLB API"). In a heavy UDP transmit workload on a
vhost-net backed tap device, these functions showed up as ~11.6% of
samples in a flamegraph of the underlying vhost worker thread.
Quoting Linus from [1]:
Anyway, every single __get_user() call I looked at looked like
historical garbage. [...] End result: I get the feeling that we
should just do a global search-and-replace of the __get_user/
__put_user users, replace them with plain get_user/put_user instead,
and then fix up any fallout (eg the coco code).
Switch to plain get_user/put_user in vhost, which results in a slight
throughput speedup. get_user now about ~8.4% of samples in flamegraph.
Basic iperf3 test on a Intel 5416S CPU with Ubuntu 25.10 guest:
TX: taskset -c 2 iperf3 -c <rx_ip> -t 60 -p 5200 -b 0 -u -i 5
RX: taskset -c 2 iperf3 -s -p 5200 -D
Before: 6.08 Gbits/sec
After: 6.32 Gbits/sec
As to what drives the speedup, Sean's patch [2] explains:
Use the normal, checked versions for get_user() and put_user() instead of
the double-underscore versions that omit range checks, as the checked
versions are actually measurably faster on modern CPUs (12%+ on Intel,
25%+ on AMD).
The performance hit on the unchecked versions is almost entirely due to
the added LFENCE on CPUs where LFENCE is serializing (which is effectively
all modern CPUs), which was added by commit 304ec1b05031 ("x86/uaccess:
Use __uaccess_begin_nospec() and uaccess_try_nospec"). The small
optimizations done by commit b19b74bc99b1 ("x86/mm: Rework address range
check in get_user() and put_user()") likely shave a few cycles off, but
the bulk of the extra latency comes from the LFENCE.
[1] https://lore.kernel.org/all/CAHk-=wiJiDSPZJTV7z3Q-u4DfLgQTNWqUqqrwSBHp0+Dh016FA@mail.gmail.com/
[2] https://lore.kernel.org/all/20251106210206.221558-1-seanjc@google.com/
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Sean Christopherson <seanjc@google.com>
Signed-off-by: Jon Kohler <jon@nutanix.com>
---
drivers/vhost/vhost.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 8570fdf2e14a..ffbd0a9a7a03 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1442,13 +1442,13 @@ static inline void __user *__vhost_get_user(struct vhost_virtqueue *vq,
({ \
int ret; \
if (!vq->iotlb) { \
- ret = __put_user(x, ptr); \
+ ret = put_user(x, ptr); \
} else { \
__typeof__(ptr) to = \
(__typeof__(ptr)) __vhost_get_user(vq, ptr, \
sizeof(*ptr), VHOST_ADDR_USED); \
if (to != NULL) \
- ret = __put_user(x, to); \
+ ret = put_user(x, to); \
else \
ret = -EFAULT; \
} \
@@ -1487,14 +1487,14 @@ static inline int vhost_put_used_idx(struct vhost_virtqueue *vq)
({ \
int ret; \
if (!vq->iotlb) { \
- ret = __get_user(x, ptr); \
+ ret = get_user(x, ptr); \
} else { \
__typeof__(ptr) from = \
(__typeof__(ptr)) __vhost_get_user(vq, ptr, \
sizeof(*ptr), \
type); \
if (from != NULL) \
- ret = __get_user(x, from); \
+ ret = get_user(x, from); \
else \
ret = -EFAULT; \
} \
--
2.43.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH net-next] vhost: use "checked" versions of get_user() and put_user() 2025-11-13 0:55 [PATCH net-next] vhost: use "checked" versions of get_user() and put_user() Jon Kohler @ 2025-11-13 1:09 ` Jason Wang 2025-11-14 14:53 ` Jon Kohler 2025-11-14 18:54 ` David Laight 1 sibling, 1 reply; 27+ messages in thread From: Jason Wang @ 2025-11-13 1:09 UTC (permalink / raw) To: Jon Kohler Cc: Michael S. Tsirkin, Eugenio Pérez, kvm, virtualization, netdev, linux-kernel, Linus Torvalds, Borislav Petkov, Sean Christopherson On Thu, Nov 13, 2025 at 8:14 AM Jon Kohler <jon@nutanix.com> wrote: > > vhost_get_user and vhost_put_user leverage __get_user and __put_user, > respectively, which were both added in 2016 by commit 6b1e6cc7855b > ("vhost: new device IOTLB API"). It has been used even before this commit. > In a heavy UDP transmit workload on a > vhost-net backed tap device, these functions showed up as ~11.6% of > samples in a flamegraph of the underlying vhost worker thread. > > Quoting Linus from [1]: > Anyway, every single __get_user() call I looked at looked like > historical garbage. [...] End result: I get the feeling that we > should just do a global search-and-replace of the __get_user/ > __put_user users, replace them with plain get_user/put_user instead, > and then fix up any fallout (eg the coco code). > > Switch to plain get_user/put_user in vhost, which results in a slight > throughput speedup. get_user now about ~8.4% of samples in flamegraph. > > Basic iperf3 test on a Intel 5416S CPU with Ubuntu 25.10 guest: > TX: taskset -c 2 iperf3 -c <rx_ip> -t 60 -p 5200 -b 0 -u -i 5 > RX: taskset -c 2 iperf3 -s -p 5200 -D > Before: 6.08 Gbits/sec > After: 6.32 Gbits/sec I wonder if we need to test on archs like ARM. Thanks ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net-next] vhost: use "checked" versions of get_user() and put_user() 2025-11-13 1:09 ` Jason Wang @ 2025-11-14 14:53 ` Jon Kohler 2025-11-14 17:48 ` Linus Torvalds 2025-11-17 4:32 ` Jason Wang 0 siblings, 2 replies; 27+ messages in thread From: Jon Kohler @ 2025-11-14 14:53 UTC (permalink / raw) To: Jason Wang Cc: Michael S. Tsirkin, Eugenio Pérez, kvm@vger.kernel.org, virtualization@lists.linux.dev, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Linus Torvalds, Borislav Petkov, Sean Christopherson > On Nov 12, 2025, at 8:09 PM, Jason Wang <jasowang@redhat.com> wrote: > > !-------------------------------------------------------------------| > CAUTION: External Email > > |-------------------------------------------------------------------! > > On Thu, Nov 13, 2025 at 8:14 AM Jon Kohler <jon@nutanix.com> wrote: >> >> vhost_get_user and vhost_put_user leverage __get_user and __put_user, >> respectively, which were both added in 2016 by commit 6b1e6cc7855b >> ("vhost: new device IOTLB API"). > > It has been used even before this commit. Ah, thanks for the pointer. I’d have to go dig to find its genesis, but its more to say, this existed prior to the LFENCE commit. > >> In a heavy UDP transmit workload on a >> vhost-net backed tap device, these functions showed up as ~11.6% of >> samples in a flamegraph of the underlying vhost worker thread. >> >> Quoting Linus from [1]: >> Anyway, every single __get_user() call I looked at looked like >> historical garbage. [...] End result: I get the feeling that we >> should just do a global search-and-replace of the __get_user/ >> __put_user users, replace them with plain get_user/put_user instead, >> and then fix up any fallout (eg the coco code). >> >> Switch to plain get_user/put_user in vhost, which results in a slight >> throughput speedup. get_user now about ~8.4% of samples in flamegraph. >> >> Basic iperf3 test on a Intel 5416S CPU with Ubuntu 25.10 guest: >> TX: taskset -c 2 iperf3 -c <rx_ip> -t 60 -p 5200 -b 0 -u -i 5 >> RX: taskset -c 2 iperf3 -s -p 5200 -D >> Before: 6.08 Gbits/sec >> After: 6.32 Gbits/sec > > I wonder if we need to test on archs like ARM. Are you thinking from a performance perspective? Or a correctness one? ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net-next] vhost: use "checked" versions of get_user() and put_user() 2025-11-14 14:53 ` Jon Kohler @ 2025-11-14 17:48 ` Linus Torvalds 2025-11-14 19:08 ` David Laight 2025-11-17 4:32 ` Jason Wang 1 sibling, 1 reply; 27+ messages in thread From: Linus Torvalds @ 2025-11-14 17:48 UTC (permalink / raw) To: Jon Kohler Cc: Jason Wang, Michael S. Tsirkin, Eugenio Pérez, kvm@vger.kernel.org, virtualization@lists.linux.dev, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Borislav Petkov, Sean Christopherson On Fri, 14 Nov 2025 at 06:53, Jon Kohler <jon@nutanix.com> wrote: > > > On Nov 12, 2025, at 8:09 PM, Jason Wang <jasowang@redhat.com> wrote: > > > > It has been used even before this commit. > > Ah, thanks for the pointer. I’d have to go dig to find its genesis, but > its more to say, this existed prior to the LFENCE commit. It might still be worth pointing out that the LFENCE is what made __get_user() and friends much slower, but the *real* issue is that __get_user() and friends became *pointless* before that due to SMAP. Because the whole - and only - point of the __get_user() interface is the historical issue of "it translates to a single load instruction and gets inlined". So back in the dark ages before SMAP, a regular "get_user()" was a function call and maybe six instructions worth over code. But a "__get_user()" was inlined to be a single instruction, and the difference between the two was quite noticeable if you did things in a loop. End result: we have a number of old historic __get_user() calls because people cared and it was noticeable. But then SMAP happens, and user space accesses aren't just a simple single load instruction, but a "enable user space access, do the access, then disable user space accesses" for safety and robustness reasons. That's actually true on non-x86 architectures too: on arm64 you also have TTBR0_PAN, and user space accesses can be quite the mess of instructions. And in that whole change, now __get_user() is not only no longer inlined, the performance advantage isn't relevant any more. Sure, it still avoided the user address space range check, but that check just is no longer relevant. It's a couple of (very cheap) instructions, but the big reason to use __get_user() has simply gone away. The real costs of user space accesses are elsewhere, and __get_user() and get_user() are basically the same performance in reality. But the historical uses of __get_user() remain, even though now they are pretty much pointless. Then LFENCE comes around due to the whole speculation issue, and initially __get_user() and get_user() *both* get it, and it only reinforces the whole "the address check is not the most expensive part of the operation" thing, but __get_user() is still technically a cycle or two faster. But then get_user() gets optimized to do the address space check using a data dependency instead of the "access_ok()" control dependency, and so get_user() doesn't need LFENCE at all, and now get_user() is *faster* than __get_user(). End result: __get_user() has been a historical artifact for a long time. It's been discouraged because it's pointless. But with the LFENCE it's not only pointless, it's actively detrimental not just for safety but even for performance. So __get_user() should die. The LFENCE is not the primary reason it should be retired, it's only the last nail in the coffin. Linus ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net-next] vhost: use "checked" versions of get_user() and put_user() 2025-11-14 17:48 ` Linus Torvalds @ 2025-11-14 19:08 ` David Laight 2025-11-14 20:48 ` Linus Torvalds 0 siblings, 1 reply; 27+ messages in thread From: David Laight @ 2025-11-14 19:08 UTC (permalink / raw) To: Linus Torvalds Cc: Jon Kohler, Jason Wang, Michael S. Tsirkin, Eugenio Pérez, kvm@vger.kernel.org, virtualization@lists.linux.dev, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Borislav Petkov, Sean Christopherson On Fri, 14 Nov 2025 09:48:02 -0800 Linus Torvalds <torvalds@linux-foundation.org> wrote: ... > But then get_user() gets optimized to do the address space check using > a data dependency instead of the "access_ok()" control dependency, and > so get_user() doesn't need LFENCE at all, and now get_user() is > *faster* than __get_user(). I think that is currently only x86-64? There are patches in the pipeline for ppc. I don't think I've seen anything for arm32 or arm64. arm64 has the issue that the hardware looks at the wrong address bit, so might need an explicit guard page at the end of user addresses. Changing x86-32 to have a guard page ought to be straightforward. But I think the user stack ends right at 0xc000000 (with argv[] and env[]) so it might be safer to also reduce the stack size by 4k (pretending env[] is larger) to avoid problems with code that is trying to map things at fixed addresses just below the stack (or do we care about that?). I'm sure I should be able to build and test the x86-32 code. I guess there are instruction for doing that under qemu somewhere? Might be time to drop support for cpu that don't support cmov? David ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net-next] vhost: use "checked" versions of get_user() and put_user() 2025-11-14 19:08 ` David Laight @ 2025-11-14 20:48 ` Linus Torvalds 2025-11-14 21:38 ` David Laight 0 siblings, 1 reply; 27+ messages in thread From: Linus Torvalds @ 2025-11-14 20:48 UTC (permalink / raw) To: David Laight Cc: Jon Kohler, Jason Wang, Michael S. Tsirkin, Eugenio Pérez, kvm@vger.kernel.org, virtualization@lists.linux.dev, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Borislav Petkov, Sean Christopherson On Fri, 14 Nov 2025 at 11:09, David Laight <david.laight.linux@gmail.com> wrote: > > I think that is currently only x86-64? > There are patches in the pipeline for ppc. > I don't think I've seen anything for arm32 or arm64. Honestly, the fact that it's mainly true on x86-64 is simply because that's the only architecture that has cared enough. Pretty much everybody else is affected by the exact same speculation bugs. Sometimes the speculation window might be so small that it doesn't matter, but in most cases it's just that the architecture is so irrelevant that it doesn't matter. So no, this is not a "x86 only" issue. It might be a "only a couple of architectures have cared enough for it to have any practical impact". End result: if some other architecture still has a __get_user() that is noticeably faster than get_user(), it's not an argument for keeping __get_user() - it's an argument that that architecture likely isn't very important. Linus ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net-next] vhost: use "checked" versions of get_user() and put_user() 2025-11-14 20:48 ` Linus Torvalds @ 2025-11-14 21:38 ` David Laight 0 siblings, 0 replies; 27+ messages in thread From: David Laight @ 2025-11-14 21:38 UTC (permalink / raw) To: Linus Torvalds Cc: Jon Kohler, Jason Wang, Michael S. Tsirkin, Eugenio Pérez, kvm@vger.kernel.org, virtualization@lists.linux.dev, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Borislav Petkov, Sean Christopherson On Fri, 14 Nov 2025 12:48:52 -0800 Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Fri, 14 Nov 2025 at 11:09, David Laight <david.laight.linux@gmail.com> wrote: > > > > I think that is currently only x86-64? > > There are patches in the pipeline for ppc. > > I don't think I've seen anything for arm32 or arm64. > > Honestly, the fact that it's mainly true on x86-64 is simply because > that's the only architecture that has cared enough. > > Pretty much everybody else is affected by the exact same speculation > bugs. Sometimes the speculation window might be so small that it > doesn't matter, but in most cases it's just that the architecture is > so irrelevant that it doesn't matter. > > So no, this is not a "x86 only" issue. It might be a "only a couple of > architectures have cared enough for it to have any practical impact". > > End result: if some other architecture still has a __get_user() that > is noticeably faster than get_user(), it's not an argument for keeping > __get_user() - it's an argument that that architecture likely isn't > very important. I was really thinking it was a justification to get the 'address masking' implemented for other architectures. It wouldn't surprise me if some of the justifications for the 'guard page' at the top of x86-64 userspace (like speculative execution across the user-kernel boundary) aren't a more general problem. So adding support to arm32, arm64, riscV and 32bit x86 might be reasonable. What does that really leave? sparc, m68k? At that point requiring a guard page for all architectures starts looking reasonable, and the non 'address masking' user access checks can all be thrown away. That isn't going to happen quickly, but seems a reasonable aim. Architectures without speculation issues (old ones) can use a C compare. I think this works for 32bit x86 (without cmov): mov $-guard_page, %guard_off add %user_addr, %guard_off sbb %mask, %mask and %mask, %guard_off sub %guard_off, %user_addr mips-like architectures (no flags) probably require a 'cmp' and 'dec' to generate the mask value. (I'm not sure how that compares to any of the ppc asm blocks.) David > > Linus ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net-next] vhost: use "checked" versions of get_user() and put_user() 2025-11-14 14:53 ` Jon Kohler 2025-11-14 17:48 ` Linus Torvalds @ 2025-11-17 4:32 ` Jason Wang 2025-11-17 17:34 ` Jon Kohler 1 sibling, 1 reply; 27+ messages in thread From: Jason Wang @ 2025-11-17 4:32 UTC (permalink / raw) To: Jon Kohler Cc: Michael S. Tsirkin, Eugenio Pérez, kvm@vger.kernel.org, virtualization@lists.linux.dev, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Linus Torvalds, Borislav Petkov, Sean Christopherson On Fri, Nov 14, 2025 at 10:53 PM Jon Kohler <jon@nutanix.com> wrote: > > > > > On Nov 12, 2025, at 8:09 PM, Jason Wang <jasowang@redhat.com> wrote: > > > > !-------------------------------------------------------------------| > > CAUTION: External Email > > > > |-------------------------------------------------------------------! > > > > On Thu, Nov 13, 2025 at 8:14 AM Jon Kohler <jon@nutanix.com> wrote: > >> > >> vhost_get_user and vhost_put_user leverage __get_user and __put_user, > >> respectively, which were both added in 2016 by commit 6b1e6cc7855b > >> ("vhost: new device IOTLB API"). > > > > It has been used even before this commit. > > Ah, thanks for the pointer. I’d have to go dig to find its genesis, but > its more to say, this existed prior to the LFENCE commit. > > > > >> In a heavy UDP transmit workload on a > >> vhost-net backed tap device, these functions showed up as ~11.6% of > >> samples in a flamegraph of the underlying vhost worker thread. > >> > >> Quoting Linus from [1]: > >> Anyway, every single __get_user() call I looked at looked like > >> historical garbage. [...] End result: I get the feeling that we > >> should just do a global search-and-replace of the __get_user/ > >> __put_user users, replace them with plain get_user/put_user instead, > >> and then fix up any fallout (eg the coco code). > >> > >> Switch to plain get_user/put_user in vhost, which results in a slight > >> throughput speedup. get_user now about ~8.4% of samples in flamegraph. > >> > >> Basic iperf3 test on a Intel 5416S CPU with Ubuntu 25.10 guest: > >> TX: taskset -c 2 iperf3 -c <rx_ip> -t 60 -p 5200 -b 0 -u -i 5 > >> RX: taskset -c 2 iperf3 -s -p 5200 -D > >> Before: 6.08 Gbits/sec > >> After: 6.32 Gbits/sec > > > > I wonder if we need to test on archs like ARM. > > Are you thinking from a performance perspective? Or a correctness one? Performance, I think the patch is correct. Thanks ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net-next] vhost: use "checked" versions of get_user() and put_user() 2025-11-17 4:32 ` Jason Wang @ 2025-11-17 17:34 ` Jon Kohler 2025-11-20 1:57 ` Jason Wang 0 siblings, 1 reply; 27+ messages in thread From: Jon Kohler @ 2025-11-17 17:34 UTC (permalink / raw) To: Jason Wang Cc: Michael S. Tsirkin, Eugenio Pérez, kvm@vger.kernel.org, virtualization@lists.linux.dev, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Linus Torvalds, Borislav Petkov, Sean Christopherson > On Nov 16, 2025, at 11:32 PM, Jason Wang <jasowang@redhat.com> wrote: > > On Fri, Nov 14, 2025 at 10:53 PM Jon Kohler <jon@nutanix.com> wrote: >> >> >> >>> On Nov 12, 2025, at 8:09 PM, Jason Wang <jasowang@redhat.com> wrote: >>> >>> !-------------------------------------------------------------------| >>> CAUTION: External Email >>> >>> |-------------------------------------------------------------------! >>> >>> On Thu, Nov 13, 2025 at 8:14 AM Jon Kohler <jon@nutanix.com> wrote: >>>> >>>> vhost_get_user and vhost_put_user leverage __get_user and __put_user, >>>> respectively, which were both added in 2016 by commit 6b1e6cc7855b >>>> ("vhost: new device IOTLB API"). >>> >>> It has been used even before this commit. >> >> Ah, thanks for the pointer. I’d have to go dig to find its genesis, but >> its more to say, this existed prior to the LFENCE commit. >> >>> >>>> In a heavy UDP transmit workload on a >>>> vhost-net backed tap device, these functions showed up as ~11.6% of >>>> samples in a flamegraph of the underlying vhost worker thread. >>>> >>>> Quoting Linus from [1]: >>>> Anyway, every single __get_user() call I looked at looked like >>>> historical garbage. [...] End result: I get the feeling that we >>>> should just do a global search-and-replace of the __get_user/ >>>> __put_user users, replace them with plain get_user/put_user instead, >>>> and then fix up any fallout (eg the coco code). >>>> >>>> Switch to plain get_user/put_user in vhost, which results in a slight >>>> throughput speedup. get_user now about ~8.4% of samples in flamegraph. >>>> >>>> Basic iperf3 test on a Intel 5416S CPU with Ubuntu 25.10 guest: >>>> TX: taskset -c 2 iperf3 -c <rx_ip> -t 60 -p 5200 -b 0 -u -i 5 >>>> RX: taskset -c 2 iperf3 -s -p 5200 -D >>>> Before: 6.08 Gbits/sec >>>> After: 6.32 Gbits/sec >>> >>> I wonder if we need to test on archs like ARM. >> >> Are you thinking from a performance perspective? Or a correctness one? > > Performance, I think the patch is correct. > > Thanks > Ok gotcha. If anyone has an ARM system stuffed in their front pocket and can give this a poke, I’d appreciate it, as I don’t have ready access to one personally. That said, I think this might end up in “well, it is what it is” territory as Linus was alluding to, i.e. if performance dips on ARM for vhost, then thats a compelling point to optimize whatever ends up being the culprit for get/put user? Said another way, would ARM perf testing (or any other arch) be a blocker to taking this change? Thanks - Jon ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net-next] vhost: use "checked" versions of get_user() and put_user() 2025-11-17 17:34 ` Jon Kohler @ 2025-11-20 1:57 ` Jason Wang 2025-11-25 19:45 ` Jon Kohler 0 siblings, 1 reply; 27+ messages in thread From: Jason Wang @ 2025-11-20 1:57 UTC (permalink / raw) To: Jon Kohler Cc: Michael S. Tsirkin, Eugenio Pérez, kvm@vger.kernel.org, virtualization@lists.linux.dev, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Linus Torvalds, Borislav Petkov, Sean Christopherson On Tue, Nov 18, 2025 at 1:35 AM Jon Kohler <jon@nutanix.com> wrote: > > > > > On Nov 16, 2025, at 11:32 PM, Jason Wang <jasowang@redhat.com> wrote: > > > > On Fri, Nov 14, 2025 at 10:53 PM Jon Kohler <jon@nutanix.com> wrote: > >> > >> > >> > >>> On Nov 12, 2025, at 8:09 PM, Jason Wang <jasowang@redhat.com> wrote: > >>> > >>> !-------------------------------------------------------------------| > >>> CAUTION: External Email > >>> > >>> |-------------------------------------------------------------------! > >>> > >>> On Thu, Nov 13, 2025 at 8:14 AM Jon Kohler <jon@nutanix.com> wrote: > >>>> > >>>> vhost_get_user and vhost_put_user leverage __get_user and __put_user, > >>>> respectively, which were both added in 2016 by commit 6b1e6cc7855b > >>>> ("vhost: new device IOTLB API"). > >>> > >>> It has been used even before this commit. > >> > >> Ah, thanks for the pointer. I’d have to go dig to find its genesis, but > >> its more to say, this existed prior to the LFENCE commit. > >> > >>> > >>>> In a heavy UDP transmit workload on a > >>>> vhost-net backed tap device, these functions showed up as ~11.6% of > >>>> samples in a flamegraph of the underlying vhost worker thread. > >>>> > >>>> Quoting Linus from [1]: > >>>> Anyway, every single __get_user() call I looked at looked like > >>>> historical garbage. [...] End result: I get the feeling that we > >>>> should just do a global search-and-replace of the __get_user/ > >>>> __put_user users, replace them with plain get_user/put_user instead, > >>>> and then fix up any fallout (eg the coco code). > >>>> > >>>> Switch to plain get_user/put_user in vhost, which results in a slight > >>>> throughput speedup. get_user now about ~8.4% of samples in flamegraph. > >>>> > >>>> Basic iperf3 test on a Intel 5416S CPU with Ubuntu 25.10 guest: > >>>> TX: taskset -c 2 iperf3 -c <rx_ip> -t 60 -p 5200 -b 0 -u -i 5 > >>>> RX: taskset -c 2 iperf3 -s -p 5200 -D > >>>> Before: 6.08 Gbits/sec > >>>> After: 6.32 Gbits/sec > >>> > >>> I wonder if we need to test on archs like ARM. > >> > >> Are you thinking from a performance perspective? Or a correctness one? > > > > Performance, I think the patch is correct. > > > > Thanks > > > > Ok gotcha. If anyone has an ARM system stuffed in their > front pocket and can give this a poke, I’d appreciate it, as > I don’t have ready access to one personally. > > That said, I think this might end up in “well, it is what it is” > territory as Linus was alluding to, i.e. if performance dips on > ARM for vhost, then thats a compelling point to optimize whatever > ends up being the culprit for get/put user? > > Said another way, would ARM perf testing (or any other arch) be a > blocker to taking this change? Not a must but at least we need to explain the implication for other archs as the discussion you quoted are all for x86. Thanks > > Thanks - Jon > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net-next] vhost: use "checked" versions of get_user() and put_user() 2025-11-20 1:57 ` Jason Wang @ 2025-11-25 19:45 ` Jon Kohler 2025-11-26 6:04 ` Jason Wang 0 siblings, 1 reply; 27+ messages in thread From: Jon Kohler @ 2025-11-25 19:45 UTC (permalink / raw) To: Jason Wang Cc: Michael S. Tsirkin, Eugenio Pérez, kvm@vger.kernel.org, virtualization@lists.linux.dev, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Linus Torvalds, Borislav Petkov, Sean Christopherson > On Nov 19, 2025, at 8:57 PM, Jason Wang <jasowang@redhat.com> wrote: > > On Tue, Nov 18, 2025 at 1:35 AM Jon Kohler <jon@nutanix.com> wrote: >> >> >>> On Nov 16, 2025, at 11:32 PM, Jason Wang <jasowang@redhat.com> wrote: >>> >>> On Fri, Nov 14, 2025 at 10:53 PM Jon Kohler <jon@nutanix.com> wrote: >>>> >>>> >>>>> On Nov 12, 2025, at 8:09 PM, Jason Wang <jasowang@redhat.com> wrote: >>>>> >>>>> On Thu, Nov 13, 2025 at 8:14 AM Jon Kohler <jon@nutanix.com> wrote: >>>>>> >>>>>> vhost_get_user and vhost_put_user leverage __get_user and __put_user, >>>>>> respectively, which were both added in 2016 by commit 6b1e6cc7855b >>>>>> ("vhost: new device IOTLB API"). >>>>> >>>>> It has been used even before this commit. >>>> >>>> Ah, thanks for the pointer. I’d have to go dig to find its genesis, but >>>> its more to say, this existed prior to the LFENCE commit. >>>> >>>>> >>>>>> In a heavy UDP transmit workload on a >>>>>> vhost-net backed tap device, these functions showed up as ~11.6% of >>>>>> samples in a flamegraph of the underlying vhost worker thread. >>>>>> >>>>>> Quoting Linus from [1]: >>>>>> Anyway, every single __get_user() call I looked at looked like >>>>>> historical garbage. [...] End result: I get the feeling that we >>>>>> should just do a global search-and-replace of the __get_user/ >>>>>> __put_user users, replace them with plain get_user/put_user instead, >>>>>> and then fix up any fallout (eg the coco code). >>>>>> >>>>>> Switch to plain get_user/put_user in vhost, which results in a slight >>>>>> throughput speedup. get_user now about ~8.4% of samples in flamegraph. >>>>>> >>>>>> Basic iperf3 test on a Intel 5416S CPU with Ubuntu 25.10 guest: >>>>>> TX: taskset -c 2 iperf3 -c <rx_ip> -t 60 -p 5200 -b 0 -u -i 5 >>>>>> RX: taskset -c 2 iperf3 -s -p 5200 -D >>>>>> Before: 6.08 Gbits/sec >>>>>> After: 6.32 Gbits/sec >>>>> >>>>> I wonder if we need to test on archs like ARM. >>>> >>>> Are you thinking from a performance perspective? Or a correctness one? >>> >>> Performance, I think the patch is correct. >>> >>> Thanks >>> >> >> Ok gotcha. If anyone has an ARM system stuffed in their >> front pocket and can give this a poke, I’d appreciate it, as >> I don’t have ready access to one personally. >> >> That said, I think this might end up in “well, it is what it is” >> territory as Linus was alluding to, i.e. if performance dips on >> ARM for vhost, then thats a compelling point to optimize whatever >> ends up being the culprit for get/put user? >> >> Said another way, would ARM perf testing (or any other arch) be a >> blocker to taking this change? > > Not a must but at least we need to explain the implication for other > archs as the discussion you quoted are all for x86. > > Thanks I’ll admit my ARM muscle is weak, but here’s my best take on this: Looking at arch/arm/include/asm/uaccess.h, the biggest thing that I noticed is the CONFIG_CPU_SPECTRE ifdef, which already remaps __get_user() to get_user(), so anyone running that in their kconfig will already practically have the behavior implemented by this patch by way of commit b1cd0a148063 ("ARM: spectre-v1: use get_user() for __get_user()”). Same deal goes for __put_user() vs put_user by way of commit e3aa6243434f ("ARM: 8795/1: spectre-v1.1: use put_user() for __put_user()”) Looking at arch/arm/mm/Kconfig, there are a variety of scenarios where CONFIG_CPU_SPECTRE will be enabled automagically. Looking at commit 252309adc81f ("ARM: Make CONFIG_CPU_V7 valid for 32bit ARMv8 implementations") it says that "ARMv8 is a superset of ARMv7", so I’d guess that just about everything ARM would include this by default? If so, that mean at least for a non-zero population of ARM’ers, they wouldn’t notice anything from this patch, yea? Happy to learn otherwise if that read is incorrect! Thanks all, Jon ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net-next] vhost: use "checked" versions of get_user() and put_user() 2025-11-25 19:45 ` Jon Kohler @ 2025-11-26 6:04 ` Jason Wang 2025-11-26 10:25 ` Arnd Bergmann 0 siblings, 1 reply; 27+ messages in thread From: Jason Wang @ 2025-11-26 6:04 UTC (permalink / raw) To: Jon Kohler Cc: Michael S. Tsirkin, Eugenio Pérez, kvm@vger.kernel.org, virtualization@lists.linux.dev, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Linus Torvalds, Borislav Petkov, Sean Christopherson, linux-arm-kernel, Russell King - ARM Linux, Catalin Marinas, Will Deacon, Arnd Bergmann, krzk, alexandre.belloni, Linus Walleij, fustini On Wed, Nov 26, 2025 at 3:45 AM Jon Kohler <jon@nutanix.com> wrote: > > > > > On Nov 19, 2025, at 8:57 PM, Jason Wang <jasowang@redhat.com> wrote: > > > > On Tue, Nov 18, 2025 at 1:35 AM Jon Kohler <jon@nutanix.com> wrote: > >> > >> > >>> On Nov 16, 2025, at 11:32 PM, Jason Wang <jasowang@redhat.com> wrote: > >>> > >>> On Fri, Nov 14, 2025 at 10:53 PM Jon Kohler <jon@nutanix.com> wrote: > >>>> > >>>> > >>>>> On Nov 12, 2025, at 8:09 PM, Jason Wang <jasowang@redhat.com> wrote: > >>>>> > >>>>> On Thu, Nov 13, 2025 at 8:14 AM Jon Kohler <jon@nutanix.com> wrote: > >>>>>> > >>>>>> vhost_get_user and vhost_put_user leverage __get_user and __put_user, > >>>>>> respectively, which were both added in 2016 by commit 6b1e6cc7855b > >>>>>> ("vhost: new device IOTLB API"). > >>>>> > >>>>> It has been used even before this commit. > >>>> > >>>> Ah, thanks for the pointer. I’d have to go dig to find its genesis, but > >>>> its more to say, this existed prior to the LFENCE commit. > >>>> > >>>>> > >>>>>> In a heavy UDP transmit workload on a > >>>>>> vhost-net backed tap device, these functions showed up as ~11.6% of > >>>>>> samples in a flamegraph of the underlying vhost worker thread. > >>>>>> > >>>>>> Quoting Linus from [1]: > >>>>>> Anyway, every single __get_user() call I looked at looked like > >>>>>> historical garbage. [...] End result: I get the feeling that we > >>>>>> should just do a global search-and-replace of the __get_user/ > >>>>>> __put_user users, replace them with plain get_user/put_user instead, > >>>>>> and then fix up any fallout (eg the coco code). > >>>>>> > >>>>>> Switch to plain get_user/put_user in vhost, which results in a slight > >>>>>> throughput speedup. get_user now about ~8.4% of samples in flamegraph. > >>>>>> > >>>>>> Basic iperf3 test on a Intel 5416S CPU with Ubuntu 25.10 guest: > >>>>>> TX: taskset -c 2 iperf3 -c <rx_ip> -t 60 -p 5200 -b 0 -u -i 5 > >>>>>> RX: taskset -c 2 iperf3 -s -p 5200 -D > >>>>>> Before: 6.08 Gbits/sec > >>>>>> After: 6.32 Gbits/sec > >>>>> > >>>>> I wonder if we need to test on archs like ARM. > >>>> > >>>> Are you thinking from a performance perspective? Or a correctness one? > >>> > >>> Performance, I think the patch is correct. > >>> > >>> Thanks > >>> > >> > >> Ok gotcha. If anyone has an ARM system stuffed in their > >> front pocket and can give this a poke, I’d appreciate it, as > >> I don’t have ready access to one personally. > >> > >> That said, I think this might end up in “well, it is what it is” > >> territory as Linus was alluding to, i.e. if performance dips on > >> ARM for vhost, then thats a compelling point to optimize whatever > >> ends up being the culprit for get/put user? > >> > >> Said another way, would ARM perf testing (or any other arch) be a > >> blocker to taking this change? > > > > Not a must but at least we need to explain the implication for other > > archs as the discussion you quoted are all for x86. > > > > Thanks > > I’ll admit my ARM muscle is weak, but here’s my best take on this: > > Looking at arch/arm/include/asm/uaccess.h, the biggest thing that I > noticed is the CONFIG_CPU_SPECTRE ifdef, which already remaps > __get_user() to get_user(), so anyone running that in their kconfig > will already practically have the behavior implemented by this patch > by way of commit b1cd0a148063 ("ARM: spectre-v1: use get_user() for > __get_user()”). > > Same deal goes for __put_user() vs put_user by way of commit > e3aa6243434f ("ARM: 8795/1: spectre-v1.1: use put_user() for __put_user()”) > > Looking at arch/arm/mm/Kconfig, there are a variety of scenarios > where CONFIG_CPU_SPECTRE will be enabled automagically. Looking at > commit 252309adc81f ("ARM: Make CONFIG_CPU_V7 valid for 32bit ARMv8 implementations") > it says that "ARMv8 is a superset of ARMv7", so I’d guess that just > about everything ARM would include this by default? > > If so, that mean at least for a non-zero population of ARM’ers, > they wouldn’t notice anything from this patch, yea? Adding ARM maintainers for more thought. Thanks > > Happy to learn otherwise if that read is incorrect! > > Thanks all, > Jon ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net-next] vhost: use "checked" versions of get_user() and put_user() 2025-11-26 6:04 ` Jason Wang @ 2025-11-26 10:25 ` Arnd Bergmann 2025-11-26 19:47 ` Jon Kohler 0 siblings, 1 reply; 27+ messages in thread From: Arnd Bergmann @ 2025-11-26 10:25 UTC (permalink / raw) To: Jason Wang, Jon Kohler Cc: Michael S. Tsirkin, Eugenio Pérez, kvm@vger.kernel.org, virtualization@lists.linux.dev, Netdev, linux-kernel@vger.kernel.org, Linus Torvalds, Borislav Petkov, Sean Christopherson, linux-arm-kernel, Russell King, Catalin Marinas, Will Deacon, Krzysztof Kozlowski, Alexandre Belloni, Linus Walleij, Drew Fustini On Wed, Nov 26, 2025, at 07:04, Jason Wang wrote: > On Wed, Nov 26, 2025 at 3:45 AM Jon Kohler <jon@nutanix.com> wrote: >> > On Nov 19, 2025, at 8:57 PM, Jason Wang <jasowang@redhat.com> wrote: >> > On Tue, Nov 18, 2025 at 1:35 AM Jon Kohler <jon@nutanix.com> wrote: >> Same deal goes for __put_user() vs put_user by way of commit >> e3aa6243434f ("ARM: 8795/1: spectre-v1.1: use put_user() for __put_user()”) >> >> Looking at arch/arm/mm/Kconfig, there are a variety of scenarios >> where CONFIG_CPU_SPECTRE will be enabled automagically. Looking at >> commit 252309adc81f ("ARM: Make CONFIG_CPU_V7 valid for 32bit ARMv8 implementations") >> it says that "ARMv8 is a superset of ARMv7", so I’d guess that just >> about everything ARM would include this by default? I think the more relevant commit is for 64-bit Arm here, but this does the same thing, see 84624087dd7e ("arm64: uaccess: Don't bother eliding access_ok checks in __{get, put}_user"). Note that there is no KVM on 32-bit Arm any more, so we really don't care about vhost performance there. The added access_ok() check in arm32 __get_user() is probably avoidable, as embedded systems with in-order cores could turn off the spectre workarounds, but as Will explained in the arm64 commit, it's not that expensive either. >> If so, that mean at least for a non-zero population of ARM’ers, >> they wouldn’t notice anything from this patch, yea? > > Adding ARM maintainers for more thought. I would think that if we change the __get_user() to get_user() in this driver, the same should be done for the __copy_{from,to}_user(), which similarly skips the access_ok() check but not the PAN/SMAP handling. In general, the access_ok()/__get_user()/__copy_from_user() pattern isn't really helpful any more, as Linus already explained. I can't tell from the vhost driver code whether we can just drop the access_ok() here and use the plain get_user()/copy_from_user(), or if it makes sense to move to the newer user_access_begin()/unsafe_get_user()/ unsafe_copy_from_user()/user_access_end() and try optimize out a few PAN/SMAP flips in the process. Arnd ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net-next] vhost: use "checked" versions of get_user() and put_user() 2025-11-26 10:25 ` Arnd Bergmann @ 2025-11-26 19:47 ` Jon Kohler 2025-11-26 19:58 ` Arnd Bergmann 2025-11-27 1:08 ` Jason Wang 0 siblings, 2 replies; 27+ messages in thread From: Jon Kohler @ 2025-11-26 19:47 UTC (permalink / raw) To: Arnd Bergmann Cc: Jason Wang, Michael S. Tsirkin, Eugenio Pérez, kvm@vger.kernel.org, virtualization@lists.linux.dev, Netdev, linux-kernel@vger.kernel.org, Linus Torvalds, Borislav Petkov, Sean Christopherson, linux-arm-kernel@lists.infradead.org, Russell King, Catalin Marinas, Will Deacon, Krzysztof Kozlowski, Alexandre Belloni, Linus Walleij, Drew Fustini > On Nov 26, 2025, at 5:25 AM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Wed, Nov 26, 2025, at 07:04, Jason Wang wrote: >> On Wed, Nov 26, 2025 at 3:45 AM Jon Kohler <jon@nutanix.com> wrote: >>>> On Nov 19, 2025, at 8:57 PM, Jason Wang <jasowang@redhat.com> wrote: >>>> On Tue, Nov 18, 2025 at 1:35 AM Jon Kohler <jon@nutanix.com> wrote: >>> Same deal goes for __put_user() vs put_user by way of commit >>> e3aa6243434f ("ARM: 8795/1: spectre-v1.1: use put_user() for __put_user()”) >>> >>> Looking at arch/arm/mm/Kconfig, there are a variety of scenarios >>> where CONFIG_CPU_SPECTRE will be enabled automagically. Looking at >>> commit 252309adc81f ("ARM: Make CONFIG_CPU_V7 valid for 32bit ARMv8 implementations") >>> it says that "ARMv8 is a superset of ARMv7", so I’d guess that just >>> about everything ARM would include this by default? > > I think the more relevant commit is for 64-bit Arm here, but this does > the same thing, see 84624087dd7e ("arm64: uaccess: Don't bother > eliding access_ok checks in __{get, put}_user"). Ah! Right, this is definitely the important bit, as it makes it crystal clear that these are exactly the same thing. The current code is: #define get_user __get_user #define put_user __put_user So, this patch changing from __* to regular versions is a no-op on arm side of the house, yea? > I would think that if we change the __get_user() to get_user() > in this driver, the same should be done for the > __copy_{from,to}_user(), which similarly skips the access_ok() > check but not the PAN/SMAP handling. Perhaps, thats a good call out. I’d file that under one battle at a time. Let’s get get/put user dusted first, then go down that road? > In general, the access_ok()/__get_user()/__copy_from_user() > pattern isn't really helpful any more, as Linus already > explained. I can't tell from the vhost driver code whether > we can just drop the access_ok() here and use the plain > get_user()/copy_from_user(), or if it makes sense to move > to the newer user_access_begin()/unsafe_get_user()/ > unsafe_copy_from_user()/user_access_end() and try optimize > out a few PAN/SMAP flips in the process. In general, I think there are a few spots where we might be able to optimize (vhost_get_vq_desc perhaps?) as that gets called quite a bit and IIRC there are at least two flips in there that perhaps we could elide to one? An investigation for another day I think. Anyhow, with this info - Jason - is there anything else you can think of that we want to double click on? Jon ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net-next] vhost: use "checked" versions of get_user() and put_user() 2025-11-26 19:47 ` Jon Kohler @ 2025-11-26 19:58 ` Arnd Bergmann 2025-11-26 21:42 ` Jon Kohler 2025-11-27 1:08 ` Jason Wang 1 sibling, 1 reply; 27+ messages in thread From: Arnd Bergmann @ 2025-11-26 19:58 UTC (permalink / raw) To: Jon Kohler Cc: Jason Wang, Michael S. Tsirkin, Eugenio Pérez, kvm@vger.kernel.org, virtualization@lists.linux.dev, Netdev, linux-kernel@vger.kernel.org, Linus Torvalds, Borislav Petkov, Sean Christopherson, linux-arm-kernel@lists.infradead.org, Russell King, Catalin Marinas, Will Deacon, Krzysztof Kozlowski, Alexandre Belloni, Linus Walleij, Drew Fustini On Wed, Nov 26, 2025, at 20:47, Jon Kohler wrote: >> On Nov 26, 2025, at 5:25 AM, Arnd Bergmann <arnd@arndb.de> wrote: >> On Wed, Nov 26, 2025, at 07:04, Jason Wang wrote: >>> On Wed, Nov 26, 2025 at 3:45 AM Jon Kohler <jon@nutanix.com> wrote: >> I think the more relevant commit is for 64-bit Arm here, but this does >> the same thing, see 84624087dd7e ("arm64: uaccess: Don't bother >> eliding access_ok checks in __{get, put}_user"). > > Ah! Right, this is definitely the important bit, as it makes it > crystal clear that these are exactly the same thing. The current > code is: > #define get_user __get_user > #define put_user __put_user > > So, this patch changing from __* to regular versions is a no-op > on arm side of the house, yea? Certainly on 64-bit, and almost always on 32-bit, yes. >> I would think that if we change the __get_user() to get_user() >> in this driver, the same should be done for the >> __copy_{from,to}_user(), which similarly skips the access_ok() >> check but not the PAN/SMAP handling. > > Perhaps, thats a good call out. I’d file that under one battle > at a time. Let’s get get/put user dusted first, then go down > that road? It depends on what your bigger plan is. Are you working on improving the vhost driver specifically, or are you trying to kill off the __get_user/__put_user calls across the entire kernel? In the latter case, I would suggest you do one driver at a time but address access_ok(), __{get,put}_user and __copy_{from,to}_user() with a single patch per driver as long as this is simple enough. For vhost specifically, doing it piecemeal is probably fine since the interaction is more complicated than most others. >> In general, the access_ok()/__get_user()/__copy_from_user() >> pattern isn't really helpful any more, as Linus already >> explained. I can't tell from the vhost driver code whether >> we can just drop the access_ok() here and use the plain >> get_user()/copy_from_user(), or if it makes sense to move >> to the newer user_access_begin()/unsafe_get_user()/ >> unsafe_copy_from_user()/user_access_end() and try optimize >> out a few PAN/SMAP flips in the process. > > In general, I think there are a few spots where we might be > able to optimize (vhost_get_vq_desc perhaps?) as that gets > called quite a bit and IIRC there are at least two flips > in there that perhaps we could elide to one? An investigation > for another day I think. Yes, sounds good. Arnd ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net-next] vhost: use "checked" versions of get_user() and put_user() 2025-11-26 19:58 ` Arnd Bergmann @ 2025-11-26 21:42 ` Jon Kohler 2025-11-26 21:45 ` Linus Torvalds 0 siblings, 1 reply; 27+ messages in thread From: Jon Kohler @ 2025-11-26 21:42 UTC (permalink / raw) To: Arnd Bergmann Cc: Jason Wang, Michael S. Tsirkin, Eugenio Pérez, kvm@vger.kernel.org, virtualization@lists.linux.dev, Netdev, linux-kernel@vger.kernel.org, Linus Torvalds, Borislav Petkov, Sean Christopherson, linux-arm-kernel@lists.infradead.org, Russell King, Catalin Marinas, Will Deacon, Krzysztof Kozlowski, Alexandre Belloni, Linus Walleij, Drew Fustini > On Nov 26, 2025, at 2:58 PM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Wed, Nov 26, 2025, at 20:47, Jon Kohler wrote: >>> On Nov 26, 2025, at 5:25 AM, Arnd Bergmann <arnd@arndb.de> wrote: >>> On Wed, Nov 26, 2025, at 07:04, Jason Wang wrote: >>>> On Wed, Nov 26, 2025 at 3:45 AM Jon Kohler <jon@nutanix.com> wrote: >>> I think the more relevant commit is for 64-bit Arm here, but this does >>> the same thing, see 84624087dd7e ("arm64: uaccess: Don't bother >>> eliding access_ok checks in __{get, put}_user"). >> >> Ah! Right, this is definitely the important bit, as it makes it >> crystal clear that these are exactly the same thing. The current >> code is: >> #define get_user __get_user >> #define put_user __put_user >> >> So, this patch changing from __* to regular versions is a no-op >> on arm side of the house, yea? > > Certainly on 64-bit, and almost always on 32-bit, yes. > >>> I would think that if we change the __get_user() to get_user() >>> in this driver, the same should be done for the >>> __copy_{from,to}_user(), which similarly skips the access_ok() >>> check but not the PAN/SMAP handling. >> >> Perhaps, thats a good call out. I’d file that under one battle >> at a time. Let’s get get/put user dusted first, then go down >> that road? > > It depends on what your bigger plan is. Are you working on > improving the vhost driver specifically, or are you trying > to kill off the __get_user/__put_user calls across the > entire kernel? I’m working on vhost / virtualized networking improvements at the moment, not the broader kernel wide work. Linus mentioned he might get into the mix and do a bulk change and kill the whole thing once and for all, so I’m simply trying to help knock an incremental amount of work off the pile in advance of that (and reap some performance benefits at the same time, at least on the x86 side). Thanks for the info and back n forth here, I appreciate it! ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net-next] vhost: use "checked" versions of get_user() and put_user() 2025-11-26 21:42 ` Jon Kohler @ 2025-11-26 21:45 ` Linus Torvalds 2025-11-27 2:58 ` Jon Kohler 0 siblings, 1 reply; 27+ messages in thread From: Linus Torvalds @ 2025-11-26 21:45 UTC (permalink / raw) To: Jon Kohler Cc: Arnd Bergmann, Jason Wang, Michael S. Tsirkin, Eugenio Pérez, kvm@vger.kernel.org, virtualization@lists.linux.dev, Netdev, linux-kernel@vger.kernel.org, Borislav Petkov, Sean Christopherson, linux-arm-kernel@lists.infradead.org, Russell King, Catalin Marinas, Will Deacon, Krzysztof Kozlowski, Alexandre Belloni, Linus Walleij, Drew Fustini On Wed, 26 Nov 2025 at 13:43, Jon Kohler <jon@nutanix.com> wrote: > > Linus mentioned he might get into the mix and do a bulk > change and kill the whole thing once and for all, so I’m > simply trying to help knock an incremental amount of work > off the pile in advance of that (and reap some performance > benefits at the same time, at least on the x86 side). So I'm definitely going to do some bulk conversion at some point, but honestly, I'll be a lot happier if most users already self-converted before that, and I only end up doing a "convert unmaintained old code that nobody really cares about". Linus ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net-next] vhost: use "checked" versions of get_user() and put_user() 2025-11-26 21:45 ` Linus Torvalds @ 2025-11-27 2:58 ` Jon Kohler 0 siblings, 0 replies; 27+ messages in thread From: Jon Kohler @ 2025-11-27 2:58 UTC (permalink / raw) To: Linus Torvalds Cc: Arnd Bergmann, Jason Wang, Michael S. Tsirkin, Eugenio Pérez, kvm@vger.kernel.org, virtualization@lists.linux.dev, Netdev, linux-kernel@vger.kernel.org, Borislav Petkov, Sean Christopherson, linux-arm-kernel@lists.infradead.org, Russell King, Catalin Marinas, Will Deacon, Krzysztof Kozlowski, Alexandre Belloni, Linus Walleij, Drew Fustini > On Nov 26, 2025, at 4:45 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Wed, 26 Nov 2025 at 13:43, Jon Kohler <jon@nutanix.com> wrote: >> >> Linus mentioned he might get into the mix and do a bulk >> change and kill the whole thing once and for all, so I’m >> simply trying to help knock an incremental amount of work >> off the pile in advance of that (and reap some performance >> benefits at the same time, at least on the x86 side). > > So I'm definitely going to do some bulk conversion at some point, but > honestly, I'll be a lot happier if most users already self-converted > before that, and I only end up doing a "convert unmaintained old code > that nobody really cares about". > > Linus That is fair, thanks Linus. I’ll see if I can pick off a couple more to start thinning out the pile, and see what sort of trouble I can get into. Cheers - Jon ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net-next] vhost: use "checked" versions of get_user() and put_user() 2025-11-26 19:47 ` Jon Kohler 2025-11-26 19:58 ` Arnd Bergmann @ 2025-11-27 1:08 ` Jason Wang 2025-11-27 3:11 ` Jon Kohler 1 sibling, 1 reply; 27+ messages in thread From: Jason Wang @ 2025-11-27 1:08 UTC (permalink / raw) To: Jon Kohler Cc: Arnd Bergmann, Michael S. Tsirkin, Eugenio Pérez, kvm@vger.kernel.org, virtualization@lists.linux.dev, Netdev, linux-kernel@vger.kernel.org, Linus Torvalds, Borislav Petkov, Sean Christopherson, linux-arm-kernel@lists.infradead.org, Russell King, Catalin Marinas, Will Deacon, Krzysztof Kozlowski, Alexandre Belloni, Linus Walleij, Drew Fustini On Thu, Nov 27, 2025 at 3:48 AM Jon Kohler <jon@nutanix.com> wrote: > > > > > On Nov 26, 2025, at 5:25 AM, Arnd Bergmann <arnd@arndb.de> wrote: > > > > On Wed, Nov 26, 2025, at 07:04, Jason Wang wrote: > >> On Wed, Nov 26, 2025 at 3:45 AM Jon Kohler <jon@nutanix.com> wrote: > >>>> On Nov 19, 2025, at 8:57 PM, Jason Wang <jasowang@redhat.com> wrote: > >>>> On Tue, Nov 18, 2025 at 1:35 AM Jon Kohler <jon@nutanix.com> wrote: > >>> Same deal goes for __put_user() vs put_user by way of commit > >>> e3aa6243434f ("ARM: 8795/1: spectre-v1.1: use put_user() for __put_user()”) > >>> > >>> Looking at arch/arm/mm/Kconfig, there are a variety of scenarios > >>> where CONFIG_CPU_SPECTRE will be enabled automagically. Looking at > >>> commit 252309adc81f ("ARM: Make CONFIG_CPU_V7 valid for 32bit ARMv8 implementations") > >>> it says that "ARMv8 is a superset of ARMv7", so I’d guess that just > >>> about everything ARM would include this by default? > > > > I think the more relevant commit is for 64-bit Arm here, but this does > > the same thing, see 84624087dd7e ("arm64: uaccess: Don't bother > > eliding access_ok checks in __{get, put}_user"). > > Ah! Right, this is definitely the important bit, as it makes it > crystal clear that these are exactly the same thing. The current > code is: > #define get_user __get_user > #define put_user __put_user > > So, this patch changing from __* to regular versions is a no-op > on arm side of the house, yea? > > > I would think that if we change the __get_user() to get_user() > > in this driver, the same should be done for the > > __copy_{from,to}_user(), which similarly skips the access_ok() > > check but not the PAN/SMAP handling. > > Perhaps, thats a good call out. I’d file that under one battle > at a time. Let’s get get/put user dusted first, then go down > that road? > > > In general, the access_ok()/__get_user()/__copy_from_user() > > pattern isn't really helpful any more, as Linus already > > explained. I can't tell from the vhost driver code whether > > we can just drop the access_ok() here and use the plain > > get_user()/copy_from_user(), or if it makes sense to move > > to the newer user_access_begin()/unsafe_get_user()/ > > unsafe_copy_from_user()/user_access_end() and try optimize > > out a few PAN/SMAP flips in the process. Right, according to my testing in the past, PAN/SMAP is a killer for small packet performance (PPS). > > In general, I think there are a few spots where we might be > able to optimize (vhost_get_vq_desc perhaps?) as that gets > called quite a bit and IIRC there are at least two flips > in there that perhaps we could elide to one? An investigation > for another day I think. Did you mean trying to read descriptors in a batch, that would be better and with IN_ORDER it would be even faster as a single (at most two) copy_from_user() might work (without the need to use user_access_begin()/user_access_end(). > > Anyhow, with this info - Jason - is there anything else you > can think of that we want to double click on? Nope. Thanks > > Jon ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net-next] vhost: use "checked" versions of get_user() and put_user() 2025-11-27 1:08 ` Jason Wang @ 2025-11-27 3:11 ` Jon Kohler 2025-11-27 6:31 ` Michael S. Tsirkin 2025-11-27 6:32 ` Michael S. Tsirkin 0 siblings, 2 replies; 27+ messages in thread From: Jon Kohler @ 2025-11-27 3:11 UTC (permalink / raw) To: Jason Wang Cc: Arnd Bergmann, Michael S. Tsirkin, Eugenio Pérez, kvm@vger.kernel.org, virtualization@lists.linux.dev, Netdev, linux-kernel@vger.kernel.org, Linus Torvalds, Borislav Petkov, Sean Christopherson, linux-arm-kernel@lists.infradead.org, Russell King, Catalin Marinas, Will Deacon, Krzysztof Kozlowski, Alexandre Belloni, Linus Walleij, Drew Fustini > On Nov 26, 2025, at 8:08 PM, Jason Wang <jasowang@redhat.com> wrote: > > On Thu, Nov 27, 2025 at 3:48 AM Jon Kohler <jon@nutanix.com> wrote: >> >> >>> On Nov 26, 2025, at 5:25 AM, Arnd Bergmann <arnd@arndb.de> wrote: >>> >>> On Wed, Nov 26, 2025, at 07:04, Jason Wang wrote: >>>> On Wed, Nov 26, 2025 at 3:45 AM Jon Kohler <jon@nutanix.com> wrote: >>>>>> On Nov 19, 2025, at 8:57 PM, Jason Wang <jasowang@redhat.com> wrote: >>>>>> On Tue, Nov 18, 2025 at 1:35 AM Jon Kohler <jon@nutanix.com> wrote: >>>>> Same deal goes for __put_user() vs put_user by way of commit >>>>> e3aa6243434f ("ARM: 8795/1: spectre-v1.1: use put_user() for __put_user()”) >>>>> >>>>> Looking at arch/arm/mm/Kconfig, there are a variety of scenarios >>>>> where CONFIG_CPU_SPECTRE will be enabled automagically. Looking at >>>>> commit 252309adc81f ("ARM: Make CONFIG_CPU_V7 valid for 32bit ARMv8 implementations") >>>>> it says that "ARMv8 is a superset of ARMv7", so I’d guess that just >>>>> about everything ARM would include this by default? >>> >>> I think the more relevant commit is for 64-bit Arm here, but this does >>> the same thing, see 84624087dd7e ("arm64: uaccess: Don't bother >>> eliding access_ok checks in __{get, put}_user"). >> >> Ah! Right, this is definitely the important bit, as it makes it >> crystal clear that these are exactly the same thing. The current >> code is: >> #define get_user __get_user >> #define put_user __put_user >> >> So, this patch changing from __* to regular versions is a no-op >> on arm side of the house, yea? >> >>> I would think that if we change the __get_user() to get_user() >>> in this driver, the same should be done for the >>> __copy_{from,to}_user(), which similarly skips the access_ok() >>> check but not the PAN/SMAP handling. >> >> Perhaps, thats a good call out. I’d file that under one battle >> at a time. Let’s get get/put user dusted first, then go down >> that road? >> >>> In general, the access_ok()/__get_user()/__copy_from_user() >>> pattern isn't really helpful any more, as Linus already >>> explained. I can't tell from the vhost driver code whether >>> we can just drop the access_ok() here and use the plain >>> get_user()/copy_from_user(), or if it makes sense to move >>> to the newer user_access_begin()/unsafe_get_user()/ >>> unsafe_copy_from_user()/user_access_end() and try optimize >>> out a few PAN/SMAP flips in the process. > > Right, according to my testing in the past, PAN/SMAP is a killer for > small packet performance (PPS). For sure, every little bit helps along that path > >> >> In general, I think there are a few spots where we might be >> able to optimize (vhost_get_vq_desc perhaps?) as that gets >> called quite a bit and IIRC there are at least two flips >> in there that perhaps we could elide to one? An investigation >> for another day I think. > > Did you mean trying to read descriptors in a batch, that would be > better and with IN_ORDER it would be even faster as a single (at most > two) copy_from_user() might work (without the need to use > user_access_begin()/user_access_end(). Yep. I haven’t fully thought through it, just a drive-by idea from looking at code for the recent work I’ve been doing, just scratching my head thinking there *must* be something we can do better there. Basically on the get rx/tx bufs path as well as the vhost_add_used_and_signal_n path, I think we could cluster together some of the get/put users and copy to/from’s. Would take some massaging, but I think there is something there. >> >> Anyhow, with this info - Jason - is there anything else you >> can think of that we want to double click on? > > Nope. > > Thanks Ok thanks. Perhaps we can land this in next and let it soak in, though, knock on wood, I don’t think there will be fallout (famous last words!) ? ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net-next] vhost: use "checked" versions of get_user() and put_user() 2025-11-27 3:11 ` Jon Kohler @ 2025-11-27 6:31 ` Michael S. Tsirkin 2025-11-27 6:32 ` Michael S. Tsirkin 1 sibling, 0 replies; 27+ messages in thread From: Michael S. Tsirkin @ 2025-11-27 6:31 UTC (permalink / raw) To: Jon Kohler Cc: Jason Wang, Arnd Bergmann, Eugenio Pérez, kvm@vger.kernel.org, virtualization@lists.linux.dev, Netdev, linux-kernel@vger.kernel.org, Linus Torvalds, Borislav Petkov, Sean Christopherson, linux-arm-kernel@lists.infradead.org, Russell King, Catalin Marinas, Will Deacon, Krzysztof Kozlowski, Alexandre Belloni, Linus Walleij, Drew Fustini On Thu, Nov 27, 2025 at 03:11:57AM +0000, Jon Kohler wrote: > > > > On Nov 26, 2025, at 8:08 PM, Jason Wang <jasowang@redhat.com> wrote: > > > > On Thu, Nov 27, 2025 at 3:48 AM Jon Kohler <jon@nutanix.com> wrote: > >> > >> > >>> On Nov 26, 2025, at 5:25 AM, Arnd Bergmann <arnd@arndb.de> wrote: > >>> > >>> On Wed, Nov 26, 2025, at 07:04, Jason Wang wrote: > >>>> On Wed, Nov 26, 2025 at 3:45 AM Jon Kohler <jon@nutanix.com> wrote: > >>>>>> On Nov 19, 2025, at 8:57 PM, Jason Wang <jasowang@redhat.com> wrote: > >>>>>> On Tue, Nov 18, 2025 at 1:35 AM Jon Kohler <jon@nutanix.com> wrote: > >>>>> Same deal goes for __put_user() vs put_user by way of commit > >>>>> e3aa6243434f ("ARM: 8795/1: spectre-v1.1: use put_user() for __put_user()”) > >>>>> > >>>>> Looking at arch/arm/mm/Kconfig, there are a variety of scenarios > >>>>> where CONFIG_CPU_SPECTRE will be enabled automagically. Looking at > >>>>> commit 252309adc81f ("ARM: Make CONFIG_CPU_V7 valid for 32bit ARMv8 implementations") > >>>>> it says that "ARMv8 is a superset of ARMv7", so I’d guess that just > >>>>> about everything ARM would include this by default? > >>> > >>> I think the more relevant commit is for 64-bit Arm here, but this does > >>> the same thing, see 84624087dd7e ("arm64: uaccess: Don't bother > >>> eliding access_ok checks in __{get, put}_user"). > >> > >> Ah! Right, this is definitely the important bit, as it makes it > >> crystal clear that these are exactly the same thing. The current > >> code is: > >> #define get_user __get_user > >> #define put_user __put_user > >> > >> So, this patch changing from __* to regular versions is a no-op > >> on arm side of the house, yea? > >> > >>> I would think that if we change the __get_user() to get_user() > >>> in this driver, the same should be done for the > >>> __copy_{from,to}_user(), which similarly skips the access_ok() > >>> check but not the PAN/SMAP handling. > >> > >> Perhaps, thats a good call out. I’d file that under one battle > >> at a time. Let’s get get/put user dusted first, then go down > >> that road? > >> > >>> In general, the access_ok()/__get_user()/__copy_from_user() > >>> pattern isn't really helpful any more, as Linus already > >>> explained. I can't tell from the vhost driver code whether > >>> we can just drop the access_ok() here and use the plain > >>> get_user()/copy_from_user(), or if it makes sense to move > >>> to the newer user_access_begin()/unsafe_get_user()/ > >>> unsafe_copy_from_user()/user_access_end() and try optimize > >>> out a few PAN/SMAP flips in the process. > > > > Right, according to my testing in the past, PAN/SMAP is a killer for > > small packet performance (PPS). > > For sure, every little bit helps along that path > > > > >> > >> In general, I think there are a few spots where we might be > >> able to optimize (vhost_get_vq_desc perhaps?) as that gets > >> called quite a bit and IIRC there are at least two flips > >> in there that perhaps we could elide to one? An investigation > >> for another day I think. > > > > Did you mean trying to read descriptors in a batch, that would be > > better and with IN_ORDER it would be even faster as a single (at most > > two) copy_from_user() might work (without the need to use > > user_access_begin()/user_access_end(). > > Yep. I haven’t fully thought through it, just a drive-by idea > from looking at code for the recent work I’ve been doing, just > scratching my head thinking there *must* be something we can do > better there. > > Basically on the get rx/tx bufs path as well as the > vhost_add_used_and_signal_n path, I think we could cluster together > some of the get/put users and copy to/from’s. Would take some > massaging, but I think there is something there. > > >> > >> Anyhow, with this info - Jason - is there anything else you > >> can think of that we want to double click on? > > > > Nope. > > > > Thanks > > Ok thanks. Perhaps we can land this in next and let it soak in, > though, knock on wood, I don’t think there will be fallout > (famous last words!) ? Yea I'll put this in linux-next and we'll see what happens. -- MST ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net-next] vhost: use "checked" versions of get_user() and put_user() 2025-11-27 3:11 ` Jon Kohler 2025-11-27 6:31 ` Michael S. Tsirkin @ 2025-11-27 6:32 ` Michael S. Tsirkin 2025-12-02 16:54 ` Jon Kohler 1 sibling, 1 reply; 27+ messages in thread From: Michael S. Tsirkin @ 2025-11-27 6:32 UTC (permalink / raw) To: Jon Kohler Cc: Jason Wang, Arnd Bergmann, Eugenio Pérez, kvm@vger.kernel.org, virtualization@lists.linux.dev, Netdev, linux-kernel@vger.kernel.org, Linus Torvalds, Borislav Petkov, Sean Christopherson, linux-arm-kernel@lists.infradead.org, Russell King, Catalin Marinas, Will Deacon, Krzysztof Kozlowski, Alexandre Belloni, Linus Walleij, Drew Fustini On Thu, Nov 27, 2025 at 03:11:57AM +0000, Jon Kohler wrote: > > > > On Nov 26, 2025, at 8:08 PM, Jason Wang <jasowang@redhat.com> wrote: > > > > On Thu, Nov 27, 2025 at 3:48 AM Jon Kohler <jon@nutanix.com> wrote: > >> > >> > >>> On Nov 26, 2025, at 5:25 AM, Arnd Bergmann <arnd@arndb.de> wrote: > >>> > >>> On Wed, Nov 26, 2025, at 07:04, Jason Wang wrote: > >>>> On Wed, Nov 26, 2025 at 3:45 AM Jon Kohler <jon@nutanix.com> wrote: > >>>>>> On Nov 19, 2025, at 8:57 PM, Jason Wang <jasowang@redhat.com> wrote: > >>>>>> On Tue, Nov 18, 2025 at 1:35 AM Jon Kohler <jon@nutanix.com> wrote: > >>>>> Same deal goes for __put_user() vs put_user by way of commit > >>>>> e3aa6243434f ("ARM: 8795/1: spectre-v1.1: use put_user() for __put_user()”) > >>>>> > >>>>> Looking at arch/arm/mm/Kconfig, there are a variety of scenarios > >>>>> where CONFIG_CPU_SPECTRE will be enabled automagically. Looking at > >>>>> commit 252309adc81f ("ARM: Make CONFIG_CPU_V7 valid for 32bit ARMv8 implementations") > >>>>> it says that "ARMv8 is a superset of ARMv7", so I’d guess that just > >>>>> about everything ARM would include this by default? > >>> > >>> I think the more relevant commit is for 64-bit Arm here, but this does > >>> the same thing, see 84624087dd7e ("arm64: uaccess: Don't bother > >>> eliding access_ok checks in __{get, put}_user"). > >> > >> Ah! Right, this is definitely the important bit, as it makes it > >> crystal clear that these are exactly the same thing. The current > >> code is: > >> #define get_user __get_user > >> #define put_user __put_user > >> > >> So, this patch changing from __* to regular versions is a no-op > >> on arm side of the house, yea? > >> > >>> I would think that if we change the __get_user() to get_user() > >>> in this driver, the same should be done for the > >>> __copy_{from,to}_user(), which similarly skips the access_ok() > >>> check but not the PAN/SMAP handling. > >> > >> Perhaps, thats a good call out. I’d file that under one battle > >> at a time. Let’s get get/put user dusted first, then go down > >> that road? > >> > >>> In general, the access_ok()/__get_user()/__copy_from_user() > >>> pattern isn't really helpful any more, as Linus already > >>> explained. I can't tell from the vhost driver code whether > >>> we can just drop the access_ok() here and use the plain > >>> get_user()/copy_from_user(), or if it makes sense to move > >>> to the newer user_access_begin()/unsafe_get_user()/ > >>> unsafe_copy_from_user()/user_access_end() and try optimize > >>> out a few PAN/SMAP flips in the process. > > > > Right, according to my testing in the past, PAN/SMAP is a killer for > > small packet performance (PPS). > > For sure, every little bit helps along that path > > > > >> > >> In general, I think there are a few spots where we might be > >> able to optimize (vhost_get_vq_desc perhaps?) as that gets > >> called quite a bit and IIRC there are at least two flips > >> in there that perhaps we could elide to one? An investigation > >> for another day I think. > > > > Did you mean trying to read descriptors in a batch, that would be > > better and with IN_ORDER it would be even faster as a single (at most > > two) copy_from_user() might work (without the need to use > > user_access_begin()/user_access_end(). > > Yep. I haven’t fully thought through it, just a drive-by idea > from looking at code for the recent work I’ve been doing, just > scratching my head thinking there *must* be something we can do > better there. > > Basically on the get rx/tx bufs path as well as the > vhost_add_used_and_signal_n path, I think we could cluster together > some of the get/put users and copy to/from’s. Would take some > massaging, but I think there is something there. > > >> > >> Anyhow, with this info - Jason - is there anything else you > >> can think of that we want to double click on? > > > > Nope. > > > > Thanks > > Ok thanks. Perhaps we can land this in next and let it soak in, > though, knock on wood, I don’t think there will be fallout > (famous last words!) ? > To clairify, I think vhost tree is better to put this in next than net-next, both because it's purely core vhost and because unlike net-next vhost rebases so it is easy to just drop the patch if there are issues. I'll put it there. -- MST ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net-next] vhost: use "checked" versions of get_user() and put_user() 2025-11-27 6:32 ` Michael S. Tsirkin @ 2025-12-02 16:54 ` Jon Kohler 0 siblings, 0 replies; 27+ messages in thread From: Jon Kohler @ 2025-12-02 16:54 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Jason Wang, Arnd Bergmann, Eugenio Pérez, kvm@vger.kernel.org, virtualization@lists.linux.dev, Netdev, linux-kernel@vger.kernel.org, Linus Torvalds, Borislav Petkov, Sean Christopherson, linux-arm-kernel@lists.infradead.org, Russell King, Catalin Marinas, Will Deacon, Krzysztof Kozlowski, Alexandre Belloni, Linus Walleij, Drew Fustini > On Nov 27, 2025, at 1:32 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Thu, Nov 27, 2025 at 03:11:57AM +0000, Jon Kohler wrote: >> >> >>> On Nov 26, 2025, at 8:08 PM, Jason Wang <jasowang@redhat.com> wrote: >>> >>> On Thu, Nov 27, 2025 at 3:48 AM Jon Kohler <jon@nutanix.com> wrote: >>>> >>>> >>>>> On Nov 26, 2025, at 5:25 AM, Arnd Bergmann <arnd@arndb.de> wrote: >>>>> >>>>> On Wed, Nov 26, 2025, at 07:04, Jason Wang wrote: >>>>>> On Wed, Nov 26, 2025 at 3:45 AM Jon Kohler <jon@nutanix.com> wrote: >>>>>>>> On Nov 19, 2025, at 8:57 PM, Jason Wang <jasowang@redhat.com> wrote: >>>>>>>> On Tue, Nov 18, 2025 at 1:35 AM Jon Kohler <jon@nutanix.com> wrote: >>>>>>> Same deal goes for __put_user() vs put_user by way of commit >>>>>>> e3aa6243434f ("ARM: 8795/1: spectre-v1.1: use put_user() for __put_user()”) >>>>>>> >>>>>>> Looking at arch/arm/mm/Kconfig, there are a variety of scenarios >>>>>>> where CONFIG_CPU_SPECTRE will be enabled automagically. Looking at >>>>>>> commit 252309adc81f ("ARM: Make CONFIG_CPU_V7 valid for 32bit ARMv8 implementations") >>>>>>> it says that "ARMv8 is a superset of ARMv7", so I’d guess that just >>>>>>> about everything ARM would include this by default? >>>>> >>>>> I think the more relevant commit is for 64-bit Arm here, but this does >>>>> the same thing, see 84624087dd7e ("arm64: uaccess: Don't bother >>>>> eliding access_ok checks in __{get, put}_user"). >>>> >>>> Ah! Right, this is definitely the important bit, as it makes it >>>> crystal clear that these are exactly the same thing. The current >>>> code is: >>>> #define get_user __get_user >>>> #define put_user __put_user >>>> >>>> So, this patch changing from __* to regular versions is a no-op >>>> on arm side of the house, yea? >>>> >>>>> I would think that if we change the __get_user() to get_user() >>>>> in this driver, the same should be done for the >>>>> __copy_{from,to}_user(), which similarly skips the access_ok() >>>>> check but not the PAN/SMAP handling. >>>> >>>> Perhaps, thats a good call out. I’d file that under one battle >>>> at a time. Let’s get get/put user dusted first, then go down >>>> that road? >>>> >>>>> In general, the access_ok()/__get_user()/__copy_from_user() >>>>> pattern isn't really helpful any more, as Linus already >>>>> explained. I can't tell from the vhost driver code whether >>>>> we can just drop the access_ok() here and use the plain >>>>> get_user()/copy_from_user(), or if it makes sense to move >>>>> to the newer user_access_begin()/unsafe_get_user()/ >>>>> unsafe_copy_from_user()/user_access_end() and try optimize >>>>> out a few PAN/SMAP flips in the process. >>> >>> Right, according to my testing in the past, PAN/SMAP is a killer for >>> small packet performance (PPS). >> >> For sure, every little bit helps along that path >> >>> >>>> >>>> In general, I think there are a few spots where we might be >>>> able to optimize (vhost_get_vq_desc perhaps?) as that gets >>>> called quite a bit and IIRC there are at least two flips >>>> in there that perhaps we could elide to one? An investigation >>>> for another day I think. >>> >>> Did you mean trying to read descriptors in a batch, that would be >>> better and with IN_ORDER it would be even faster as a single (at most >>> two) copy_from_user() might work (without the need to use >>> user_access_begin()/user_access_end(). >> >> Yep. I haven’t fully thought through it, just a drive-by idea >> from looking at code for the recent work I’ve been doing, just >> scratching my head thinking there *must* be something we can do >> better there. >> >> Basically on the get rx/tx bufs path as well as the >> vhost_add_used_and_signal_n path, I think we could cluster together >> some of the get/put users and copy to/from’s. Would take some >> massaging, but I think there is something there. >> >>>> >>>> Anyhow, with this info - Jason - is there anything else you >>>> can think of that we want to double click on? >>> >>> Nope. >>> >>> Thanks >> >> Ok thanks. Perhaps we can land this in next and let it soak in, >> though, knock on wood, I don’t think there will be fallout >> (famous last words!) ? >> > > > To clairify, I think vhost tree is better to put this > in next than net-next, both because it's purely core vhost > and because unlike net-next vhost rebases so it is easy to > just drop the patch if there are issues. > I'll put it there. > > -- > MST > Ok cool, thank you! ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net-next] vhost: use "checked" versions of get_user() and put_user() 2025-11-13 0:55 [PATCH net-next] vhost: use "checked" versions of get_user() and put_user() Jon Kohler 2025-11-13 1:09 ` Jason Wang @ 2025-11-14 18:54 ` David Laight 2025-11-14 19:30 ` Jon Kohler 1 sibling, 1 reply; 27+ messages in thread From: David Laight @ 2025-11-14 18:54 UTC (permalink / raw) To: Jon Kohler Cc: Michael S. Tsirkin, Jason Wang, Eugenio Pérez, kvm, virtualization, netdev, linux-kernel, Linus Torvalds, Borislav Petkov, Sean Christopherson On Wed, 12 Nov 2025 17:55:28 -0700 Jon Kohler <jon@nutanix.com> wrote: > vhost_get_user and vhost_put_user leverage __get_user and __put_user, > respectively, which were both added in 2016 by commit 6b1e6cc7855b > ("vhost: new device IOTLB API"). In a heavy UDP transmit workload on a > vhost-net backed tap device, these functions showed up as ~11.6% of > samples in a flamegraph of the underlying vhost worker thread. > > Quoting Linus from [1]: > Anyway, every single __get_user() call I looked at looked like > historical garbage. [...] End result: I get the feeling that we > should just do a global search-and-replace of the __get_user/ > __put_user users, replace them with plain get_user/put_user instead, > and then fix up any fallout (eg the coco code). > > Switch to plain get_user/put_user in vhost, which results in a slight > throughput speedup. get_user now about ~8.4% of samples in flamegraph. > > Basic iperf3 test on a Intel 5416S CPU with Ubuntu 25.10 guest: > TX: taskset -c 2 iperf3 -c <rx_ip> -t 60 -p 5200 -b 0 -u -i 5 > RX: taskset -c 2 iperf3 -s -p 5200 -D > Before: 6.08 Gbits/sec > After: 6.32 Gbits/sec > > As to what drives the speedup, Sean's patch [2] explains: > Use the normal, checked versions for get_user() and put_user() instead of > the double-underscore versions that omit range checks, as the checked > versions are actually measurably faster on modern CPUs (12%+ on Intel, > 25%+ on AMD). Is there an associated access_ok() that can also be removed? David ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net-next] vhost: use "checked" versions of get_user() and put_user() 2025-11-14 18:54 ` David Laight @ 2025-11-14 19:30 ` Jon Kohler 2025-11-14 20:32 ` David Laight 2025-11-16 6:32 ` Michael S. Tsirkin 0 siblings, 2 replies; 27+ messages in thread From: Jon Kohler @ 2025-11-14 19:30 UTC (permalink / raw) To: David Laight Cc: Michael S. Tsirkin, Jason Wang, Eugenio Pérez, kvm@vger.kernel.org, virtualization@lists.linux.dev, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Linus Torvalds, Borislav Petkov, Sean Christopherson > On Nov 14, 2025, at 1:54 PM, David Laight <david.laight.linux@gmail.com> wrote: > > !-------------------------------------------------------------------| > CAUTION: External Email > > |-------------------------------------------------------------------! > > On Wed, 12 Nov 2025 17:55:28 -0700 > Jon Kohler <jon@nutanix.com> wrote: > >> vhost_get_user and vhost_put_user leverage __get_user and __put_user, >> respectively, which were both added in 2016 by commit 6b1e6cc7855b >> ("vhost: new device IOTLB API"). In a heavy UDP transmit workload on a >> vhost-net backed tap device, these functions showed up as ~11.6% of >> samples in a flamegraph of the underlying vhost worker thread. >> >> Quoting Linus from [1]: >> Anyway, every single __get_user() call I looked at looked like >> historical garbage. [...] End result: I get the feeling that we >> should just do a global search-and-replace of the __get_user/ >> __put_user users, replace them with plain get_user/put_user instead, >> and then fix up any fallout (eg the coco code). >> >> Switch to plain get_user/put_user in vhost, which results in a slight >> throughput speedup. get_user now about ~8.4% of samples in flamegraph. >> >> Basic iperf3 test on a Intel 5416S CPU with Ubuntu 25.10 guest: >> TX: taskset -c 2 iperf3 -c <rx_ip> -t 60 -p 5200 -b 0 -u -i 5 >> RX: taskset -c 2 iperf3 -s -p 5200 -D >> Before: 6.08 Gbits/sec >> After: 6.32 Gbits/sec >> >> As to what drives the speedup, Sean's patch [2] explains: >> Use the normal, checked versions for get_user() and put_user() instead of >> the double-underscore versions that omit range checks, as the checked >> versions are actually measurably faster on modern CPUs (12%+ on Intel, >> 25%+ on AMD). > > Is there an associated access_ok() that can also be removed? > > David Hey David - IIUC, the access_ok() for non-iotlb setups is done at initial setup time, not per event, see vhost_vring_set_addr and for the vhost net side see vhost_net_set_backend -> vhost_vq_access_ok. Will lean on MST/Jason to help sanity check my understanding. In the iotlb case, that’s handled differently (Jason can speak to that side), but I dont think there is something we’d remove there? ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net-next] vhost: use "checked" versions of get_user() and put_user() 2025-11-14 19:30 ` Jon Kohler @ 2025-11-14 20:32 ` David Laight 2025-11-16 6:32 ` Michael S. Tsirkin 1 sibling, 0 replies; 27+ messages in thread From: David Laight @ 2025-11-14 20:32 UTC (permalink / raw) To: Jon Kohler Cc: Michael S. Tsirkin, Jason Wang, Eugenio Pérez, kvm@vger.kernel.org, virtualization@lists.linux.dev, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Linus Torvalds, Borislav Petkov, Sean Christopherson On Fri, 14 Nov 2025 19:30:32 +0000 Jon Kohler <jon@nutanix.com> wrote: > > On Nov 14, 2025, at 1:54 PM, David Laight <david.laight.linux@gmail.com> wrote: > > > > !-------------------------------------------------------------------| > > CAUTION: External Email > > > > |-------------------------------------------------------------------! > > > > On Wed, 12 Nov 2025 17:55:28 -0700 > > Jon Kohler <jon@nutanix.com> wrote: > > > >> vhost_get_user and vhost_put_user leverage __get_user and __put_user, > >> respectively, which were both added in 2016 by commit 6b1e6cc7855b > >> ("vhost: new device IOTLB API"). In a heavy UDP transmit workload on a > >> vhost-net backed tap device, these functions showed up as ~11.6% of > >> samples in a flamegraph of the underlying vhost worker thread. > >> > >> Quoting Linus from [1]: > >> Anyway, every single __get_user() call I looked at looked like > >> historical garbage. [...] End result: I get the feeling that we > >> should just do a global search-and-replace of the __get_user/ > >> __put_user users, replace them with plain get_user/put_user instead, > >> and then fix up any fallout (eg the coco code). > >> > >> Switch to plain get_user/put_user in vhost, which results in a slight > >> throughput speedup. get_user now about ~8.4% of samples in flamegraph. > >> > >> Basic iperf3 test on a Intel 5416S CPU with Ubuntu 25.10 guest: > >> TX: taskset -c 2 iperf3 -c <rx_ip> -t 60 -p 5200 -b 0 -u -i 5 > >> RX: taskset -c 2 iperf3 -s -p 5200 -D > >> Before: 6.08 Gbits/sec > >> After: 6.32 Gbits/sec > >> > >> As to what drives the speedup, Sean's patch [2] explains: > >> Use the normal, checked versions for get_user() and put_user() instead of > >> the double-underscore versions that omit range checks, as the checked > >> versions are actually measurably faster on modern CPUs (12%+ on Intel, > >> 25%+ on AMD). > > > > Is there an associated access_ok() that can also be removed? > > > > David > > Hey David - IIUC, the access_ok() for non-iotlb setups is done at > initial setup time, not per event, see vhost_vring_set_addr and > for the vhost net side see vhost_net_set_backend -> > vhost_vq_access_ok. This is a long way away from the actual access.... The early 'sanity check' might be worth keeping, but the code has to allow for the user access faulting (the application might unmap it). But, in some sense, that early check is optimising for the user passing in an invalid buffer - so not actually worth while, > Will lean on MST/Jason to help sanity check my understanding. > > In the iotlb case, that’s handled differently (Jason can speak to > that side), but I dont think there is something we’d remove there? Isn't the application side much the same? (But I don't know what the code is doing...) David ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net-next] vhost: use "checked" versions of get_user() and put_user() 2025-11-14 19:30 ` Jon Kohler 2025-11-14 20:32 ` David Laight @ 2025-11-16 6:32 ` Michael S. Tsirkin 1 sibling, 0 replies; 27+ messages in thread From: Michael S. Tsirkin @ 2025-11-16 6:32 UTC (permalink / raw) To: Jon Kohler Cc: David Laight, Jason Wang, Eugenio Pérez, kvm@vger.kernel.org, virtualization@lists.linux.dev, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Linus Torvalds, Borislav Petkov, Sean Christopherson On Fri, Nov 14, 2025 at 07:30:32PM +0000, Jon Kohler wrote: > > > > On Nov 14, 2025, at 1:54 PM, David Laight <david.laight.linux@gmail.com> wrote: > > > > !-------------------------------------------------------------------| > > CAUTION: External Email > > > > |-------------------------------------------------------------------! > > > > On Wed, 12 Nov 2025 17:55:28 -0700 > > Jon Kohler <jon@nutanix.com> wrote: > > > >> vhost_get_user and vhost_put_user leverage __get_user and __put_user, > >> respectively, which were both added in 2016 by commit 6b1e6cc7855b > >> ("vhost: new device IOTLB API"). In a heavy UDP transmit workload on a > >> vhost-net backed tap device, these functions showed up as ~11.6% of > >> samples in a flamegraph of the underlying vhost worker thread. > >> > >> Quoting Linus from [1]: > >> Anyway, every single __get_user() call I looked at looked like > >> historical garbage. [...] End result: I get the feeling that we > >> should just do a global search-and-replace of the __get_user/ > >> __put_user users, replace them with plain get_user/put_user instead, > >> and then fix up any fallout (eg the coco code). > >> > >> Switch to plain get_user/put_user in vhost, which results in a slight > >> throughput speedup. get_user now about ~8.4% of samples in flamegraph. > >> > >> Basic iperf3 test on a Intel 5416S CPU with Ubuntu 25.10 guest: > >> TX: taskset -c 2 iperf3 -c <rx_ip> -t 60 -p 5200 -b 0 -u -i 5 > >> RX: taskset -c 2 iperf3 -s -p 5200 -D > >> Before: 6.08 Gbits/sec > >> After: 6.32 Gbits/sec > >> > >> As to what drives the speedup, Sean's patch [2] explains: > >> Use the normal, checked versions for get_user() and put_user() instead of > >> the double-underscore versions that omit range checks, as the checked > >> versions are actually measurably faster on modern CPUs (12%+ on Intel, > >> 25%+ on AMD). > > > > Is there an associated access_ok() that can also be removed? > > > > David > > Hey David - IIUC, the access_ok() for non-iotlb setups is done at > initial setup time, not per event, see vhost_vring_set_addr and > for the vhost net side see vhost_net_set_backend -> > vhost_vq_access_ok. > > Will lean on MST/Jason to help sanity check my understanding. Right. > In the iotlb case, that’s handled differently (Jason can speak to > that side), but I dont think there is something we’d remove there? ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2025-12-02 16:55 UTC | newest] Thread overview: 27+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-11-13 0:55 [PATCH net-next] vhost: use "checked" versions of get_user() and put_user() Jon Kohler 2025-11-13 1:09 ` Jason Wang 2025-11-14 14:53 ` Jon Kohler 2025-11-14 17:48 ` Linus Torvalds 2025-11-14 19:08 ` David Laight 2025-11-14 20:48 ` Linus Torvalds 2025-11-14 21:38 ` David Laight 2025-11-17 4:32 ` Jason Wang 2025-11-17 17:34 ` Jon Kohler 2025-11-20 1:57 ` Jason Wang 2025-11-25 19:45 ` Jon Kohler 2025-11-26 6:04 ` Jason Wang 2025-11-26 10:25 ` Arnd Bergmann 2025-11-26 19:47 ` Jon Kohler 2025-11-26 19:58 ` Arnd Bergmann 2025-11-26 21:42 ` Jon Kohler 2025-11-26 21:45 ` Linus Torvalds 2025-11-27 2:58 ` Jon Kohler 2025-11-27 1:08 ` Jason Wang 2025-11-27 3:11 ` Jon Kohler 2025-11-27 6:31 ` Michael S. Tsirkin 2025-11-27 6:32 ` Michael S. Tsirkin 2025-12-02 16:54 ` Jon Kohler 2025-11-14 18:54 ` David Laight 2025-11-14 19:30 ` Jon Kohler 2025-11-14 20:32 ` David Laight 2025-11-16 6:32 ` Michael S. Tsirkin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).