Re: [PATCH v3 1/1] app/testpmd: add GPU memory option for mbuf pools

[Elena Agostini <eagostini@nvidia.com>]

[-- Attachment #1: Type: text/plain, Size: 8585 bytes --]

> > On Wed, Nov 17, 2021 at 7:20 PM Elena Agostini <eagostini@nvidia.com> wrote:
> > >
> > > > On Wed, Nov 17, 2021 at 6:09 PM Elena Agostini <eagostini@nvidia.com> wrote:
> > >
> > > > >
> > >
> > > > > > > >>
> > >
> > > > >
> > >
> > > > > > > >>>>
> > >
> > > > >
> > >
> > > > > > > >>
> > >
> > > > >
> > >
> > > > > > > >>>>> On Wed, 17 Nov 2021 03:04:59 +0000
> > >
> > > > >
> > >
> > > > > > > >>
> > >
> > > > >
> > >
> > > > > > > >>>>
> > >
> > > > >
> > >
> > > > > > > >>>>>> This patch introduces GPU memory in testpmd through the gpudev library.
> > >
> > > > >
> > >
> > > > > > > >>
> > >
> > > > >
> > >
> > > > > > > >>>>
> > >
> > > > >
> > >
> > > > > > > >>
> > >
> > > > >
> > >
> > > > > > > >>>>>> Testpmd can be used for network benchmarks when using GPU memory
> > >
> > > > >
> > >
> > > > > > > >>
> > >
> > > > >
> > >
> > > > > > > >>>>
> > >
> > > > >
> > >
> > > > > > > >>
> > >
> > > > >
> > >
> > > > > > > >>>>>> instead of regular CPU memory to send and receive packets.
> > >
> > > > >
> > >
> > > > > > > >>
> > >
> > > > >
> > >
> > > > > > > >>>>
> > >
> > > > >
> > >
> > > > > > > >>
> > >
> > > > >
> > >
> > > > > > > >>>>>> This option is currently limited to iofwd engine to ensure
> > >
> > > > >
> > >
> > > > > > > >>
> > >
> > > > >
> > >
> > > > > > > >>>>
> > >
> > > > >
> > >
> > > > > > > >>
> > >
> > > > >
> > >
> > > > > > > >>>>>> no workload is applied on packets not accessible from the CPU.
> > >
> > > > >
> > >
> > > > > > > >>
> > >
> > > > >
> > >
> > > > > > > >>>>
> > >
> > > > >
> > >
> > > > > > > >>
> > >
> > > > >
> > >
> > > > > > > >>>>>>
> > >
> > > > >
> > >
> > > > > > > >>
> > >
> > > > >
> > >
> > > > > > > >>>>
> > >
> > > > >
> > >
> > > > > > > >>
> > >
> > > > >
> > >
> > > > > > > >>>>>> The options chose is --mbuf-size so buffer split feature across
> > >
> > > > >
> > >
> > > > > > > >>
> > >
> > > > >
> > >
> > > > > > > >>>>
> > >
> > > > >
> > >
> > > > > > > >>
> > >
> > > > >
> > >
> > > > > > > >>>>>> different mempools can be enabled.
> > >
> > > > >
> > >
> > > > > > > >>
> > >
> > > > >
> > >
> > > > > > > >>>>
> > >
> > > > >
> > >
> > > > > > > >>
> > >
> > > > >
> > >
> > > > > > > >>>>>>
> > >
> > > > >
> > >
> > > > > > > >>
> > >
> > > > >
> > >
> > > > > > > >>>>
> > >
> > > > >
> > >
> > > > > > > >>
> > >
> > > > >
> > >
> > > > > > > >>>>>> Signed-off-by: Elena Agostini <eagostini@nvidia.com>
> > >
> > > > >
> > >
> > > > > > > >>
> > >
> > > > >
> > >
> > > > > > > >>>>
> > >
> > > > >
> > >
> > > > > > > >>
> > >
> > > > >
> > >
> > > > > > > >>>>>
> > >
> > > > >
> > >
> > > > > > > >>
> > >
> > > > >
> > >
> > > > > > > >>>>
> > >
> > > > >
> > >
> > > > > > > >>
> > >
> > > > >
> > >
> > > > > > > >>>>> Won't this create a hard dependency of test-pmd on gpudev?
> > >
> > > > >
> > >
> > > > > > > >>
> > >
> > > > >
> > >
> > > > > > > >>>>
> > >
> > > > >
> > >
> > > > > > > >>
> > >
> > > > >
> > >
> > > > > > > >>>>> I thought gpudev was supposed to be optional
> > >
> > > > >
> > >
> > > > > > > >>
> > >
> > > > >
> > >
> > > > > > > >>>>
> > >
> > > > >
> > >
> > > > > > > >>
> > >
> > > > >
> > >
> > > > > > > >>>>
> > >
> > > > >
> > >
> > > > > > > >>
> > >
> > > > >
> > >
> > > > > > > >>>>
> > >
> > > > >
> > >
> > > > > > > >>
> > >
> > > > >
> > >
> > > > > > > >>>> Sure, let me submit another patch to make it optional
> > >
> > > > >
> > >
> > > > > > > >>
> > >
> > > > >
> > >
> > > > > > > >>>
> > >
> > > > >
> > >
> > > > > > > >>
> > >
> > > > >
> > >
> > > > > > > >>> Why to add yet another compile time macro everywhere in testpmd and
> > >
> > > > >
> > >
> > > > > > > >>
> > >
> > > > >
> > >
> > > > > > > >>> make hard to maintain?
> > >
> > > > >
> > >
> > > > > > > >>
> > >
> > > > >
> > >
> > > > > > > >>> Adding iofwd kind of code is very simple to add test/test-gpudev and
> > >
> > > > >
> > >
> > > > > > > >>
> > >
> > > > >
> > >
> > > > > > > >>> all GPU specific options
> > >
> > > > >
> > >
> > > > > > > >>
> > >
> > > > >
> > >
> > > > > > > >>> can be added in test-gpudev. It also helps to review the patches as
> > >
> > > > >
> > >
> > > > > > > >>
> > >
> > > > >
> > >
> > > > > > > >>> test cases focus on
> > >
> > > > >
> > >
> > > > > > > >>
> > >
> > > > >
> > >
> > > > > > > >>> each device class.
> > >
> > > > >
> > >
> > > > > > > >>
> > >
> > > > >
> > >
> > > > > > > >>
> > >
> > > > >
> > >
> > > > > > > >>
> > >
> > > > >
> > >
> > > > > > > >> Test-gpudev is standalone unit test to ensure gpudev functions work correctly.
> > >
> > > > >
> > >
> > > > > > > >>
> > >
> > > > >
> > >
> > > > > > > >> In testpmd instead, there is a connection between gpudev and the network.
> > >
> > > > >
> > >
> > > > > > > >
> > >
> > > > >
> > >
> > > > > > > > I understand that. We had the same case with eventdev, where it needs to
> > >
> > > > >
> > >
> > > > > > > > work with network. Testpmd is already complicated, IMO, we should
> > >
> > > > >
> > >
> > > > > > > > focus only ethdev
> > >
> > > > >
> > >
> > > > > > > > test cases on testpmd, test-gpudev can use ethdev API to enable
> > >
> > > > >
> > >
> > > > > > > > networking requirements for gpudev.
> > >
> > > > >
> > >
> > > > > > > >
> > >
> > > > >
> > >
> > > > > > >
> > >
> > > > >
> > >
> > > > > > > +1
> > >
> > > > >
> > >
> > > > > >
> > >
> > > > >
> > >
> > > > > > +1
> > >
> > > > >
> > >
> > > > >
> > >
> > > > >
> > >
> > > > > Testpmd already manages different type of memories for mempools.
> > >
> > > > >
> > >
> > > > > gpudev is just another type of memory, there is nothing more than that.
> > >
> > > >
> > >
> > > > Let take this example:
> > >
> > > > 1) New code changes
> > >
> > > >
> > >
> > >  > app/test-pmd/cmdline.c    |  32 +++++++-
> > >
> > > > app/test-pmd/config.c     |   4 +-
> > >
> > > > app/test-pmd/icmpecho.c   |   2 +-
> > >
> > > > app/test-pmd/meson.build  |   2 +-
> > >
> > > > app/test-pmd/parameters.c |  15 +++-
> > >
> > > > app/test-pmd/testpmd.c    | 167 +++++++++++++++++++++++++++++++++++---
> > >
> > > > app/test-pmd/testpmd.h    |  16 +++-
> > >
> > > > 7 files changed, 217 insertions(+), 21 deletions(-)
> > >
> > > >
> > >
> > > > 2) Good amount of code need to go through condition compilation as
> > >
> > > > gpudev is optional that make
> > >
> > > > testpmd further ugly.
> > >
> > > >
> > >
> > > > 3) It introduces new memtype, now
> > >
> > > >
> > >
> > > > +enum mbuf_mem_type {
> > >
> > > > + MBUF_MEM_CPU,
> > >
> > > > + MBUF_MEM_GPU
> > >
> > > > +};
> > >
> > > >
> > >
> > > > The question largely, why testpmd need to pollute for this, testpmd,
> > >
> > > > we are using for testing ethdev device class.
> > >
> > > > All we are saying is to enable this use case in test-gpudev so that it
> > >
> > > > focuses on GPU specific, Whoever is not
> > >
> > > > interested in specific libraries do not even need to review the testpmd patches.
> > >
> > >
> > >
> > > I understand your point. I don’t understand why this testpmd patch is there since Oct 29 but
> > >
> > I'm receiving reviews only few days before rc4 when I have a limited amount of time to get new code > accepted.>
>
> > I understand that pain. Welcome to DPDK, we have all gone through this
> > review issue one or another way.>
> >
>
> > >
> > >
> > >
> > I can provide a gpudev + ethdev example by end of today (I'd like to keep test-gpudev as it is to > test gpudev API standalone).
> > >
> > > Is there any chance this new example will be reviewed and eventually accepted in DPDK 21.11?>
>
> > Why a new example? I don't have any issues in updating app/test-gpudev/.
>
> Understood. If this can also help to speed up the acceptance process, I’ll provide a new patch for app/> test-gpudev to introduce ethdev to send and receive packets

This is the patch to introduce ethdev in app/test-gpudev as alternative to testpmd patch, please give me your feedback.

https://patches.dpdk.org/project/dpdk/patch/20211118015228.30628-2-eagostini@nvidia.com

It would be really helpful for the gpudev library to have a concrete example to send and receive packets using GPU memory

[-- Attachment #2: Type: text/html, Size: 52981 bytes --]