From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751975AbdI1HpK (ORCPT ); Thu, 28 Sep 2017 03:45:10 -0400 Received: from mx1.redhat.com ([209.132.183.28]:48354 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751437AbdI1HpI (ORCPT ); Thu, 28 Sep 2017 03:45:08 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 5864F98195 Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=jasowang@redhat.com Subject: Re: [PATCH net-next RFC 2/5] vhost: introduce helper to prefetch desc index To: Willem de Bruijn Cc: "Michael S. Tsirkin" , virtualization@lists.linux-foundation.org, Network Development , LKML , kvm@vger.kernel.org References: <1506067355-5771-1-git-send-email-jasowang@redhat.com> <1506067355-5771-3-git-send-email-jasowang@redhat.com> From: Jason Wang Message-ID: <4450795e-7967-8427-6b41-28fe4f912e51@redhat.com> Date: Thu, 28 Sep 2017 15:44:50 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Thu, 28 Sep 2017 07:45:08 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2017年09月28日 08:47, Willem de Bruijn wrote: > On Fri, Sep 22, 2017 at 4:02 AM, Jason Wang wrote: >> This patch introduces vhost_prefetch_desc_indices() which could batch >> descriptor indices fetching and used ring updating. This intends to >> reduce the cache misses of indices fetching and updating and reduce >> cache line bounce when virtqueue is almost full. copy_to_user() was >> used in order to benefit from modern cpus that support fast string >> copy. Batched virtqueue processing will be the first user. >> >> Signed-off-by: Jason Wang >> --- >> drivers/vhost/vhost.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++ >> drivers/vhost/vhost.h | 3 +++ >> 2 files changed, 58 insertions(+) >> >> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c >> index f87ec75..8424166d 100644 >> --- a/drivers/vhost/vhost.c >> +++ b/drivers/vhost/vhost.c >> @@ -2437,6 +2437,61 @@ struct vhost_msg_node *vhost_dequeue_msg(struct vhost_dev *dev, >> } >> EXPORT_SYMBOL_GPL(vhost_dequeue_msg); >> >> +int vhost_prefetch_desc_indices(struct vhost_virtqueue *vq, >> + struct vring_used_elem *heads, >> + u16 num, bool used_update) >> +{ >> + int ret, ret2; >> + u16 last_avail_idx, last_used_idx, total, copied; >> + __virtio16 avail_idx; >> + struct vring_used_elem __user *used; >> + int i; >> + >> + if (unlikely(vhost_get_avail(vq, avail_idx, &vq->avail->idx))) { >> + vq_err(vq, "Failed to access avail idx at %p\n", >> + &vq->avail->idx); >> + return -EFAULT; >> + } >> + last_avail_idx = vq->last_avail_idx & (vq->num - 1); >> + vq->avail_idx = vhost16_to_cpu(vq, avail_idx); >> + total = vq->avail_idx - vq->last_avail_idx; >> + ret = total = min(total, num); >> + >> + for (i = 0; i < ret; i++) { >> + ret2 = vhost_get_avail(vq, heads[i].id, >> + &vq->avail->ring[last_avail_idx]); >> + if (unlikely(ret2)) { >> + vq_err(vq, "Failed to get descriptors\n"); >> + return -EFAULT; >> + } >> + last_avail_idx = (last_avail_idx + 1) & (vq->num - 1); >> + } > This is understandably very similar to the existing logic in vhost_get_vq_desc. > Can that be extracted to a helper to avoid code duplication? > > Perhaps one helper to update vq->avail_idx and return num, and > another to call vhost_get_avail one or more times. Yes it can. > >> + >> + if (!used_update) >> + return ret; >> + >> + last_used_idx = vq->last_used_idx & (vq->num - 1); >> + while (total) { >> + copied = min((u16)(vq->num - last_used_idx), total); >> + ret2 = vhost_copy_to_user(vq, >> + &vq->used->ring[last_used_idx], >> + &heads[ret - total], >> + copied * sizeof(*used)); >> + >> + if (unlikely(ret2)) { >> + vq_err(vq, "Failed to update used ring!\n"); >> + return -EFAULT; >> + } >> + >> + last_used_idx = 0; >> + total -= copied; >> + } > This second part seems unrelated and could be a separate function? Yes. > > Also, no need for ret2 and double assignment "ret = total =" if not > modifying total > in the the second loop: > > for (i = 0; i < total; ) { > ... > i += copied; > } Right, will do this in V2. Thanks