From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id A84B537A6 for ; Wed, 2 Dec 2015 14:53:05 +0100 (CET) Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id 04232C04FF81; Wed, 2 Dec 2015 13:53:04 +0000 (UTC) Received: from redhat.com (vpn-201-9.tlv.redhat.com [10.35.201.9]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with SMTP id tB2Dr2be006388; Wed, 2 Dec 2015 08:53:03 -0500 Date: Wed, 2 Dec 2015 15:53:01 +0200 From: Victor Kaplansky To: Yuanhan Liu Message-ID: <20151202153050-mutt-send-email-victork@redhat.com> References: <1449027793-30975-1-git-send-email-yuanhan.liu@linux.intel.com> <1449027793-30975-3-git-send-email-yuanhan.liu@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1449027793-30975-3-git-send-email-yuanhan.liu@linux.intel.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.22 Cc: dev@dpdk.org, "Michael S. Tsirkin" Subject: Re: [dpdk-dev] [PATCH 2/4] 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 02 Dec 2015 13:53:06 -0000 On Wed, Dec 02, 2015 at 11:43:11AM +0800, 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. > > The page address is biased by log_guest_addr, which is derived from > SET_VRING_ADDR request as part of the vring related addresses. > > Signed-off-by: Yuanhan Liu > --- > lib/librte_vhost/rte_virtio_net.h | 34 ++++++++++++++++++++++++++++++++++ > lib/librte_vhost/virtio-net.c | 4 ++++ > 2 files changed, 38 insertions(+) > > diff --git a/lib/librte_vhost/rte_virtio_net.h b/lib/librte_vhost/rte_virtio_net.h > index 416dac2..191c1be 100644 > --- a/lib/librte_vhost/rte_virtio_net.h > +++ b/lib/librte_vhost/rte_virtio_net.h > @@ -40,6 +40,7 @@ > */ > > #include > +#include > #include > #include > #include > @@ -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}; > @@ -82,6 +85,7 @@ struct vhost_virtqueue { > struct vring_desc *desc; /**< Virtqueue descriptor ring. */ > struct vring_avail *avail; /**< Virtqueue available ring. */ > struct vring_used *used; /**< Virtqueue used ring. */ > + uint64_t log_guest_addr; /**< Physical address of used ring, for logging */ > uint32_t size; /**< Size of descriptor ring. */ > uint32_t backend; /**< Backend value to determine if device should started/stopped. */ > uint16_t vhost_hlen; /**< Vhost header length (varies depending on RX merge buffers. */ > @@ -203,6 +207,36 @@ 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) > +{ > + /* TODO: to make it atomic? */ > + log_base[page / 8] |= 1 << (page % 8); I think the atomic OR operation is necessary only if there can be more than one vhost-user back-end updating the guest's memory simultaneously. However probably it is pretty safe to perform regular OR operation, since rings are not shared between back-end. What about buffers pointed by descriptors? To be on the safe side, I would use a GCC built-in function __sync_fetch_and_or(). > +} > + > +static inline void __attribute__((always_inline)) > +vhost_log_write(struct virtio_net *dev, struct vhost_virtqueue *vq, > + uint64_t offset, uint64_t len) > +{ > + uint64_t addr = vq->log_guest_addr; > + uint64_t page; > + > + if (unlikely(((dev->features & (1ULL << VHOST_F_LOG_ALL)) == 0) || > + !dev->log_base || !len)) > + return; Isn't "likely" more appropriate in above, since the whole expression is expected to be true most of the time? > + > + addr += offset; > + if (dev->log_size < ((addr + len - 1) / VHOST_LOG_PAGE / 8)) > + return; > + > + page = addr / VHOST_LOG_PAGE; > + while (page * VHOST_LOG_PAGE < addr + len) { > + vhost_log_page(dev->log_base, page); > + page += VHOST_LOG_PAGE; > + } > +} > + > + > /** > * Disable features in feature_mask. Returns 0 on success. > */ > diff --git a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c > index 8364938..4481827 100644 > --- a/lib/librte_vhost/virtio-net.c > +++ b/lib/librte_vhost/virtio-net.c > @@ -666,12 +666,16 @@ set_vring_addr(struct vhost_device_ctx ctx, struct vhost_vring_addr *addr) > return -1; > } > > + vq->log_guest_addr = addr->log_guest_addr; > + > LOG_DEBUG(VHOST_CONFIG, "(%"PRIu64") mapped address desc: %p\n", > dev->device_fh, vq->desc); > LOG_DEBUG(VHOST_CONFIG, "(%"PRIu64") mapped address avail: %p\n", > dev->device_fh, vq->avail); > LOG_DEBUG(VHOST_CONFIG, "(%"PRIu64") mapped address used: %p\n", > dev->device_fh, vq->used); > + LOG_DEBUG(VHOST_CONFIG, "(%"PRIu64") log_guest_addr: %p\n", > + dev->device_fh, (void *)(uintptr_t)vq->log_guest_addr); > > return 0; > } > -- > 1.9.0