Messages in this thread | | | Subject | Re: [PATCH net-next 0/3] vhost: accelerate metadata access through vmap() | From | Jason Wang <> | Date | Mon, 24 Dec 2018 16:32:39 +0800 |
| |
On 2018/12/14 下午8:33, Michael S. Tsirkin wrote: > On Fri, Dec 14, 2018 at 11:42:18AM +0800, Jason Wang wrote: >> On 2018/12/13 下午11:27, Michael S. Tsirkin wrote: >>> On Thu, Dec 13, 2018 at 06:10:19PM +0800, Jason Wang wrote: >>>> Hi: >>>> >>>> This series tries to access virtqueue metadata through kernel virtual >>>> address instead of copy_user() friends since they had too much >>>> overheads like checks, spec barriers or even hardware feature >>>> toggling. >>> Userspace accesses through remapping tricks and next time there's a need >>> for a new barrier we are left to figure it out by ourselves. >> >> I don't get here, do you mean spec barriers? > I mean the next barrier people decide to put into userspace > memory accesses. > >> It's completely unnecessary for >> vhost which is kernel thread. > It's defence in depth. Take a look at the commit that added them. > And yes quite possibly in most cases we actually have a spec > barrier in the validation phase. If we do let's use the > unsafe variants so they can be found.
unsafe variants can only work if you can batch userspace access. This is not necessarily the case for light load.
> >> And even if you're right, vhost is not the >> only place, there's lots of vmap() based accessing in kernel. > For sure. But if one can get by without get user pages, one > really should. Witness recently uncovered mess with file > backed storage.
We only pin metadata pages, I don't believe they will be used by any DMA.
> >> Think in >> another direction, this means we won't suffer form unnecessary barriers for >> kthread like vhost in the future, we will manually pick the one we really >> need > I personally think we should err on the side of caution not on the side of > performance.
So what you suggest may lead unnecessary performance regression (10%-20%) which is part of the goal of this series. We should audit and only use the one we really need instead of depending on copy_user() friends().
If we do it our own, it could be slow for for security fix but it's no less safe than before with performance kept.
> >> (but it should have little possibility). > History seems to teach otherwise.
What case did you mean here?
> >> Please notice we only access metdata through remapping not the data itself. >> This idea has been used for high speed userspace backend for years, e.g >> packet socket or recent AF_XDP. > I think their justification for the higher risk is that they are mostly > designed for priveledged userspace.
I think it's the same with TUN/TAP, privileged process can pass them to unprivileged ones.
> >> The only difference is the page was remap to >> from kernel to userspace. > At least that avoids the g.u.p mess.
I'm still not very clear at the point. We only pin 2 or 4 pages, they're several other cases that will pin much more.
> >>> I don't >>> like the idea I have to say. As a first step, why don't we switch to >>> unsafe_put_user/unsafe_get_user etc? >> >> Several reasons: >> >> - They only have x86 variant, it won't have any difference for the rest of >> architecture. > Is there an issue on other architectures? If yes they can be extended > there.
Consider the unexpected amount of work and in the best case it can give the same performance to vmap(). I'm not sure it's worth.
> >> - unsafe_put_user/unsafe_get_user is not sufficient for accessing structures >> (e.g accessing descriptor) or arrays (batching). > So you want unsafe_copy_xxx_user? I can do this. Hang on will post. > >> - Unless we can batch at least the accessing of two places in three of >> avail, used and descriptor in one run. There will be no difference. E.g we >> can batch updating used ring, but it won't make any difference in this case. >> > So let's batch them all?
Batching might not help for the case of light load. And we need to measure the gain/cost of batching itself.
> > >>> That would be more of an apples to apples comparison, would it not? >> >> Apples to apples comparison only help if we are the No.1. But the fact is we >> are not. If we want to compete with e.g dpdk or AF_XDP, vmap() is the >> fastest method AFAIK. >> >> >> Thanks > We need to speed up the packet access itself too though. > You can't vmap all of guest memory.
This series only pin and vmap very few pages (metadata).
Thanks
> > >>> >>>> Test shows about 24% improvement on TX PPS. It should benefit other >>>> cases as well. >>>> >>>> Please review >>>> >>>> Jason Wang (3): >>>> vhost: generalize adding used elem >>>> vhost: fine grain userspace memory accessors >>>> vhost: access vq metadata through kernel virtual address >>>> >>>> drivers/vhost/vhost.c | 281 ++++++++++++++++++++++++++++++++++++++---- >>>> drivers/vhost/vhost.h | 11 ++ >>>> 2 files changed, 266 insertions(+), 26 deletions(-) >>>> >>>> -- >>>> 2.17.1
| |