diff mbox series

[v3] examples/vhost: fix ioat dependency issue

Message ID 20201112072147.4345-1-Cheng1.jiang@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Maxime Coquelin
Headers
Series [v3] examples/vhost: fix ioat dependency issue |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-broadcom-Performance success Performance Testing PASS
ci/Intel-compilation success Compilation OK
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/travis-robot success Travis build: passed

Commit Message

Jiang, Cheng1 Nov. 12, 2020, 7:21 a.m. UTC 
  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 <Cheng1.jiang@intel.com>
---
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(-)

--
2.29.2

Comments

David Marchand Nov. 12, 2020, 9:36 a.m. UTC | #1 
On Thu, Nov 12, 2020 at 8:30 AM Cheng Jiang <Cheng1.jiang@intel.com> 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 <Cheng1.jiang@intel.com>
> ---
> 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
> diff --git a/examples/vhost/ioat.h b/examples/vhost/ioat.h
> index 9664fcc3a..d6e1e2e07 100644
> --- a/examples/vhost/ioat.h
> +++ b/examples/vhost/ioat.h
> @@ -24,14 +24,8 @@ struct dma_for_vhost {
>         uint16_t nr;
>  };
>
> -#ifdef RTE_ARCH_X86
> +#ifdef RTE_RAW_IOAT
>  int open_ioat(const char *value);
> -#else
> -static int open_ioat(const char *value __rte_unused)
> -{
> -       return -1;
> -}
> -#endif
>
>  uint32_t
>  ioat_transfer_data_cb(int vid, uint16_t queue_id,
> @@ -42,4 +36,28 @@ uint32_t
>  ioat_check_completed_copies_cb(int vid, uint16_t queue_id,
>                 struct rte_vhost_async_status *opaque_data,
>                 uint16_t max_packets);
> +#else
> +static int open_ioat(const char *value __rte_unused)
> +{
> +       return -1;
> +}
> +
> +static uint32_t
> +ioat_transfer_data_cb(int vid __rte_unused, uint16_t queue_id __rte_unused,
> +               struct rte_vhost_async_desc *descs __rte_unused,
> +               struct rte_vhost_async_status *opaque_data __rte_unused,
> +               uint16_t count __rte_unused)
> +{
> +       return -1;
> +}
> +
> +static uint32_t
> +ioat_check_completed_copies_cb(int vid __rte_unused,
> +               uint16_t queue_id __rte_unused,
> +               struct rte_vhost_async_status *opaque_data __rte_unused,
> +               uint16_t max_packets __rte_unused)
> +{
> +       return -1;
> +}
> +#endif
>  #endif /* _IOAT_H_ */
> diff --git a/examples/vhost/main.c b/examples/vhost/main.c
> index 59a1aff07..4dc6102ab 100644
> --- a/examples/vhost/main.c
> +++ b/examples/vhost/main.c
> @@ -1294,13 +1294,6 @@ new_device(int vid)
>         int lcore, core_add = 0;
>         uint32_t device_num_min = num_devices;
>         struct vhost_dev *vdev;
> -
> -       struct rte_vhost_async_channel_ops channel_ops = {
> -               .transfer_data = ioat_transfer_data_cb,
> -               .check_completed_copies = ioat_check_completed_copies_cb
> -       };
> -       struct rte_vhost_async_features f;
> -
>         vdev = rte_zmalloc("vhost device", sizeof(*vdev), RTE_CACHE_LINE_SIZE);
>         if (vdev == NULL) {
>                 RTE_LOG(INFO, VHOST_DATA,
> @@ -1342,10 +1335,17 @@ new_device(int vid)
>                 vid, vdev->coreid);
>
>         if (async_vhost_driver) {
> -               f.async_inorder = 1;
> -               f.async_threshold = 256;
> -               return rte_vhost_async_channel_register(vid, VIRTIO_RXQ,
> -                       f.intval, &channel_ops);
> +               struct rte_vhost_async_features f;
> +               struct rte_vhost_async_channel_ops channel_ops;
> +               if (strncmp(dma_type, "ioat", 4) == 0) {
> +                       channel_ops.transfer_data = ioat_transfer_data_cb;
> +                       channel_ops.check_completed_copies =
> +                               ioat_check_completed_copies_cb;
> +                       f.async_inorder = 1;
> +                       f.async_threshold = 256;
> +                       return rte_vhost_async_channel_register(vid, VIRTIO_RXQ,
> +                               f.intval, &channel_ops);
> +               }
>         }
>
>         return 0;
> diff --git a/examples/vhost/meson.build b/examples/vhost/meson.build
> index 24f1f7131..d5388a795 100644
> --- a/examples/vhost/meson.build
> +++ b/examples/vhost/meson.build
> @@ -15,7 +15,7 @@ sources = files(
>         'main.c', 'virtio_net.c'
>  )
>
> -if dpdk_conf.has('RTE_ARCH_X86')
> +if dpdk_conf.has('RTE_RAW_IOAT')
>         deps += 'raw_ioat'
>         sources += files('ioat.c')
>  endif
> --
> 2.29.2
>

I'll let Bruce and Maxime have the last word on this patch.
But at least it works for me,
Tested-by: David Marchand <david.marchand@redhat.com>
Bruce Richardson Nov. 12, 2020, 10:28 a.m. UTC | #2 
On Thu, Nov 12, 2020 at 10:36:50AM +0100, David Marchand wrote:
> On Thu, Nov 12, 2020 at 8:30 AM Cheng Jiang <Cheng1.jiang@intel.com> 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 <Cheng1.jiang@intel.com>
> > ---
> > 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

<snip>

> I'll let Bruce and Maxime have the last word on this patch.
> But at least it works for me,
> Tested-by: David Marchand <david.marchand@redhat.com>
> 
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
Jiang, Cheng1 Nov. 12, 2020, 11:29 a.m. UTC | #3 
Hi Bruce,

> -----Original Message-----
> From: Bruce Richardson <bruce.richardson@intel.com>
> Sent: Thursday, November 12, 2020 6:28 PM
> To: David Marchand <david.marchand@redhat.com>
> Cc: Jiang, Cheng1 <cheng1.jiang@intel.com>; Maxime Coquelin
> <maxime.coquelin@redhat.com>; Xia, Chenbo <chenbo.xia@intel.com>;
> dev <dev@dpdk.org>; Fu, Patrick <patrick.fu@intel.com>; Yang, YvonneX
> <yvonnex.yang@intel.com>; Hu, Jiayu <jiayu.hu@intel.com>
> 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 <Cheng1.jiang@intel.com>
> 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 <Cheng1.jiang@intel.com>
> > > ---
> > > 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
> 
> <snip>
> 
> > I'll let Bruce and Maxime have the last word on this patch.
> > But at least it works for me,
> > Tested-by: David Marchand <david.marchand@redhat.com>
> >
> 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. 

Thanks,
Cheng
Bruce Richardson Nov. 12, 2020, 12:02 p.m. UTC | #4 
On Thu, Nov 12, 2020 at 11:29:56AM +0000, Jiang, Cheng1 wrote:
> Hi Bruce,
> 
> > -----Original Message-----
> > From: Bruce Richardson <bruce.richardson@intel.com>
> > Sent: Thursday, November 12, 2020 6:28 PM
> > To: David Marchand <david.marchand@redhat.com>
> > Cc: Jiang, Cheng1 <cheng1.jiang@intel.com>; Maxime Coquelin
> > <maxime.coquelin@redhat.com>; Xia, Chenbo <chenbo.xia@intel.com>;
> > dev <dev@dpdk.org>; Fu, Patrick <patrick.fu@intel.com>; Yang, YvonneX
> > <yvonnex.yang@intel.com>; Hu, Jiayu <jiayu.hu@intel.com>
> > 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 <Cheng1.jiang@intel.com>
> > 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 <Cheng1.jiang@intel.com>
> > > > ---
> > > > 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
> >
> > <snip>
> >
> > > I'll let Bruce and Maxime have the last word on this patch.
> > > But at least it works for me,
> > > Tested-by: David Marchand <david.marchand@redhat.com>
> > >
> > 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 */
Jiang, Cheng1 Nov. 12, 2020, 2:06 p.m. UTC | #5 
Submitted v4 patch as per Bruce's suggestion.

> -----Original Message-----
> From: Bruce Richardson <bruce.richardson@intel.com>
> Sent: Thursday, November 12, 2020 8:02 PM
> To: Jiang, Cheng1 <cheng1.jiang@intel.com>
> Cc: David Marchand <david.marchand@redhat.com>; Maxime Coquelin
> <maxime.coquelin@redhat.com>; Xia, Chenbo <chenbo.xia@intel.com>;
> dev <dev@dpdk.org>; Fu, Patrick <patrick.fu@intel.com>; Yang, YvonneX
> <yvonnex.yang@intel.com>; Hu, Jiayu <jiayu.hu@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v3] examples/vhost: fix ioat dependency
> issue
> 
> On Thu, Nov 12, 2020 at 11:29:56AM +0000, Jiang, Cheng1 wrote:
> > Hi Bruce,
> >
> > > -----Original Message-----
> > > From: Bruce Richardson <bruce.richardson@intel.com>
> > > Sent: Thursday, November 12, 2020 6:28 PM
> > > To: David Marchand <david.marchand@redhat.com>
> > > Cc: Jiang, Cheng1 <cheng1.jiang@intel.com>; Maxime Coquelin
> > > <maxime.coquelin@redhat.com>; Xia, Chenbo <chenbo.xia@intel.com>;
> > > dev <dev@dpdk.org>; Fu, Patrick <patrick.fu@intel.com>; Yang, YvonneX
> > > <yvonnex.yang@intel.com>; Hu, Jiayu <jiayu.hu@intel.com>
> > > 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 <Cheng1.jiang@intel.com>
> > > 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 <Cheng1.jiang@intel.com>
> > > > > ---
> > > > > 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
> > >
> > > <snip>
> > >
> > > > I'll let Bruce and Maxime have the last word on this patch.
> > > > But at least it works for me,
> > > > Tested-by: David Marchand <david.marchand@redhat.com>
> > > >
> > > 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 */
diff mbox series

Patch

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
diff --git a/examples/vhost/ioat.h b/examples/vhost/ioat.h
index 9664fcc3a..d6e1e2e07 100644
--- a/examples/vhost/ioat.h
+++ b/examples/vhost/ioat.h
@@ -24,14 +24,8 @@  struct dma_for_vhost {
 	uint16_t nr;
 };

-#ifdef RTE_ARCH_X86
+#ifdef RTE_RAW_IOAT
 int open_ioat(const char *value);
-#else
-static int open_ioat(const char *value __rte_unused)
-{
-	return -1;
-}
-#endif

 uint32_t
 ioat_transfer_data_cb(int vid, uint16_t queue_id,
@@ -42,4 +36,28 @@  uint32_t
 ioat_check_completed_copies_cb(int vid, uint16_t queue_id,
 		struct rte_vhost_async_status *opaque_data,
 		uint16_t max_packets);
+#else
+static int open_ioat(const char *value __rte_unused)
+{
+	return -1;
+}
+
+static uint32_t
+ioat_transfer_data_cb(int vid __rte_unused, uint16_t queue_id __rte_unused,
+		struct rte_vhost_async_desc *descs __rte_unused,
+		struct rte_vhost_async_status *opaque_data __rte_unused,
+		uint16_t count __rte_unused)
+{
+	return -1;
+}
+
+static uint32_t
+ioat_check_completed_copies_cb(int vid __rte_unused,
+		uint16_t queue_id __rte_unused,
+		struct rte_vhost_async_status *opaque_data __rte_unused,
+		uint16_t max_packets __rte_unused)
+{
+	return -1;
+}
+#endif
 #endif /* _IOAT_H_ */
diff --git a/examples/vhost/main.c b/examples/vhost/main.c
index 59a1aff07..4dc6102ab 100644
--- a/examples/vhost/main.c
+++ b/examples/vhost/main.c
@@ -1294,13 +1294,6 @@  new_device(int vid)
 	int lcore, core_add = 0;
 	uint32_t device_num_min = num_devices;
 	struct vhost_dev *vdev;
-
-	struct rte_vhost_async_channel_ops channel_ops = {
-		.transfer_data = ioat_transfer_data_cb,
-		.check_completed_copies = ioat_check_completed_copies_cb
-	};
-	struct rte_vhost_async_features f;
-
 	vdev = rte_zmalloc("vhost device", sizeof(*vdev), RTE_CACHE_LINE_SIZE);
 	if (vdev == NULL) {
 		RTE_LOG(INFO, VHOST_DATA,
@@ -1342,10 +1335,17 @@  new_device(int vid)
 		vid, vdev->coreid);

 	if (async_vhost_driver) {
-		f.async_inorder = 1;
-		f.async_threshold = 256;
-		return rte_vhost_async_channel_register(vid, VIRTIO_RXQ,
-			f.intval, &channel_ops);
+		struct rte_vhost_async_features f;
+		struct rte_vhost_async_channel_ops channel_ops;
+		if (strncmp(dma_type, "ioat", 4) == 0) {
+			channel_ops.transfer_data = ioat_transfer_data_cb;
+			channel_ops.check_completed_copies =
+				ioat_check_completed_copies_cb;
+			f.async_inorder = 1;
+			f.async_threshold = 256;
+			return rte_vhost_async_channel_register(vid, VIRTIO_RXQ,
+				f.intval, &channel_ops);
+		}
 	}

 	return 0;
diff --git a/examples/vhost/meson.build b/examples/vhost/meson.build
index 24f1f7131..d5388a795 100644
--- a/examples/vhost/meson.build
+++ b/examples/vhost/meson.build
@@ -15,7 +15,7 @@  sources = files(
 	'main.c', 'virtio_net.c'
 )

-if dpdk_conf.has('RTE_ARCH_X86')
+if dpdk_conf.has('RTE_RAW_IOAT')
 	deps += 'raw_ioat'
 	sources += files('ioat.c')
 endif