From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id BB2F7376C for ; Thu, 3 Dec 2015 08:21:28 +0100 (CET) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga103.fm.intel.com with ESMTP; 02 Dec 2015 23:21:27 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,376,1444719600"; d="scan'208";a="833271752" Received: from yliu-dev.sh.intel.com (HELO yliu-dev) ([10.239.66.49]) by orsmga001.jf.intel.com with ESMTP; 02 Dec 2015 23:21:26 -0800 Date: Thu, 3 Dec 2015 15:25:01 +0800 From: Yuanhan Liu To: Stephen Hemminger Message-ID: <20151203072501.GA2325@yliu-dev.sh.intel.com> References: <1449122773-25510-1-git-send-email-yuanhan.liu@linux.intel.com> <1449122773-25510-2-git-send-email-yuanhan.liu@linux.intel.com> <20151202230244.5e4ca3fb@xeon-e3> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20151202230244.5e4ca3fb@xeon-e3> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: dev@dpdk.org, Victor Kaplansky , "Michael S. Tsirkin" Subject: Re: [dpdk-dev] [PATCH 1/5] vhost: refactor rte_vhost_dequeue_burst 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: Thu, 03 Dec 2015 07:21:29 -0000 On Wed, Dec 02, 2015 at 11:02:44PM -0800, Stephen Hemminger wrote: > On Thu, 3 Dec 2015 14:06:09 +0800 > Yuanhan Liu wrote: > > > +#define COPY(dst, src) do { \ > > + cpy_len = RTE_MIN(desc_avail, mbuf_avail); \ > > + rte_memcpy((void *)(uintptr_t)(dst), \ > > + (const void *)(uintptr_t)(src), cpy_len); \ > > + \ > > + mbuf_avail -= cpy_len; \ > > + mbuf_offset += cpy_len; \ > > + desc_avail -= cpy_len; \ > > + desc_offset += cpy_len; \ > > +} while(0) > > + > > I see lots of issues here. > > All those void * casts are unnecessary, C casts arguements already. Hi Stephen, Without the cast, the compile will not work, as dst is actully with uint64_t type. > rte_memcpy is slower for constant size values than memcpy() Sorry, what does it have something to do with this patch? > This macro violates the rule that ther should be no hidden variables > in a macro. I.e you are assuming cpy_len, desc_avail, and mbuf_avail > are defined in all code using the macro. Yes, I'm aware of that. And I agree that it's not a good usage _in general_. But I'm thinking that it's okay to use it here, for the three major functions has quite similar logic. And if my memory servers me right, there are also quite many codes like that in Linux kernel. > Why use an un-typed macro when an inline function would be just > as fast and give type safety? It references too many variables, upto 6. And there are 4 vars needed to be updated. Therefore, making it to be a function would make the caller long and ugly. That's why I was thinking to use a macro to remove few lines of repeat code. So, if hidden var macro is forbidden here, I guess I would go with just unfolding those lines of code, but not introducing a helper function. --yliu