From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id C9258A09D3; Thu, 12 Nov 2020 13:02:15 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 2FFFC5928; Thu, 12 Nov 2020 13:02:14 +0100 (CET) Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by dpdk.org (Postfix) with ESMTP id 3568E56A3 for ; Thu, 12 Nov 2020 13:02:12 +0100 (CET) IronPort-SDR: Zi2n+evxNPHv1Yc/PHRF7h9q789clXpmruH3FF3E+e2M0aYMHm0BGFdOryuoyYEDFDtISkIMCz IXblthCi7vdQ== X-IronPort-AV: E=McAfee;i="6000,8403,9802"; a="234460089" X-IronPort-AV: E=Sophos;i="5.77,471,1596524400"; d="scan'208";a="234460089" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga105.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 Nov 2020 04:02:09 -0800 IronPort-SDR: Bylf4rM8UxRaN9yWpVhH6MH4LgadFPwMRla5/6fO+z4lTg5benYTuA2V037F4Pz7XfIn0xRbUX zdIzm8HJAJCw== X-IronPort-AV: E=Sophos;i="5.77,471,1596524400"; d="scan'208";a="542235406" Received: from bricha3-mobl.ger.corp.intel.com ([10.213.208.146]) by orsmga005-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 12 Nov 2020 04:02:07 -0800 Date: Thu, 12 Nov 2020 12:02:04 +0000 From: Bruce Richardson To: "Jiang, Cheng1" Cc: David Marchand , Maxime Coquelin , "Xia, Chenbo" , dev , "Fu, Patrick" , "Yang, YvonneX" , "Hu, Jiayu" Message-ID: <20201112120204.GH1309@bricha3-MOBL.ger.corp.intel.com> References: <20201111111957.46090-1-Cheng1.jiang@intel.com> <20201112072147.4345-1-Cheng1.jiang@intel.com> <20201112102810.GE1309@bricha3-MOBL.ger.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [dpdk-dev] [PATCH v3] examples/vhost: fix ioat dependency issue 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Thu, Nov 12, 2020 at 11:29:56AM +0000, Jiang, Cheng1 wrote: > Hi Bruce, > > > -----Original Message----- > > From: Bruce Richardson > > Sent: Thursday, November 12, 2020 6:28 PM > > To: David Marchand > > Cc: Jiang, Cheng1 ; Maxime Coquelin > > ; Xia, Chenbo ; > > dev ; Fu, Patrick ; Yang, YvonneX > > ; Hu, Jiayu > > Subject: Re: [dpdk-dev] [PATCH v3] examples/vhost: fix ioat dependency > > issue > > > > On Thu, Nov 12, 2020 at 10:36:50AM +0100, David Marchand wrote: > > > On Thu, Nov 12, 2020 at 8:30 AM Cheng Jiang > > wrote: > > > > > > > > Fix vhost-switch compiling issue when ioat dependency is missing. > > > > Change 'RTE_x86' check into 'RTE_RAW_IOAT' check in meson build file > > > > and update Makefile. Clean some codes. > > > > > > > > Fixes: abec60e7115d ("examples/vhost: support vhost async data > > > > path") > > > > Fixes: 3a04ecb21420 ("examples/vhost: add async vhost args parsing") > > > > > > > > Signed-off-by: Cheng Jiang > > > > --- > > > > v3: > > > > * Added fixes lines in commit log. > > > > > > > > v2: > > > > * Cleaned some codes > > > > * Changed RTE_RAW_IOAT check method in Makefile > > > > * Added ioat function definition when RTE_RAW_IOAT is missing > > > > > > > > examples/vhost/Makefile | 5 +++++ > > > > examples/vhost/ioat.h | 32 +++++++++++++++++++++++++------- > > > > examples/vhost/main.c | 22 +++++++++++----------- > > > > examples/vhost/meson.build | 2 +- > > > > 4 files changed, 42 insertions(+), 19 deletions(-) > > > > > > > > diff --git a/examples/vhost/Makefile b/examples/vhost/Makefile index > > > > cec59d0e0..cbe56f742 100644 > > > > --- a/examples/vhost/Makefile > > > > +++ b/examples/vhost/Makefile > > > > @@ -28,6 +28,11 @@ CFLAGS += -O3 $(shell $(PKGCONF) --cflags > > > > libdpdk) LDFLAGS_SHARED = $(shell $(PKGCONF) --libs libdpdk) > > > > LDFLAGS_STATIC = $(shell $(PKGCONF) --static --libs libdpdk) > > > > > > > > +HAS_RAW_IOAT=$(shell echo RTE_RAW_IOAT | $(CPP) $(CFLAGS) -P - | > > > > +tail -1) ifeq ($(HAS_RAW_IOAT), 1) SRCS-y += ioat.c endif > > > > + > > > > CFLAGS += -DALLOW_EXPERIMENTAL_API > > > > > > > > build/$(APP)-shared: $(SRCS-y) Makefile $(PC_FILE) | build > > > > > > > > > I'll let Bruce and Maxime have the last word on this patch. > > > But at least it works for me, > > > Tested-by: David Marchand > > > > > I don't have a really strong objection to this, but I still would rather see the > > ioat detection done in the C code only rather than in the Makefile. > > I'm concerned about us adding too much complexity into our makefiles, so > > would like to keep them simple as much as possible. > > > > Therefore, I'd like to see ioat.c always included in SRCS-y, and ioat.c just have > > #ifdef RTE_RAW_IOAT to block out any ioat-dependent code. > > > > /Bruce > > Tomorrow is the deadline for RC4, it's a little bit rush to prepare a new framework to cover the conditional compiling. Considering we have test efforts and CI check already on-going based on current patch, I would prefer to keep current code structure. Plus, pure C code change could also introduce more readability problem when we have more than one async callback implementations. > I don't see any readability problems at all, in fact the result is cleaner and shorter IMHO - especially the Makefile. I just tested with the following changes applied on top of your patch, and all seemed to build ok. Do you see any implementation problems with the below solution? /Bruce diff --git a/examples/vhost/Makefile b/examples/vhost/Makefile index cbe56f742..8c969caaa 100644 --- a/examples/vhost/Makefile +++ b/examples/vhost/Makefile @@ -5,7 +5,7 @@ APP = vhost-switch # all source are stored in SRCS-y -SRCS-y := main.c virtio_net.c +SRCS-y := main.c virtio_net.c ioat.c # Build using pkg-config variables if possible ifneq ($(shell pkg-config --exists libdpdk && echo 0),0) @@ -28,11 +28,6 @@ CFLAGS += -O3 $(shell $(PKGCONF) --cflags libdpdk) LDFLAGS_SHARED = $(shell $(PKGCONF) --libs libdpdk) LDFLAGS_STATIC = $(shell $(PKGCONF) --static --libs libdpdk) -HAS_RAW_IOAT=$(shell echo RTE_RAW_IOAT | $(CPP) $(CFLAGS) -P - | tail -1) -ifeq ($(HAS_RAW_IOAT), 1) -SRCS-y += ioat.c -endif - CFLAGS += -DALLOW_EXPERIMENTAL_API build/$(APP)-shared: $(SRCS-y) Makefile $(PC_FILE) | build diff --git a/examples/vhost/ioat.c b/examples/vhost/ioat.c index b2c74f653..7c1a4f40d 100644 --- a/examples/vhost/ioat.c +++ b/examples/vhost/ioat.c @@ -8,6 +8,8 @@ #include "ioat.h" #include "main.h" +#ifdef RTE_RAW_IOAT + struct dma_for_vhost dma_bind[MAX_VHOST_DEVICE]; struct packet_tracker { @@ -199,3 +201,5 @@ ioat_check_completed_copies_cb(int vid, uint16_t queue_id, /* Opaque data is not supported */ return -1; } + +#endif /* RTE_RAW_IOAT */