> On Nov 27, 2025, at 1:32 AM, Michael S. Tsirkin <[email protected]> wrote: > > On Thu, Nov 27, 2025 at 03:11:57AM +0000, Jon Kohler wrote: >> >> >>> On Nov 26, 2025, at 8:08 PM, Jason Wang <[email protected]> wrote: >>> >>> On Thu, Nov 27, 2025 at 3:48 AM Jon Kohler <[email protected]> wrote: >>>> >>>> >>>>> On Nov 26, 2025, at 5:25 AM, Arnd Bergmann <[email protected]> wrote: >>>>> >>>>> On Wed, Nov 26, 2025, at 07:04, Jason Wang wrote: >>>>>> On Wed, Nov 26, 2025 at 3:45 AM Jon Kohler <[email protected]> wrote: >>>>>>>> On Nov 19, 2025, at 8:57 PM, Jason Wang <[email protected]> wrote: >>>>>>>> On Tue, Nov 18, 2025 at 1:35 AM Jon Kohler <[email protected]> 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!
