From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <yuanhan.liu@linux.intel.com>
Received: from mga09.intel.com (mga09.intel.com [134.134.136.24])
by dpdk.org (Postfix) with ESMTP id 3E777370
for <dev@dpdk.org>; Tue, 22 Dec 2015 03:39:49 +0100 (CET)
Received: from fmsmga003.fm.intel.com ([10.253.24.29])
by orsmga102.jf.intel.com with ESMTP; 21 Dec 2015 18:39:48 -0800
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.20,462,1444719600"; d="scan'208";a="621945565"
Received: from yliu-dev.sh.intel.com (HELO yliu-dev) ([10.239.66.49])
by FMSMGA003.fm.intel.com with ESMTP; 21 Dec 2015 18:39:46 -0800
Date: Tue, 22 Dec 2015 10:40:58 +0800
From: Yuanhan Liu <yuanhan.liu@linux.intel.com>
To: "Xie, Huawei" <huawei.xie@intel.com>
Message-ID: <20151222024058.GE18863@yliu-dev.sh.intel.com>
References: <1449027793-30975-1-git-send-email-yuanhan.liu@linux.intel.com>
<1450321921-27799-1-git-send-email-yuanhan.liu@linux.intel.com>
<1450321921-27799-3-git-send-email-yuanhan.liu@linux.intel.com>
<C37D651A908B024F974696C65296B57B4C54DD5F@SHSMSX101.ccr.corp.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <C37D651A908B024F974696C65296B57B4C54DD5F@SHSMSX101.ccr.corp.intel.com>
User-Agent: Mutt/1.5.23 (2014-03-12)
Cc: "Michael S. Tsirkin" <mst@redhat.com>, "dev@dpdk.org" <dev@dpdk.org>,
Victor Kaplansky <vkaplans@redhat.com>
Subject: Re: [dpdk-dev] [PATCH v2 2/6] vhost: introduce vhost_log_write
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: patches and discussions about DPDK <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Tue, 22 Dec 2015 02:39:49 -0000
On Mon, Dec 21, 2015 at 03:06:43PM +0000, Xie, Huawei wrote:
> On 12/17/2015 11:11 AM, Yuanhan Liu wrote:
> > Introduce vhost_log_write() helper function to log the dirty pages we
> > touched. Page size is harded code to 4096 (VHOST_LOG_PAGE), and each
> > log is presented by 1 bit.
> >
> > Therefore, vhost_log_write() simply finds the right bit for related
> > page we are gonna change, and set it to 1. dev->log_base denotes the
> > start of the dirty page bitmap.
> >
> > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> > Signed-off-by: Victor Kaplansky <victork@redhat.com
> > ---
> > lib/librte_vhost/rte_virtio_net.h | 29 +++++++++++++++++++++++++++++
> > 1 file changed, 29 insertions(+)
> >
> > diff --git a/lib/librte_vhost/rte_virtio_net.h b/lib/librte_vhost/rte_virtio_net.h
> > index 8acee02..5726683 100644
> > --- a/lib/librte_vhost/rte_virtio_net.h
> > +++ b/lib/librte_vhost/rte_virtio_net.h
> > @@ -40,6 +40,7 @@
> > */
> >
> > #include <stdint.h>
> > +#include <linux/vhost.h>
> > #include <linux/virtio_ring.h>
> > #include <linux/virtio_net.h>
> > #include <sys/eventfd.h>
> > @@ -59,6 +60,8 @@ struct rte_mbuf;
> > /* Backend value set by guest. */
> > #define VIRTIO_DEV_STOPPED -1
> >
> > +#define VHOST_LOG_PAGE 4096
> > +
> >
> > /* Enum for virtqueue management. */
> > enum {VIRTIO_RXQ, VIRTIO_TXQ, VIRTIO_QNUM};
> > @@ -205,6 +208,32 @@ gpa_to_vva(struct virtio_net *dev, uint64_t guest_pa)
> > return vhost_va;
> > }
> >
> > +static inline void __attribute__((always_inline))
> > +vhost_log_page(uint8_t *log_base, uint64_t page)
> > +{
> > + log_base[page / 8] |= 1 << (page % 8);
> > +}
> > +
> Those logging functions are not supposed to be API. Could we move them
> into an internal header file?
Agreed. I should have put them into vhost_rxtx.c
> > +static inline void __attribute__((always_inline))
> > +vhost_log_write(struct virtio_net *dev, uint64_t addr, uint64_t len)
> > +{
> > + uint64_t page;
> > +
> Before we log, we need memory barrier to make sure updates are in place.
> > + if (likely(((dev->features & (1ULL << VHOST_F_LOG_ALL)) == 0) ||
> > + !dev->log_base || !len))
> > + return;
Put a memory barrier inside set_features()?
I see no var dependence here, why putting a barrier then? We are
accessing and modifying same var, doesn't the cache MESI protocol
will get rid of your concerns?
> > +
> > + if (unlikely(dev->log_size < ((addr + len - 1) / VHOST_LOG_PAGE / 8)))
> > + return;
> > +
> > + page = addr / VHOST_LOG_PAGE;
> > + while (page * VHOST_LOG_PAGE < addr + len) {
> Let us have a page_end var to make the code simpler?
Could do that.
> > + vhost_log_page((uint8_t *)(uintptr_t)dev->log_base, page);
> > + page += VHOST_LOG_PAGE;
> page += 1?
Oops, right.
--yliu
> > + }
> > +}
> > +
> > +
> > /**
> > * Disable features in feature_mask. Returns 0 on success.
> > */
>