[v2,2/5] eal: add accessor functions for lcore_config
Checks
Commit Message
The fields of the internal EAL core configuration are currently
laid bare as part of the API. This is not good practice and limits
fixing issues with layout and sizes.
Make new accessor functions for the fields used by current drivers
and examples. Mark return code functions as experimental
since this value might change in the future and probably shouldn't
have been used by non EAL code anyway.
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Reviewed-by: David Marchand <david.marchand@redhat.com>
---
doc/guides/rel_notes/release_19_05.rst | 6 +++
lib/librte_eal/common/eal_common_lcore.c | 39 ++++++++++++++++++
lib/librte_eal/common/include/rte_lcore.h | 50 ++++++++++++++++-------
lib/librte_eal/rte_eal_version.map | 11 +++++
4 files changed, 92 insertions(+), 14 deletions(-)
Comments
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Stephen Hemminger
> Sent: Wednesday, April 10, 2019 10:46 PM
> To: dev@dpdk.org
> Cc: Stephen Hemminger <stephen@networkplumber.org>
> Subject: [dpdk-dev] [PATCH v2 2/5] eal: add accessor functions for lcore_config
>
> The fields of the internal EAL core configuration are currently laid bare as part of
> the API. This is not good practice and limits fixing issues with layout and sizes.
>
> Make new accessor functions for the fields used by current drivers and
> examples. Mark return code functions as experimental since this value might
> change in the future and probably shouldn't have been used by non EAL code
> anyway.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Reviewed-by: David Marchand <david.marchand@redhat.com>
> Shared Library Versions
> -----------------------
> diff --git a/lib/librte_eal/common/eal_common_lcore.c
> b/lib/librte_eal/common/eal_common_lcore.c
> index 1cbac42286ba..6cf4d7abb0bd 100644
> --- a/lib/librte_eal/common/eal_common_lcore.c
> +++ b/lib/librte_eal/common/eal_common_lcore.c
> @@ -16,6 +16,45 @@
> #include "eal_private.h"
> #include "eal_thread.h"
>
> +int rte_lcore_index(int lcore_id)
> +{
> + if (unlikely(lcore_id >= RTE_MAX_LCORE))
> + return -1;
> +
> + if (lcore_id < 0)
> + lcore_id = (int)rte_lcore_id();
> +
> + return lcore_config[lcore_id].core_index;
> +}
If I understand it correctly, We are planning to do this scheme only for slow path functions. Right?
Is rte_lcore_* functions comes in slow path category? I thought a few of them can be used
In fast path too.
I am bit concerned about a low end arm cores where function invocation overhead significant vs inline.
ODP has taken a route of introducing a compile time config to choose inline vs separate function to address
the performance regression . I am not sure what would be the correct way to handle this.
On Tue, 16 Apr 2019 17:03:47 +0000
Jerin Jacob Kollanukkaran <jerinj@marvell.com> wrote:
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Stephen Hemminger
> > Sent: Wednesday, April 10, 2019 10:46 PM
> > To: dev@dpdk.org
> > Cc: Stephen Hemminger <stephen@networkplumber.org>
> > Subject: [dpdk-dev] [PATCH v2 2/5] eal: add accessor functions for lcore_config
> >
> > The fields of the internal EAL core configuration are currently laid bare as part of
> > the API. This is not good practice and limits fixing issues with layout and sizes.
> >
> > Make new accessor functions for the fields used by current drivers and
> > examples. Mark return code functions as experimental since this value might
> > change in the future and probably shouldn't have been used by non EAL code
> > anyway.
> >
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > Reviewed-by: David Marchand <david.marchand@redhat.com>
> > Shared Library Versions
> > -----------------------
> > diff --git a/lib/librte_eal/common/eal_common_lcore.c
> > b/lib/librte_eal/common/eal_common_lcore.c
> > index 1cbac42286ba..6cf4d7abb0bd 100644
> > --- a/lib/librte_eal/common/eal_common_lcore.c
> > +++ b/lib/librte_eal/common/eal_common_lcore.c
> > @@ -16,6 +16,45 @@
> > #include "eal_private.h"
> > #include "eal_thread.h"
> >
> > +int rte_lcore_index(int lcore_id)
> > +{
> > + if (unlikely(lcore_id >= RTE_MAX_LCORE))
> > + return -1;
> > +
> > + if (lcore_id < 0)
> > + lcore_id = (int)rte_lcore_id();
> > +
> > + return lcore_config[lcore_id].core_index;
> > +}
>
> If I understand it correctly, We are planning to do this scheme only for slow path functions. Right?
> Is rte_lcore_* functions comes in slow path category? I thought a few of them can be used
> In fast path too.
> I am bit concerned about a low end arm cores where function invocation overhead significant vs inline.
> ODP has taken a route of introducing a compile time config to choose inline vs separate function to address
> the performance regression . I am not sure what would be the correct way to handle this.
The lcore config itself is not accessed anywhere in fastpath of any of the examples.
It is usually is part of the setup process. The rte_lcore_index is only used
by lthread in a log message.
The main fastpath is rte_lcore_id() which is already done by per-thread variable
and is unchanged by these patches.
I don't have facilities to do any deep dive performance testing on ARM.
> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Wednesday, May 1, 2019 2:23 AM
> To: Jerin Jacob Kollanukkaran <jerinj@marvell.com>
> Cc: dev@dpdk.org
> Subject: [EXT] Re: [dpdk-dev] [PATCH v2 2/5] eal: add accessor functions for
> lcore_config
> On Tue, 16 Apr 2019 17:03:47 +0000
> Jerin Jacob Kollanukkaran <jerinj@marvell.com> wrote:
>
> > > -----Original Message-----
> > > From: dev <dev-bounces@dpdk.org> On Behalf Of Stephen Hemminger
> > > Sent: Wednesday, April 10, 2019 10:46 PM
> > > To: dev@dpdk.org
> > > Cc: Stephen Hemminger <stephen@networkplumber.org>
> > > Subject: [dpdk-dev] [PATCH v2 2/5] eal: add accessor functions for
> > > lcore_config
> > >
> > > The fields of the internal EAL core configuration are currently laid
> > > bare as part of the API. This is not good practice and limits fixing issues with
> layout and sizes.
> > >
> > > Make new accessor functions for the fields used by current drivers
> > > and examples. Mark return code functions as experimental since this
> > > value might change in the future and probably shouldn't have been
> > > used by non EAL code anyway.
> > >
> > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > > Reviewed-by: David Marchand <david.marchand@redhat.com> Shared
> > > Library Versions
> > > -----------------------
> > > diff --git a/lib/librte_eal/common/eal_common_lcore.c
> > > b/lib/librte_eal/common/eal_common_lcore.c
> > > index 1cbac42286ba..6cf4d7abb0bd 100644
> > > --- a/lib/librte_eal/common/eal_common_lcore.c
> > > +++ b/lib/librte_eal/common/eal_common_lcore.c
> > > @@ -16,6 +16,45 @@
> > > #include "eal_private.h"
> > > #include "eal_thread.h"
> > >
> > > +int rte_lcore_index(int lcore_id)
> > > +{
> > > + if (unlikely(lcore_id >= RTE_MAX_LCORE))
> > > + return -1;
> > > +
> > > + if (lcore_id < 0)
> > > + lcore_id = (int)rte_lcore_id();
> > > +
> > > + return lcore_config[lcore_id].core_index;
> > > +}
> >
> > If I understand it correctly, We are planning to do this scheme only for slow
> path functions. Right?
> > Is rte_lcore_* functions comes in slow path category? I thought a few
> > of them can be used In fast path too.
> > I am bit concerned about a low end arm cores where function invocation
> overhead significant vs inline.
> > ODP has taken a route of introducing a compile time config to choose
> > inline vs separate function to address the performance regression . I am not
> sure what would be the correct way to handle this.
>
> The lcore config itself is not accessed anywhere in fastpath of any of the
> examples.
> It is usually is part of the setup process. The rte_lcore_index is only used by
> lthread in a log message.
Makes sense.
>
> The main fastpath is rte_lcore_id() which is already done by per-thread variable
> and is unchanged by these patches.
>
How about creating a tag like __rte_experimental, say __rte_fastpath
for fastpath functions to avoid confusion between what is fastpath and what is not
between developers and end users. By creating new section in linker, will
make all fast path function in one area.
> I don't have facilities to do any deep dive performance testing on ARM.
On Wed, Apr 10, 2019 at 7:16 PM Stephen Hemminger <
stephen@networkplumber.org> wrote:
> The fields of the internal EAL core configuration are currently
> laid bare as part of the API. This is not good practice and limits
> fixing issues with layout and sizes.
>
> Make new accessor functions for the fields used by current drivers
> and examples. Mark return code functions as experimental
> since this value might change in the future and probably shouldn't
> have been used by non EAL code anyway.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Reviewed-by: David Marchand <david.marchand@redhat.com>
> ---
> doc/guides/rel_notes/release_19_05.rst | 6 +++
> lib/librte_eal/common/eal_common_lcore.c | 39 ++++++++++++++++++
> lib/librte_eal/common/include/rte_lcore.h | 50 ++++++++++++++++-------
> lib/librte_eal/rte_eal_version.map | 11 +++++
> 4 files changed, 92 insertions(+), 14 deletions(-)
>
> diff --git a/doc/guides/rel_notes/release_19_05.rst
> b/doc/guides/rel_notes/release_19_05.rst
> index dbdf07a0c05b..32aae5d3bcfa 100644
> --- a/doc/guides/rel_notes/release_19_05.rst
> +++ b/doc/guides/rel_notes/release_19_05.rst
> @@ -222,6 +222,12 @@ ABI Changes
> alignment for ``rte_crypto_asym_op`` to restore expected
> ``rte_crypto_op``
> layout and alignment.
>
> +* eal: the lcore config structure ``struct lcore_config`` will be made
> + internal to the EAL in a future release. This will allow the structure
> to
> + change without impacting API or ABI. All accesses to fields of this
> + structure should be done by the corresponding accessor functions.
> + For example, instead of using ``lcore_config[lcore_id].socket_id``
> + the function ``rte_lcore_socket_id(lcore_id)`` should be used instead.
>
> Shared Library Versions
> -----------------------
> diff --git a/lib/librte_eal/common/eal_common_lcore.c
> b/lib/librte_eal/common/eal_common_lcore.c
> index 1cbac42286ba..6cf4d7abb0bd 100644
> --- a/lib/librte_eal/common/eal_common_lcore.c
> +++ b/lib/librte_eal/common/eal_common_lcore.c
> @@ -16,6 +16,45 @@
> #include "eal_private.h"
> #include "eal_thread.h"
>
> +int rte_lcore_index(int lcore_id)
> +{
> + if (unlikely(lcore_id >= RTE_MAX_LCORE))
> + return -1;
> +
> + if (lcore_id < 0)
> + lcore_id = (int)rte_lcore_id();
> +
> + return lcore_config[lcore_id].core_index;
> +}
> +
> +int rte_lcore_to_cpu_id(int lcore_id)
> +{
> + if (unlikely(lcore_id >= RTE_MAX_LCORE))
> + return -1;
> +
> + if (lcore_id < 0)
> + lcore_id = (int)rte_lcore_id();
> +
> + return lcore_config[lcore_id].core_id;
> +}
> +
> +rte_cpuset_t rte_lcore_cpuset(unsigned int lcore_id)
> +{
> + return lcore_config[lcore_id].cpuset;
> +}
> +
> +unsigned int
> +rte_lcore_to_socket_id(unsigned int lcore_id)
> +{
> + return lcore_config[lcore_id].socket_id;
> +}
> +
> +int
> +rte_lcore_return_code(unsigned int lcore_id)
> +{
> + return lcore_config[lcore_id].ret;
> +}
> +
> static int
> socket_id_cmp(const void *a, const void *b)
> {
> diff --git a/lib/librte_eal/common/include/rte_lcore.h
> b/lib/librte_eal/common/include/rte_lcore.h
> index 959ef9ece4b2..dc9f3dc0843d 100644
> --- a/lib/librte_eal/common/include/rte_lcore.h
> +++ b/lib/librte_eal/common/include/rte_lcore.h
> @@ -121,15 +121,8 @@ rte_lcore_count(void)
> * @return
> * The relative index, or -1 if not enabled.
> */
> -static inline int
> -rte_lcore_index(int lcore_id)
> -{
> - if (lcore_id >= RTE_MAX_LCORE)
> - return -1;
> - if (lcore_id < 0)
> - lcore_id = (int)rte_lcore_id();
> - return lcore_config[lcore_id].core_index;
> -}
> +int __rte_experimental
> +rte_lcore_index(int lcore_id);
>
This is new from v2.
Please remove this __rte_experimental tag.
> /**
> * Return the ID of the physical socket of the logical core we are
> @@ -177,11 +170,40 @@ rte_socket_id_by_idx(unsigned int idx);
> * @return
> * the ID of lcoreid's physical socket
> */
> -static inline unsigned int
> -rte_lcore_to_socket_id(unsigned int lcore_id)
> -{
> - return lcore_config[lcore_id].socket_id;
> -}
> +unsigned int
> +rte_lcore_to_socket_id(unsigned int lcore_id);
> +
> +/**
> + * Return the id of the lcore on a socket starting from zero.
> + *
> + * @param lcore_id
> + * The targeted lcore, or -1 for the current one.
> + * @return
> + * The relative index, or -1 if not enabled.
> + */
> +int
> +rte_lcore_to_cpu_id(int lcore_id);
> +
> +/**
> + * Return the cpuset for a given lcore.
> + * @param lcore_id
> + * the targeted lcore, which MUST be between 0 and RTE_MAX_LCORE-1.
> + * @return
> + * The cpuset of that lcore
> + */
> +rte_cpuset_t
> +rte_lcore_cpuset(unsigned int lcore_id);
> +
> +/**
> + * Get the return code from a lcore thread.
> + * @param lcore_id
> + * the targeted lcore, which MUST be between 0 and RTE_MAX_LCORE-1
> + * and finished
> + * @return
> + * the return code from the lcore thread
> + */
> +int __rte_experimental
> +rte_lcore_return_code(unsigned int lcore_id);
>
> /**
> * Test if an lcore is enabled.
> diff --git a/lib/librte_eal/rte_eal_version.map
> b/lib/librte_eal/rte_eal_version.map
> index d6e375135ad1..f6688327cad3 100644
> --- a/lib/librte_eal/rte_eal_version.map
> +++ b/lib/librte_eal/rte_eal_version.map
> @@ -268,6 +268,16 @@ DPDK_18.11 {
>
> } DPDK_18.08;
>
> +DPDK_19.05 {
> + global:
> +
> + rte_lcore_cpuset;
> + rte_lcore_index;
> + rte_lcore_to_cpu_id;
> + rte_lcore_to_socket_id;
> +
> +} DPDK_18.08;
> +
>
19.05 inherits from 18.11 not 18.08.
EXPERIMENTAL {
> global:
>
> @@ -329,6 +339,7 @@ EXPERIMENTAL {
> rte_fbarray_set_free;
> rte_fbarray_set_used;
> rte_intr_callback_unregister_pending;
> + rte_lcore_return_code;
> rte_log_register_type_and_pick_level;
> rte_malloc_dump_heaps;
> rte_malloc_heap_create;
> --
> 2.17.1
>
And with these two changes, renewing my tag.
Reviewed-by: David Marchand <david.marchand@redhat.com>
@@ -222,6 +222,12 @@ ABI Changes
alignment for ``rte_crypto_asym_op`` to restore expected ``rte_crypto_op``
layout and alignment.
+* eal: the lcore config structure ``struct lcore_config`` will be made
+ internal to the EAL in a future release. This will allow the structure to
+ change without impacting API or ABI. All accesses to fields of this
+ structure should be done by the corresponding accessor functions.
+ For example, instead of using ``lcore_config[lcore_id].socket_id``
+ the function ``rte_lcore_socket_id(lcore_id)`` should be used instead.
Shared Library Versions
-----------------------
@@ -16,6 +16,45 @@
#include "eal_private.h"
#include "eal_thread.h"
+int rte_lcore_index(int lcore_id)
+{
+ if (unlikely(lcore_id >= RTE_MAX_LCORE))
+ return -1;
+
+ if (lcore_id < 0)
+ lcore_id = (int)rte_lcore_id();
+
+ return lcore_config[lcore_id].core_index;
+}
+
+int rte_lcore_to_cpu_id(int lcore_id)
+{
+ if (unlikely(lcore_id >= RTE_MAX_LCORE))
+ return -1;
+
+ if (lcore_id < 0)
+ lcore_id = (int)rte_lcore_id();
+
+ return lcore_config[lcore_id].core_id;
+}
+
+rte_cpuset_t rte_lcore_cpuset(unsigned int lcore_id)
+{
+ return lcore_config[lcore_id].cpuset;
+}
+
+unsigned int
+rte_lcore_to_socket_id(unsigned int lcore_id)
+{
+ return lcore_config[lcore_id].socket_id;
+}
+
+int
+rte_lcore_return_code(unsigned int lcore_id)
+{
+ return lcore_config[lcore_id].ret;
+}
+
static int
socket_id_cmp(const void *a, const void *b)
{
@@ -121,15 +121,8 @@ rte_lcore_count(void)
* @return
* The relative index, or -1 if not enabled.
*/
-static inline int
-rte_lcore_index(int lcore_id)
-{
- if (lcore_id >= RTE_MAX_LCORE)
- return -1;
- if (lcore_id < 0)
- lcore_id = (int)rte_lcore_id();
- return lcore_config[lcore_id].core_index;
-}
+int __rte_experimental
+rte_lcore_index(int lcore_id);
/**
* Return the ID of the physical socket of the logical core we are
@@ -177,11 +170,40 @@ rte_socket_id_by_idx(unsigned int idx);
* @return
* the ID of lcoreid's physical socket
*/
-static inline unsigned int
-rte_lcore_to_socket_id(unsigned int lcore_id)
-{
- return lcore_config[lcore_id].socket_id;
-}
+unsigned int
+rte_lcore_to_socket_id(unsigned int lcore_id);
+
+/**
+ * Return the id of the lcore on a socket starting from zero.
+ *
+ * @param lcore_id
+ * The targeted lcore, or -1 for the current one.
+ * @return
+ * The relative index, or -1 if not enabled.
+ */
+int
+rte_lcore_to_cpu_id(int lcore_id);
+
+/**
+ * Return the cpuset for a given lcore.
+ * @param lcore_id
+ * the targeted lcore, which MUST be between 0 and RTE_MAX_LCORE-1.
+ * @return
+ * The cpuset of that lcore
+ */
+rte_cpuset_t
+rte_lcore_cpuset(unsigned int lcore_id);
+
+/**
+ * Get the return code from a lcore thread.
+ * @param lcore_id
+ * the targeted lcore, which MUST be between 0 and RTE_MAX_LCORE-1
+ * and finished
+ * @return
+ * the return code from the lcore thread
+ */
+int __rte_experimental
+rte_lcore_return_code(unsigned int lcore_id);
/**
* Test if an lcore is enabled.
@@ -268,6 +268,16 @@ DPDK_18.11 {
} DPDK_18.08;
+DPDK_19.05 {
+ global:
+
+ rte_lcore_cpuset;
+ rte_lcore_index;
+ rte_lcore_to_cpu_id;
+ rte_lcore_to_socket_id;
+
+} DPDK_18.08;
+
EXPERIMENTAL {
global:
@@ -329,6 +339,7 @@ EXPERIMENTAL {
rte_fbarray_set_free;
rte_fbarray_set_used;
rte_intr_callback_unregister_pending;
+ rte_lcore_return_code;
rte_log_register_type_and_pick_level;
rte_malloc_dump_heaps;
rte_malloc_heap_create;