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 545BAA874 for ; Wed, 2 Mar 2016 03:48:05 +0100 (CET) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga103.fm.intel.com with ESMTP; 01 Mar 2016 18:48:04 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.22,526,1449561600"; d="scan'208";a="914899622" Received: from yliu-dev.sh.intel.com (HELO yliu-dev) ([10.239.66.49]) by fmsmga001.fm.intel.com with ESMTP; 01 Mar 2016 18:48:05 -0800 Date: Wed, 2 Mar 2016 10:49:18 +0800 From: Yuanhan Liu To: "Qiu, Michael" Message-ID: <20160302024918.GQ14300@yliu-dev.sh.intel.com> References: <1456476662-23081-1-git-send-email-sshukla@mvista.com> <533710CFB86FA344BFBF2D6802E6028622F5A41C@SHSMSX101.ccr.corp.intel.com> <533710CFB86FA344BFBF2D6802E6028622F5AC3E@SHSMSX101.ccr.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <533710CFB86FA344BFBF2D6802E6028622F5AC3E@SHSMSX101.ccr.corp.intel.com> User-Agent: Mutt/1.5.23 (2014-03-12) Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH v1] virtio: Use cpuflag for vector api 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 Mar 2016 02:48:05 -0000 On Wed, Mar 02, 2016 at 02:10:14AM +0000, Qiu, Michael wrote: > On 3/1/2016 5:46 PM, Santosh Shukla wrote: > > On Tue, Mar 1, 2016 at 2:41 PM, Qiu, Michael wrote: > >> On 2/26/2016 4:53 PM, Santosh Shukla wrote: > >>> Check cpuflag macro before using vectored api. > >>> -virtio_recv_pkts_vec() uses _sse3__ simd instruction for now so added cpuflag. > >>> - Also wrap other vectored freind api ie.. > >>> 1) virtqueue_enqueue_recv_refill_simple > >>> 2) virtio_rxq_vec_setup > >>> > >>> todo: > >>> 1) Move virtio_recv_pkts_vec() implementation to > >>> drivers/virtio/virtio_vec_.h file. > >>> 2) Remove use_simple_rxtx flag, so that virtio/virtio_vec_.h > >>> files to provide vectored/non-vectored rx/tx apis. > >>> > >>> Signed-off-by: Santosh Shukla > >>> --- > >>> - v1: This is a rework of patch [1]. > >>> Note: This patch will let non-x86 arch to use virtio pmd. > >>> > >>> [1] http://dpdk.org/dev/patchwork/patch/10429/ > >>> > >>> drivers/net/virtio/virtio_rxtx.c | 16 +++++++++++++++- > >>> drivers/net/virtio/virtio_rxtx.h | 2 ++ > >>> drivers/net/virtio/virtio_rxtx_simple.c | 11 ++++++++++- > >>> 3 files changed, 27 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c > >>> index 41a1366..ec0b8de 100644 > >>> --- a/drivers/net/virtio/virtio_rxtx.c > >>> +++ b/drivers/net/virtio/virtio_rxtx.c > >>> @@ -67,7 +67,9 @@ > >>> #define VIRTIO_SIMPLE_FLAGS ((uint32_t)ETH_TXQ_FLAGS_NOMULTSEGS | \ > >>> ETH_TXQ_FLAGS_NOOFFLOADS) > >>> > >>> +#ifdef RTE_MACHINE_CPUFLAG_SSSE3 > >>> static int use_simple_rxtx; > >>> +#endif > >>> > >>> > >> I don't think so much #ifdef ... #endif in *.c file is a good choice. > >> Would you consider let it only in header file like: > >> > >> in drivers/net/virtio/virtio_rxtx.h > >> > >> [...] > >> > >> #ifdef RTE_MACHINE_CPUFLAG_SSSE3 > >> int virtio_rxq_vec_setup(struct virtqueue *rxq); > >> > >> int virtqueue_enqueue_recv_refill_simple(struct virtqueue *vq, > >> struct rte_mbuf *m); > >> #else > >> int virtio_rxq_vec_setup(__rte_unused struct virtqueue *rxq) {return -1;} > >> int virtqueue_enqueue_recv_refill_simple(__rte_unused struct virtqueue *vq, > >> __rte_unused struct rte_mbuf *m) { > >> return -1; > >> } > >> #endif > >> > >> and remove most #ifdef ... #endif in *.c file. > >> > > I guess, above approach wont work for non-x86 arch, ad those func are > > dummy, right? also code wont build for arm/non-86 arch because > > tx/rx_pkt_burst callback will be using x86 specific virtio vec rx/tx > > api. > > You may right, but you really need to reduce the #ifdef in *.c files. In general, yes. But for this case, no: those vec stuff are for platforms that support it. For other platforms, we should not invoke a dummy function like virtio_rxq_vec_setup() at all. The right way to go is to add another wrapper beyond the vector stuff, something like: virtio_rxq_setup() { if (has_vec_support) virtio_rxq_vec_setup(); else virtio_rxq_generic_setup(); } Where virtio_rxq_vec_setup() could have a per-arch implementation, say for X86, or ARM. It touchs more code, but for now, I'd like to make it simple first. With the virtio_rxtx_simple.c isolated from Makefile, there aren't many #ifdef after all. --yliu