From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 0A36D8D8F for ; Tue, 22 Dec 2015 08:02:14 +0100 (CET) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga102.fm.intel.com with ESMTP; 21 Dec 2015 23:02:14 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,463,1444719600"; d="scan'208";a="876550943" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by orsmga002.jf.intel.com with ESMTP; 21 Dec 2015 23:02:14 -0800 Received: from fmsmsx113.amr.corp.intel.com (10.18.116.7) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.248.2; Mon, 21 Dec 2015 23:02:13 -0800 Received: from shsmsx102.ccr.corp.intel.com (10.239.4.154) by FMSMSX113.amr.corp.intel.com (10.18.116.7) with Microsoft SMTP Server (TLS) id 14.3.248.2; Mon, 21 Dec 2015 23:02:12 -0800 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.190]) by shsmsx102.ccr.corp.intel.com ([169.254.2.158]) with mapi id 14.03.0248.002; Tue, 22 Dec 2015 15:02:10 +0800 From: "Xie, Huawei" To: Yuanhan Liu Thread-Topic: [PATCH v2 2/6] vhost: introduce vhost_log_write Thread-Index: AQHRPAEzuIYE9Vl6N0Oddxfj/raOrQ== Date: Tue, 22 Dec 2015 07:02:10 +0000 Message-ID: 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> <20151222024058.GE18863@yliu-dev.sh.intel.com> <20151222030432.GG18863@yliu-dev.sh.intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Cc: "Michael S. Tsirkin" , "dev@dpdk.org" , Victor Kaplansky 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 22 Dec 2015 07:02:15 -0000 On 12/22/2015 11:03 AM, Yuanhan Liu wrote:=0A= > On Tue, Dec 22, 2015 at 02:45:52AM +0000, Xie, Huawei wrote:=0A= >>>>> +static inline void __attribute__((always_inline))=0A= >>>>> +vhost_log_write(struct virtio_net *dev, uint64_t addr, uint64_t len)= =0A= >>>>> +{=0A= >>>>> + uint64_t page;=0A= >>>>> +=0A= >>>> Before we log, we need memory barrier to make sure updates are in plac= e.=0A= >>>>> + if (likely(((dev->features & (1ULL << VHOST_F_LOG_ALL)) =3D=3D 0) |= |=0A= >>>>> + !dev->log_base || !len))=0A= >>>>> + return;=0A= >>> Put a memory barrier inside set_features()?=0A= >>>=0A= >>> I see no var dependence here, why putting a barrier then? We are=0A= >>> accessing and modifying same var, doesn't the cache MESI protocol=0A= >>> will get rid of your concerns?=0A= >> This fence isn't about feature var. It is to ensure that updates to the= =0A= >> guest buffer are committed before the logging.=0A= > Oh.., I was thinking you were talking about the "dev->features" field=0A= > concurrent access and modify you mentioned from V1.=0A= >=0A= >> For IA strong memory model, compiler barrier is enough. For other weak= =0A= >> memory model, fence is required.=0A= >>>>> +=0A= >>>>> + if (unlikely(dev->log_size < ((addr + len - 1) / VHOST_LOG_PAGE / 8= )))=0A= >>>>> + return;=0A= > So that I should put a "rte_mb()" __here__?=0A= >=0A= > --yliu=0A= =0A= I find that we already have the arch dependent version of rte_smp_wmb()=0A= --huawei=0A= >>>>> +=0A= >>>>> + page =3D addr / VHOST_LOG_PAGE;=0A= >>>>> + while (page * VHOST_LOG_PAGE < addr + len) {=0A= >>>> Let us have a page_end var to make the code simpler?=0A= >>> Could do that.=0A= >>>=0A= >>>=0A= >>>>> + vhost_log_page((uint8_t *)(uintptr_t)dev->log_base, page);=0A= >>>>> + page +=3D VHOST_LOG_PAGE;=0A= >>>> page +=3D 1?=0A= >>> Oops, right.=0A= >>>=0A= >>> --yliu=0A= >>>=0A= >>>>> + }=0A= >>>>> +}=0A= >>>>> +=0A= >>>>> +=0A= >>>>> /**=0A= >>>>> * Disable features in feature_mask. Returns 0 on success.=0A= >>>>> */=0A= =0A=