diff mbox series

config/x86: add support for AMD platform

Message ID 20211102145253.413467-1-aman.kumar@vvdntech.in (mailing list archive)
State New
Delegated to: Thomas Monjalon
Headers show
Series config/x86: add support for AMD platform | expand

Checks

Context Check Description
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/intel-Testing success Testing PASS
ci/Intel-compilation success Compilation OK
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-spell-check-testing warning Testing issues
ci/github-robot: build success github build: passed
ci/checkpatch success coding style OK

Commit Message

Aman Kumar Nov. 2, 2021, 2:52 p.m. UTC
-Dcpu_instruction_set=znverX meson option can be used
to build dpdk for AMD platforms. Supported options are
znver1, znver2 and znver3.

Signed-off-by: Aman Kumar <aman.kumar@vvdntech.in>
---
 config/x86/meson.build              | 9 +++++++++
 doc/guides/linux_gsg/build_dpdk.rst | 2 +-
 2 files changed, 10 insertions(+), 1 deletion(-)

Comments

Thomas Monjalon Nov. 2, 2021, 3:18 p.m. UTC | #1
02/11/2021 15:52, Aman Kumar:
> -Dcpu_instruction_set=znverX meson option can be used
> to build dpdk for AMD platforms. Supported options are
> znver1, znver2 and znver3.
> 
> Signed-off-by: Aman Kumar <aman.kumar@vvdntech.in>
> ---
> +# AMD platform support
> +if get_option('cpu_instruction_set') == 'znver1'
> +    dpdk_conf.set('RTE_MAX_LCORE', 256)
> +elif get_option('cpu_instruction_set') == 'znver2'
> +    dpdk_conf.set('RTE_MAX_LCORE', 512)
> +elif get_option('cpu_instruction_set') == 'znver3'
> +    dpdk_conf.set('RTE_MAX_LCORE', 512)
> +endif

Ideally we should try getting rid of this config
and make all structs depending on max lcores allocated dynamically.
I think it may be an interesting target for next year 22.11 release.

About the patch itself, I am OK to merge it in 21.11-rc2.
Song, Keesang Nov. 2, 2021, 3:56 p.m. UTC | #2
[AMD Official Use Only]

It would be great to make max lcores dynamically allocated.
Thanks so much Thomas for your confirmation that this is to be merge in 21.11-rc2.

Best,
Keesang

-----Original Message-----
From: Thomas Monjalon <thomas@monjalon.net>
Sent: Tuesday, November 2, 2021 8:19 AM
To: Aman Kumar <aman.kumar@vvdntech.in>
Cc: dev@dpdk.org; Song, Keesang <Keesang.Song@amd.com>; david.marchand@redhat.com
Subject: Re: [PATCH] config/x86: add support for AMD platform

[CAUTION: External Email]

02/11/2021 15:52, Aman Kumar:
> -Dcpu_instruction_set=znverX meson option can be used to build dpdk
> for AMD platforms. Supported options are znver1, znver2 and znver3.
>
> Signed-off-by: Aman Kumar <aman.kumar@vvdntech.in>
> ---
> +# AMD platform support
> +if get_option('cpu_instruction_set') == 'znver1'
> +    dpdk_conf.set('RTE_MAX_LCORE', 256) elif
> +get_option('cpu_instruction_set') == 'znver2'
> +    dpdk_conf.set('RTE_MAX_LCORE', 512) elif
> +get_option('cpu_instruction_set') == 'znver3'
> +    dpdk_conf.set('RTE_MAX_LCORE', 512) endif

Ideally we should try getting rid of this config and make all structs depending on max lcores allocated dynamically.
I think it may be an interesting target for next year 22.11 release.

About the patch itself, I am OK to merge it in 21.11-rc2.
David Marchand Nov. 2, 2021, 6:45 p.m. UTC | #3
On Tue, Nov 2, 2021 at 3:53 PM Aman Kumar <aman.kumar@vvdntech.in> wrote:
>
> -Dcpu_instruction_set=znverX meson option can be used
> to build dpdk for AMD platforms. Supported options are
> znver1, znver2 and znver3.
>
> Signed-off-by: Aman Kumar <aman.kumar@vvdntech.in>
> ---
>  config/x86/meson.build              | 9 +++++++++
>  doc/guides/linux_gsg/build_dpdk.rst | 2 +-
>  2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/config/x86/meson.build b/config/x86/meson.build
> index 29f3dea181..21cda6fd33 100644
> --- a/config/x86/meson.build
> +++ b/config/x86/meson.build
> @@ -72,3 +72,12 @@ endif
>  dpdk_conf.set('RTE_CACHE_LINE_SIZE', 64)
>  dpdk_conf.set('RTE_MAX_LCORE', 128)
>  dpdk_conf.set('RTE_MAX_NUMA_NODES', 32)
> +
> +# AMD platform support
> +if get_option('cpu_instruction_set') == 'znver1'
> +    dpdk_conf.set('RTE_MAX_LCORE', 256)
> +elif get_option('cpu_instruction_set') == 'znver2'
> +    dpdk_conf.set('RTE_MAX_LCORE', 512)
> +elif get_option('cpu_instruction_set') == 'znver3'
> +    dpdk_conf.set('RTE_MAX_LCORE', 512)
> +endif

