Re: [PATCH net-next 0/3] vhost: accelerate metadata access through vmap()

[Jason Wang]

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