From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id C49DA1B31D; Wed, 16 May 2018 10:08:34 +0200 (CEST) X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 16 May 2018 01:08:32 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.49,390,1520924400"; d="scan'208";a="54782445" Received: from debian.sh.intel.com (HELO debian) ([10.67.104.203]) by fmsmga004.fm.intel.com with ESMTP; 16 May 2018 01:08:31 -0700 Date: Wed, 16 May 2018 16:08:58 +0800 From: Tiwei Bie To: Maxime Coquelin Cc: dev@dpdk.org, mst@redhat.com, stable@dpdk.org Message-ID: <20180516080858.GA15144@debian> References: <20180515173021.31903-1-maxime.coquelin@redhat.com> <20180516062107.GA26229@debian> <465b4530-f19e-66e0-fc32-437595a3819e@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <465b4530-f19e-66e0-fc32-437595a3819e@redhat.com> User-Agent: Mutt/1.9.5 (2018-04-13) Subject: Re: [dpdk-dev] [PATCH v2] vhost: improve dirty pages logging performance X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 16 May 2018 08:08:36 -0000 On Wed, May 16, 2018 at 09:49:45AM +0200, Maxime Coquelin wrote: > On 05/16/2018 08:21 AM, Tiwei Bie wrote: > > On Tue, May 15, 2018 at 07:30:21PM +0200, Maxime Coquelin wrote: [...] > > > @@ -309,7 +322,15 @@ struct virtio_net { > > > static __rte_always_inline void > > > vhost_set_bit(unsigned int nr, volatile uint8_t *addr) > > > { > > > +#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION < 70100) > > > > Just curious, is there any reference about why > > this version was chosen? Thanks! > > I googled Michael reference to the LWN article [0], and they mention GCC > 7.1. > > I haven't checked by myself though whether generated code is different in > GCC >= 7.1. > > [0]: https://lwn.net/Articles/691128/ Thanks for the link! > > > > > + /* > > > + * __sync_ built-ins are deprecated, but __atomic_ ones > > > + * are sub-optimized in older GCC versions. > > > + */ > > > > The indent isn't right (just need one tab here). > Right, will fix. > > > > > > __sync_fetch_and_or_8(addr, (1U << nr)); > > This is unrelated to this patch set, but from GCC doc [1], shouldn't > we use __sync_fetch_and_or_1 as the size is in bytes? > > [1]: https://gcc.gnu.org/onlinedocs/gcc/_005f_005fsync-Builtins.html You are right. I didn't notice it.. Please also fix this. > > > > +#else > > > + __atomic_fetch_or(addr, (1U << nr), __ATOMIC_RELAXED); [...] > > > @@ -106,6 +106,8 @@ flush_shadow_used_ring(struct virtio_net *dev, struct vhost_virtqueue *vq) > > > rte_smp_wmb(); > > > + vhost_log_cache_sync(dev, vq); > > > > Each time we call vhost_log_cache_sync(), there > > is already a rte_smp_wmb() which is to protect > > the used->idx update. So maybe there is no need > > to call rte_smp_wmb() in vhost_log_cache_sync(). > > Right, I can remove it in vhost_log_cache_sync(), and > maybe add a comment there stating that a write barrier > before calling the function is expected. Good idea. Best regards, Tiwei Bie