I already replied to a similar patch earlier in this release.
https://inbox.dpdk.org/dev/CAJFAV8z-5amvEnr3mazkTqH-7SZX_C6EqCua6UdMXXHgrcmT6g@mail.gmail.com/

So repeating the same: do you actually _need_ more than 128 lcores in
a single DPDK application?
Thomas Monjalon Nov. 2, 2021, 7:04 p.m. UTC | #4
02/11/2021 19:45, David Marchand:
> On Tue, Nov 2, 2021 at 3:53 PM Aman Kumar <aman.kumar@vvdntech.in> wrote:
> >
> > -Dcpu_instruction_set=znverX meson option can be used
> > to build dpdk for AMD platforms. Supported options are
> > znver1, znver2 and znver3.
> >
> > Signed-off-by: Aman Kumar <aman.kumar@vvdntech.in>
> > ---
> >  config/x86/meson.build              | 9 +++++++++
> >  doc/guides/linux_gsg/build_dpdk.rst | 2 +-
> >  2 files changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/config/x86/meson.build b/config/x86/meson.build
> > index 29f3dea181..21cda6fd33 100644
> > --- a/config/x86/meson.build
> > +++ b/config/x86/meson.build
> > @@ -72,3 +72,12 @@ endif
> >  dpdk_conf.set('RTE_CACHE_LINE_SIZE', 64)
> >  dpdk_conf.set('RTE_MAX_LCORE', 128)
> >  dpdk_conf.set('RTE_MAX_NUMA_NODES', 32)
> > +
> > +# AMD platform support
> > +if get_option('cpu_instruction_set') == 'znver1'
> > +    dpdk_conf.set('RTE_MAX_LCORE', 256)
> > +elif get_option('cpu_instruction_set') == 'znver2'
> > +    dpdk_conf.set('RTE_MAX_LCORE', 512)
> > +elif get_option('cpu_instruction_set') == 'znver3'
> > +    dpdk_conf.set('RTE_MAX_LCORE', 512)
> > +endif
> 
> I already replied to a similar patch earlier in this release.
> https://inbox.dpdk.org/dev/CAJFAV8z-5amvEnr3mazkTqH-7SZX_C6EqCua6UdMXXHgrcmT6g@mail.gmail.com/
> 
> So repeating the same: do you actually _need_ more than 128 lcores in
> a single DPDK application?

Yes I forgot this previous discussion concluding that we should not increase
more than 128 threads.

The --lcores syntax and David's work on rte_thread_register should unblock
most of use cases.
Thomas Monjalon Nov. 18, 2021, 12:25 p.m. UTC | #5
I request a techboard decision for this patch.


02/11/2021 20:04, Thomas Monjalon:
> 02/11/2021 19:45, David Marchand:
> > On Tue, Nov 2, 2021 at 3:53 PM Aman Kumar <aman.kumar@vvdntech.in> wrote:
> > >
> > > -Dcpu_instruction_set=znverX meson option can be used
> > > to build dpdk for AMD platforms. Supported options are
> > > znver1, znver2 and znver3.
> > >
> > > Signed-off-by: Aman Kumar <aman.kumar@vvdntech.in>
> > > ---
> > >  dpdk_conf.set('RTE_CACHE_LINE_SIZE', 64)
> > >  dpdk_conf.set('RTE_MAX_LCORE', 128)
> > >  dpdk_conf.set('RTE_MAX_NUMA_NODES', 32)
> > > +
> > > +# AMD platform support
> > > +if get_option('cpu_instruction_set') == 'znver1'
> > > +    dpdk_conf.set('RTE_MAX_LCORE', 256)
> > > +elif get_option('cpu_instruction_set') == 'znver2'
> > > +    dpdk_conf.set('RTE_MAX_LCORE', 512)
> > > +elif get_option('cpu_instruction_set') == 'znver3'
> > > +    dpdk_conf.set('RTE_MAX_LCORE', 512)
> > > +endif
> > 
> > I already replied to a similar patch earlier in this release.
> > https://inbox.dpdk.org/dev/CAJFAV8z-5amvEnr3mazkTqH-7SZX_C6EqCua6UdMXXHgrcmT6g@mail.gmail.com/
> > 
> > So repeating the same: do you actually _need_ more than 128 lcores in
> > a single DPDK application?

