On Tue, Dec 25, 2018 at 06:05:25PM +0800, Jason Wang wrote:
On 2018/12/25 äå2:10, Michael S. Tsirkin wrote:Thanks!
On Mon, Dec 24, 2018 at 03:53:16PM +0800, Jason Wang wrote:
On 2018/12/14 äå8:36, Michael S. Tsirkin wrote:We as in vhost? networking isn't the only thing guest does.
On Fri, Dec 14, 2018 at 11:57:35AM +0800, Jason Wang wrote:The pages wil be released during set features.
On 2018/12/13 äå11:44, Michael S. Tsirkin wrote:
On Thu, Dec 13, 2018 at 06:10:22PM +0800, Jason Wang wrote:
It was noticed that the copy_user() friends that was used to accessAs I said I have doubts about the whole approach, but this
virtqueue metdata tends to be very expensive for dataplane
implementation like vhost since it involves lots of software check,
speculation barrier, hardware feature toggling (e.g SMAP). The
extra cost will be more obvious when transferring small packets.
This patch tries to eliminate those overhead by pin vq metadata pages
and access them through vmap(). During SET_VRING_ADDR, we will setup
those mappings and memory accessors are modified to use pointers to
access the metadata directly.
Note, this was only done when device IOTLB is not enabled. We could
use similar method to optimize it in the future.
Tests shows about ~24% improvement on TX PPS when using virtio-user +
vhost_net + xdp1 on TAP (CONFIG_HARDENED_USERCOPY is not enabled):
Signed-off-by: Jason Wang<jasowang@xxxxxxxxxx>
drivers/vhost/vhost.c | 178 ++++++++++++++++++++++++++++++++++++++++++
drivers/vhost/vhost.h | 11 +++
2 files changed, 189 insertions(+)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index bafe39d2e637..1bd24203afb6 100644
@@ -443,6 +443,9 @@ void vhost_dev_init(struct vhost_dev *dev,
vq->indirect = NULL;
vq->heads = NULL;
vq->dev = dev;
+ memset(&vq->avail_ring, 0, sizeof(vq->avail_ring));
+ memset(&vq->used_ring, 0, sizeof(vq->used_ring));
+ memset(&vq->desc_ring, 0, sizeof(vq->desc_ring));
@@ -614,6 +617,102 @@ static void vhost_clear_msg(struct vhost_dev *dev)
+static int vhost_init_vmap(struct vhost_vmap *map, unsigned long uaddr,
+ size_t size, int write)
+ struct page **pages;
+ int npages = DIV_ROUND_UP(size, PAGE_SIZE);
+ int npinned;
+ void *vaddr;
+ pages = kmalloc_array(npages, sizeof(struct page *), GFP_KERNEL);
+ if (!pages)
+ return -ENOMEM;
+ npinned = get_user_pages_fast(uaddr, npages, write, pages);
+ if (npinned != npages)
+ goto err;
implementation in particular isn't a good idea
as it keeps the page around forever.
For THP, we will probably miss 2 or 4 pages, but does this really matterSo no THP, no NUMA rebalancing,
consider the gain we have?
We don't even know if this guest does a lot of networking.
know what else is in this huge page. Can be something very important
that guest touches all the time.
Well, the probability should be very small consider we usually give several
gigabytes to guest. The rest of the pages that doesn't sit in the same
hugepage with metadata can still be merged by THP.Â Anyway, I can test the
I am not sure why. It should be easy enough to force the VCPU and vhostFor NUMA rebalancing, I'm even not quite sure ifSo again it's a gain for vhost but has a completely unpredictable effect on
it can helps for the case of IPC (vhost). It looks to me the worst case it
may cause page to be thrash between nodes if vhost and userspace are running
in two nodes.
other functionality of the guest.
That's what bothers me with this approach.
- The rest of the pages could still be balanced to other nodes, no?
- try to balance metadata pages (belongs to co-operate processes) itself is
to move (e.g. start them pinned to 1 cpu, then pin them to another one).
Clearly sometimes this would be necessary for load balancing reasons.
With autonuma after a while (could take seconds but it will happen) the
memory will migrate.
But only for TX and temporarily. We pin, read, unpin.Zerocopy codes (enabled by defualt) use them for years.This is the price of all GUP users not only vhost itself.Yes. GUP is just not a great interface for vhost to use.
Probably not. For several reasons that the page will be not be released soon
or held for a very long period of time or even forever.
With zero copy? Well it's pinned until transmit. Takes a while
but could be enough for autocopy to work esp since
its the packet memory so not reused immediately.
Your patch is different
- it writes into memory and GUP has known issues with file
The ordinary user for vhost is anonymous pages I think?
It's not the most common scenario and not the fastest one
(e.g. THP does not work) but file backed is useful sometimes.
It would not be nice at all to corrupt guest memory in that case.
OK let's forget smap for now.- it keeps pages pinned forever
It's true for a real life applications but a malicious oneWhat kind of assumption do you they have?What's moreSo these guys assume userspace knows what it's doing.
important, the goal is not to be left too much behind for other backends
like DPDK or AF_XDP (all of which are using GUP).
We can't assume that.
The problem is not kthread but the address of userspace address. TheI don't see how a kthread makes any difference. We do have a validationuserspace-controlledIt's pretty easy to add this since the slow path was still kept. If we
amount of memory locked up and not accounted for.
exceeds the limitation, we can switch back to slow path.
Don't get me wrong it's a great patch in an ideal world.Again, this is only for metadata accessing not the data which has been used
But then in an ideal world no barriers smap etc are necessary at all.
for years for real use cases.
For SMAP, it makes senses for the address that kernel can not forcast. But
it's not the case for the vhost metadata since we know the address will be
accessed very frequently. For speculation barrier, it helps nothing for the
data path of vhost which is a kthread.
step which makes some difference.
addresses of vq metadata tends to be consistent for a while, and vhost knows
they will be frequently. SMAP doesn't help too much in this case.
can call the setup ioctls any number of times. And SMAP is
all about malcious applications.
We don't do this in the path of ioctl, there's no context switch between
userspace and kernel in the worker thread. SMAP is used to prevent kernel
from accessing userspace pages unexpectedly which is not the case for
Packet or AF_XDP benefit from
accessing metadata directly, we should do it as well.