We did not receive an answer to this question.

> Yes I forgot this previous discussion concluding that we should not increase
> more than 128 threads.

We had a discussion yesterday in techboard meeting.
The consensus is that we didn't hear for real need of more than 128 threads,
except for configuration usability convenience.

Now looking again at the code, this is how it is defined:

option('max_lcores', type: 'string', value: 'default', description:
       'Set maximum number of cores/threads supported by EAL;
       "default" is different per-arch, "detect" detects the number of cores on the build machine.')
config/x86/meson.build: dpdk_conf.set('RTE_MAX_LCORE', 128)
config/ppc/meson.build: dpdk_conf.set('RTE_MAX_LCORE', 128)
config/arm/meson.build: it goes from 4 to 1280!

So I feel it is not fair to reject this AMD patch if we allow Arm to go beyond.
Techboard, let's have a quick decision please for 21.11-rc4.


> The --lcores syntax and David's work on rte_thread_register should unblock
> most of use cases.
Bruce Richardson Nov. 18, 2021, 1:52 p.m. UTC | #6
On Thu, Nov 18, 2021 at 01:25:38PM +0100, Thomas Monjalon wrote:
> I request a techboard decision for this patch.
> 
> 
> 02/11/2021 20:04, Thomas Monjalon:
> > 02/11/2021 19:45, David Marchand:
> > > On Tue, Nov 2, 2021 at 3:53 PM Aman Kumar <aman.kumar@vvdntech.in> wrote:
> > > >
> > > > -Dcpu_instruction_set=znverX meson option can be used
> > > > to build dpdk for AMD platforms. Supported options are
> > > > znver1, znver2 and znver3.
> > > >
> > > > Signed-off-by: Aman Kumar <aman.kumar@vvdntech.in>
> > > > ---
> > > >  dpdk_conf.set('RTE_CACHE_LINE_SIZE', 64)
> > > >  dpdk_conf.set('RTE_MAX_LCORE', 128)
> > > >  dpdk_conf.set('RTE_MAX_NUMA_NODES', 32)
> > > > +
> > > > +# AMD platform support
> > > > +if get_option('cpu_instruction_set') == 'znver1'
> > > > +    dpdk_conf.set('RTE_MAX_LCORE', 256)
> > > > +elif get_option('cpu_instruction_set') == 'znver2'
> > > > +    dpdk_conf.set('RTE_MAX_LCORE', 512)
> > > > +elif get_option('cpu_instruction_set') == 'znver3'
> > > > +    dpdk_conf.set('RTE_MAX_LCORE', 512)
> > > > +endif
> > > 
> > > I already replied to a similar patch earlier in this release.
> > > https://inbox.dpdk.org/dev/CAJFAV8z-5amvEnr3mazkTqH-7SZX_C6EqCua6UdMXXHgrcmT6g@mail.gmail.com/
> > > 
> > > So repeating the same: do you actually _need_ more than 128 lcores in
> > > a single DPDK application?
> 
> We did not receive an answer to this question.
> 
> > Yes I forgot this previous discussion concluding that we should not increase
> > more than 128 threads.
> 
> We had a discussion yesterday in techboard meeting.
> The consensus is that we didn't hear for real need of more than 128 threads,
> except for configuration usability convenience.
> 
> Now looking again at the code, this is how it is defined:
> 
> option('max_lcores', type: 'string', value: 'default', description:
>        'Set maximum number of cores/threads supported by EAL;
>        "default" is different per-arch, "detect" detects the number of cores on the build machine.')
> config/x86/meson.build: dpdk_conf.set('RTE_MAX_LCORE', 128)
> config/ppc/meson.build: dpdk_conf.set('RTE_MAX_LCORE', 128)
> config/arm/meson.build: it goes from 4 to 1280!
> 
> So I feel it is not fair to reject this AMD patch if we allow Arm to go beyond.
> Techboard, let's have a quick decision please for 21.11-rc4.
> 
I would support increasing the default value for x86 in this release.

I believe Dave H. had some patches to decrease the memory footprint
overhead of such a change. I don't believe that they were merged, and while
it's a bit late for 21.11 now, those should be considered for 22.03 release
and then maybe for backport.

/Bruce
Thomas Monjalon Nov. 18, 2021, 2:05 p.m. UTC | #7
18/11/2021 14:52, Bruce Richardson:
> On Thu, Nov 18, 2021 at 01:25:38PM +0100, Thomas Monjalon wrote:
> > I request a techboard decision for this patch.
> > 
> > 
> > 02/11/2021 20:04, Thomas Monjalon:
> > > 02/11/2021 19:45, David Marchand:
> > > > On Tue, Nov 2, 2021 at 3:53 PM Aman Kumar <aman.kumar@vvdntech.in> wrote:
> > > > >
> > > > > -Dcpu_instruction_set=znverX meson option can be used
> > > > > to build dpdk for AMD platforms. Supported options are
> > > > > znver1, znver2 and znver3.
> > > > >
> > > > > Signed-off-by: Aman Kumar <aman.kumar@vvdntech.in>
> > > > > ---
> > > > >  dpdk_conf.set('RTE_CACHE_LINE_SIZE', 64)
> > > > >  dpdk_conf.set('RTE_MAX_LCORE', 128)
> > > > >  dpdk_conf.set('RTE_MAX_NUMA_NODES', 32)
> > > > > +
> > > > > +# AMD platform support
> > > > > +if get_option('cpu_instruction_set') == 'znver1'
> > > > > +    dpdk_conf.set('RTE_MAX_LCORE', 256)
> > > > > +elif get_option('cpu_instruction_set') == 'znver2'
> > > > > +    dpdk_conf.set('RTE_MAX_LCORE', 512)
> > > > > +elif get_option('cpu_instruction_set') == 'znver3'
> > > > > +    dpdk_conf.set('RTE_MAX_LCORE', 512)
> > > > > +endif
> > > > 
> > > > I already replied to a similar patch earlier in this release.
> > > > https://inbox.dpdk.org/dev/CAJFAV8z-5amvEnr3mazkTqH-7SZX_C6EqCua6UdMXXHgrcmT6g@mail.gmail.com/
> > > > 
> > > > So repeating the same: do you actually _need_ more than 128 lcores in
> > > > a single DPDK application?
> > 
> > We did not receive an answer to this question.
> > 
> > > Yes I forgot this previous discussion concluding that we should not increase
> > > more than 128 threads.
> > 
> > We had a discussion yesterday in techboard meeting.
> > The consensus is that we didn't hear for real need of more than 128 threads,
> > except for configuration usability convenience.
> > 
> > Now looking again at the code, this is how it is defined:
> > 
> > option('max_lcores', type: 'string', value: 'default', description:
> >        'Set maximum number of cores/threads supported by EAL;
> >        "default" is different per-arch, "detect" detects the number of cores on the build machine.')
> > config/x86/meson.build: dpdk_conf.set('RTE_MAX_LCORE', 128)
> > config/ppc/meson.build: dpdk_conf.set('RTE_MAX_LCORE', 128)
> > config/arm/meson.build: it goes from 4 to 1280!
> > 
> > So I feel it is not fair to reject this AMD patch if we allow Arm to go beyond.
> > Techboard, let's have a quick decision please for 21.11-rc4.
> > 
> I would support increasing the default value for x86 in this release.

This patch is not increasing the default for all x86,
only for some CPUs as given at compilation time.
I think it is the same logic as Arm CPU-specific compilation.

> I believe Dave H. had some patches to decrease the memory footprint
> overhead of such a change. I don't believe that they were merged, and while
> it's a bit late for 21.11 now, those should be considered for 22.03 release
> and then maybe for backport.
Thomas Monjalon Nov. 24, 2021, 12:36 p.m. UTC | #8
Ping techboard for comments

18/11/2021 15:05, Thomas Monjalon:
> 18/11/2021 14:52, Bruce Richardson:
> > On Thu, Nov 18, 2021 at 01:25:38PM +0100, Thomas Monjalon wrote:
> > > I request a techboard decision for this patch.
> > > 
> > > 
> > > 02/11/2021 20:04, Thomas Monjalon:
> > > > 02/11/2021 19:45, David Marchand:
> > > > > On Tue, Nov 2, 2021 at 3:53 PM Aman Kumar <aman.kumar@vvdntech.in> wrote:
> > > > > >
> > > > > > -Dcpu_instruction_set=znverX meson option can be used
> > > > > > to build dpdk for AMD platforms. Supported options are
> > > > > > znver1, znver2 and znver3.
> > > > > >
> > > > > > Signed-off-by: Aman Kumar <aman.kumar@vvdntech.in>
> > > > > > ---
> > > > > >  dpdk_conf.set('RTE_CACHE_LINE_SIZE', 64)
> > > > > >  dpdk_conf.set('RTE_MAX_LCORE', 128)
> > > > > >  dpdk_conf.set('RTE_MAX_NUMA_NODES', 32)
> > > > > > +
> > > > > > +# AMD platform support
> > > > > > +if get_option('cpu_instruction_set') == 'znver1'
> > > > > > +    dpdk_conf.set('RTE_MAX_LCORE', 256)
> > > > > > +elif get_option('cpu_instruction_set') == 'znver2'
> > > > > > +    dpdk_conf.set('RTE_MAX_LCORE', 512)
> > > > > > +elif get_option('cpu_instruction_set') == 'znver3'
> > > > > > +    dpdk_conf.set('RTE_MAX_LCORE', 512)
> > > > > > +endif
> > > > > 
> > > > > I already replied to a similar patch earlier in this release.
> > > > > https://inbox.dpdk.org/dev/CAJFAV8z-5amvEnr3mazkTqH-7SZX_C6EqCua6UdMXXHgrcmT6g@mail.gmail.com/
> > > > > 
> > > > > So repeating the same: do you actually _need_ more than 128 lcores in
> > > > > a single DPDK application?
> > > 
> > > We did not receive an answer to this question.
> > > 
> > > > Yes I forgot this previous discussion concluding that we should not increase
> > > > more than 128 threads.
> > > 
> > > We had a discussion yesterday in techboard meeting.
> > > The consensus is that we didn't hear for real need of more than 128 threads,
> > > except for configuration usability convenience.
> > > 
> > > Now looking again at the code, this is how it is defined:
> > > 
> > > option('max_lcores', type: 'string', value: 'default', description:
> > >        'Set maximum number of cores/threads supported by EAL;
> > >        "default" is different per-arch, "detect" detects the number of cores on the build machine.')
> > > config/x86/meson.build: dpdk_conf.set('RTE_MAX_LCORE', 128)
> > > config/ppc/meson.build: dpdk_conf.set('RTE_MAX_LCORE', 128)
> > > config/arm/meson.build: it goes from 4 to 1280!
> > > 
> > > So I feel it is not fair to reject this AMD patch if we allow Arm to go beyond.
> > > Techboard, let's have a quick decision please for 21.11-rc4.
> > > 
> > I would support increasing the default value for x86 in this release.
> 
> This patch is not increasing the default for all x86,
> only for some CPUs as given at compilation time.
> I think it is the same logic as Arm CPU-specific compilation.
> 
> > I believe Dave H. had some patches to decrease the memory footprint
> > overhead of such a change. I don't believe that they were merged, and while
> > it's a bit late for 21.11 now, those should be considered for 22.03 release
> > and then maybe for backport.
diff mbox series

Patch

diff --git a/config/x86/meson.build b/config/x86/meson.build
index 29f3dea181..21cda6fd33 100644
--- a/config/x86/meson.build
+++ b/config/x86/meson.build
@@ -72,3 +72,12 @@  endif
 dpdk_conf.set('RTE_CACHE_LINE_SIZE', 64)
 dpdk_conf.set('RTE_MAX_LCORE', 128)
 dpdk_conf.set('RTE_MAX_NUMA_NODES', 32)
+
+# AMD platform support
+if get_option('cpu_instruction_set') == 'znver1'
+    dpdk_conf.set('RTE_MAX_LCORE', 256)
+elif get_option('cpu_instruction_set') == 'znver2'
+    dpdk_conf.set('RTE_MAX_LCORE', 512)
+elif get_option('cpu_instruction_set') == 'znver3'
+    dpdk_conf.set('RTE_MAX_LCORE', 512)
+endif
diff --git a/doc/guides/linux_gsg/build_dpdk.rst b/doc/guides/linux_gsg/build_dpdk.rst
index 0b08492ca2..e224a06cbd 100644
--- a/doc/guides/linux_gsg/build_dpdk.rst
+++ b/doc/guides/linux_gsg/build_dpdk.rst
@@ -111,7 +111,7 @@  The instruction set will be set automatically by default according to these rule
   a common minimal baseline needed for DPDK.
 
 To override what instruction set will be used, set the ``cpu_instruction_set``
-parameter to the instruction set of your choice (such as ``corei7``, ``power8``, etc.).
+parameter to the instruction set of your choice (such as ``corei7``, ``power8``, ``znver3``, etc.).
 
 ``cpu_instruction_set`` is not used in Arm builds, as setting the instruction set
 without other parameters leads to inferior builds. The way to tailor Arm builds