[PATCH 1/2] eal: expose lcore pthread id

[PATCH 1/2] eal: expose lcore pthread id

[Markus Theil]

From: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>

Also expose the pthread id of each lcore, in
order to allow modification of pthread attributes,
for example use rte_thread_setname without executing
pthread_self() on the maybe already running lcore.

The rte_lcore_to_thread_id function is added to API.

Signed-off-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>
---
 lib/eal/common/eal_common_lcore.c |  9 +++++++++
 lib/eal/include/rte_lcore.h       | 14 ++++++++++++++
 lib/eal/version.map               |  1 +
 3 files changed, 24 insertions(+)

diff --git a/lib/eal/common/eal_common_lcore.c b/lib/eal/common/eal_common_lcore.c
index 06c594b022..48c333cff1 100644
--- a/lib/eal/common/eal_common_lcore.c
+++ b/lib/eal/common/eal_common_lcore.c
@@ -114,6 +114,15 @@ rte_lcore_to_socket_id(unsigned int lcore_id)
 	return lcore_config[lcore_id].socket_id;
 }
 
+int
+rte_lcore_to_thread_id(int lcore_id, pthread_t *thread_id) {
+	if (unlikely(lcore_id < 0 || lcore_id >= RTE_MAX_LCORE))
+		return -1;
+
+	*thread_id = lcore_config[lcore_id].thread_id;
+	return 0;
+}
+
 static int
 socket_id_cmp(const void *a, const void *b)
 {
diff --git a/lib/eal/include/rte_lcore.h b/lib/eal/include/rte_lcore.h
index 4d3978512c..3ccd0a73b9 100644
--- a/lib/eal/include/rte_lcore.h
+++ b/lib/eal/include/rte_lcore.h
@@ -171,6 +171,20 @@ rte_lcore_to_socket_id(unsigned int lcore_id);
  */
 int rte_lcore_to_cpu_id(int lcore_id);
 
+/**
+ * Get the ID of the thread of the specified lcore
+ *
+ * @param lcore_id
+ *   the targeted lcore, which MUST be between 0 and RTE_MAX_LCORE-1.
+ * @param thread_id
+ *   the thread id is returned in this argument if lcore_id is in range
+ * @return
+ *   0 on success, or -1 if out of range
+ */
+__rte_experimental
+int
+rte_lcore_to_thread_id(int lcore_id, pthread_t *thread_id);
+
 #ifdef RTE_HAS_CPUSET
 
 /**
diff --git a/lib/eal/version.map b/lib/eal/version.map
index 2d64a2592e..2e57242920 100644
--- a/lib/eal/version.map
+++ b/lib/eal/version.map
@@ -440,6 +440,7 @@ EXPERIMENTAL {
 	rte_thread_detach;
 	rte_thread_equal;
 	rte_thread_join;
+	rte_lcore_to_thread_id;
 };
 
 INTERNAL {
-- 
2.38.0

[PATCH 2/2] eal: prevent OOB read in rte_lcore_to_socket_id

[Markus Theil]

Signed-off-by: Markus Theil <markus.theil@tu-ilmenau.de>
---
 lib/eal/common/eal_common_lcore.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/eal/common/eal_common_lcore.c b/lib/eal/common/eal_common_lcore.c
index 48c333cff1..94625c2ebc 100644
--- a/lib/eal/common/eal_common_lcore.c
+++ b/lib/eal/common/eal_common_lcore.c
@@ -111,6 +111,9 @@ unsigned int rte_get_next_lcore(unsigned int i, int skip_main, int wrap)
 unsigned int
 rte_lcore_to_socket_id(unsigned int lcore_id)
 {
+	if (unlikely(lcore_id >= RTE_MAX_LCORE))
+		return -1;
+
 	return lcore_config[lcore_id].socket_id;
 }
 
-- 
2.38.0

[PATCH v2 1/2] eal: expose lcore pthread id

[Markus Theil]

From: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>

Also expose the pthread id of each lcore, in
order to allow modification of pthread attributes,
for example use rte_thread_setname without executing
pthread_self() on the maybe already running lcore.

The rte_lcore_to_thread_id function is added to API.

Signed-off-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>
---
 lib/eal/common/eal_common_lcore.c | 10 ++++++++++
 lib/eal/include/rte_lcore.h       | 14 ++++++++++++++
 lib/eal/version.map               |  1 +
 3 files changed, 25 insertions(+)

diff --git a/lib/eal/common/eal_common_lcore.c b/lib/eal/common/eal_common_lcore.c
index 06c594b022..812e62bcb3 100644
--- a/lib/eal/common/eal_common_lcore.c
+++ b/lib/eal/common/eal_common_lcore.c
@@ -114,6 +114,16 @@ rte_lcore_to_socket_id(unsigned int lcore_id)
 	return lcore_config[lcore_id].socket_id;
 }
 
+int
+rte_lcore_to_thread_id(int lcore_id, pthread_t *thread_id)
+{
+	if (unlikely(lcore_id < 0 || lcore_id >= RTE_MAX_LCORE))
+		return -1;
+
+	*thread_id = lcore_config[lcore_id].thread_id;
+	return 0;
+}
+
 static int
 socket_id_cmp(const void *a, const void *b)
 {
diff --git a/lib/eal/include/rte_lcore.h b/lib/eal/include/rte_lcore.h
index 4d3978512c..3ccd0a73b9 100644
--- a/lib/eal/include/rte_lcore.h
+++ b/lib/eal/include/rte_lcore.h
@@ -171,6 +171,20 @@ rte_lcore_to_socket_id(unsigned int lcore_id);
  */
 int rte_lcore_to_cpu_id(int lcore_id);
 
+/**
+ * Get the ID of the thread of the specified lcore
+ *
+ * @param lcore_id
+ *   the targeted lcore, which MUST be between 0 and RTE_MAX_LCORE-1.
+ * @param thread_id
+ *   the thread id is returned in this argument if lcore_id is in range
+ * @return
+ *   0 on success, or -1 if out of range
+ */
+__rte_experimental
+int
+rte_lcore_to_thread_id(int lcore_id, pthread_t *thread_id);
+
 #ifdef RTE_HAS_CPUSET
 
 /**
diff --git a/lib/eal/version.map b/lib/eal/version.map
index 2d64a2592e..2e57242920 100644
--- a/lib/eal/version.map
+++ b/lib/eal/version.map
@@ -440,6 +440,7 @@ EXPERIMENTAL {
 	rte_thread_detach;
 	rte_thread_equal;
 	rte_thread_join;
+	rte_lcore_to_thread_id;
 };
 
 INTERNAL {
-- 
2.38.0

[PATCH v2 2/2] eal: prevent OOB read in rte_lcore_to_socket_id

[Markus Theil]

rte_lcore_to_socket_id did not check the lcore_id
range before accessing an array with RTE_MAX_LCORE
elements.

Signed-off-by: Markus Theil <markus.theil@tu-ilmenau.de>
---
 lib/eal/common/eal_common_lcore.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/eal/common/eal_common_lcore.c b/lib/eal/common/eal_common_lcore.c
index 812e62bcb3..af53efcc24 100644
--- a/lib/eal/common/eal_common_lcore.c
+++ b/lib/eal/common/eal_common_lcore.c
@@ -111,6 +111,9 @@ unsigned int rte_get_next_lcore(unsigned int i, int skip_main, int wrap)
 unsigned int
 rte_lcore_to_socket_id(unsigned int lcore_id)
 {
+	if (unlikely(lcore_id >= RTE_MAX_LCORE))
+		return -1;
+
 	return lcore_config[lcore_id].socket_id;
 }
 
-- 
2.38.0

Re: [PATCH v2 1/2] eal: expose lcore pthread id

[David Marchand]

On Fri, Oct 14, 2022 at 9:54 AM Markus Theil <markus.theil@tu-ilmenau.de> wrote:
>
> From: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>
>
> Also expose the pthread id of each lcore, in
> order to allow modification of pthread attributes,
> for example use rte_thread_setname without executing
> pthread_self() on the maybe already running lcore.
>
> The rte_lcore_to_thread_id function is added to API.
>
> Signed-off-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>

We are trying to abstract the use of pthread in DPDK API.
So I don't think this patch is going in the right direction.

Cc: Tyler.


-- 
David Marchand

Re: [PATCH v2 1/2] eal: expose lcore pthread id

[Stephen Hemminger]

On Thu, 20 Oct 2022 13:20:40 +0200
David Marchand <david.marchand@redhat.com> wrote:

> On Fri, Oct 14, 2022 at 9:54 AM Markus Theil <markus.theil@tu-ilmenau.de> wrote:
> >
> > From: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>
> >
> > Also expose the pthread id of each lcore, in
> > order to allow modification of pthread attributes,
> > for example use rte_thread_setname without executing
> > pthread_self() on the maybe already running lcore.
> >
> > The rte_lcore_to_thread_id function is added to API.
> >
> > Signed-off-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>  
> 
> We are trying to abstract the use of pthread in DPDK API.
> So I don't think this patch is going in the right direction.

Agree, exposing this will make Windows support harder
and who knows what next OS to come will need.

Re: [PATCH v2 1/2] eal: expose lcore pthread id

[Michael Pfeiffer]

On Thu, 2022-10-20 at 08:36 -0700, Stephen Hemminger wrote:
> On Thu, 20 Oct 2022 13:20:40 +0200
> David Marchand <david.marchand@redhat.com> wrote:
> 
> > On Fri, Oct 14, 2022 at 9:54 AM Markus Theil <markus.theil@tu-ilmenau.de>
> > wrote:
> > > 
> > > From: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>
> > > 
> > > Also expose the pthread id of each lcore, in
> > > order to allow modification of pthread attributes,
> > > for example use rte_thread_setname without executing
> > > pthread_self() on the maybe already running lcore.
> > > 
> > > The rte_lcore_to_thread_id function is added to API.
> > > 
> > > Signed-off-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>  
> > 
> > We are trying to abstract the use of pthread in DPDK API.
> > So I don't think this patch is going in the right direction.
> 
> Agree, exposing this will make Windows support harder
> and who knows what next OS to come will need.

Hi,
thanks for your feedback. I understand your concerns regarding abstraction and
portability.

Markus and I ultimately use the function in the patch to call
rte_thread_setname() (which takes the pthread id as an argument) to rename our
lcore workers from "lcore-worker-X" to something more meaningful in the scope
of our application. Having descriptive thread names makes debugging
significantly easier. For example, verifying CPU pinning worked as intended
with ps -T ..., or identifying threads in the Intel VTune profiler.

Would you consider something like
- int rte_lcore_setname(unsigned int lcore_id, const char *name)
- int rte_lcore_getname(unsigned int lcore_id, char *name, size_t len)
a more appropriate API? That would still allow us to set names from the main
lcore, but would not expose any pthread internals.

Thanks & regards
Michael

Re: [PATCH v2 1/2] eal: expose lcore pthread id

[Tyler Retzlaff]

On Thu, Oct 20, 2022 at 08:36:05AM -0700, Stephen Hemminger wrote:
> On Thu, 20 Oct 2022 13:20:40 +0200
> David Marchand <david.marchand@redhat.com> wrote:
> 
> > On Fri, Oct 14, 2022 at 9:54 AM Markus Theil <markus.theil@tu-ilmenau.de> wrote:
> > >
> > > From: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>
> > >
> > > Also expose the pthread id of each lcore, in
> > > order to allow modification of pthread attributes,
> > > for example use rte_thread_setname without executing
> > > pthread_self() on the maybe already running lcore.
> > >
> > > The rte_lcore_to_thread_id function is added to API.
> > >
> > > Signed-off-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>  
> > 
> > We are trying to abstract the use of pthread in DPDK API.
> > So I don't think this patch is going in the right direction.
> 
> Agree, exposing this will make Windows support harder
> and who knows what next OS to come will need.

+1

please don't expose platform details through eal you destroy
portability.

i'll take a closer look at this mail next week to see if i have anything
to offer.

Re: [PATCH v2 1/2] eal: expose lcore pthread id

[Tyler Retzlaff]

On Thu, Oct 20, 2022 at 10:03:01PM +0200, Michael Pfeiffer wrote:
> On Thu, 2022-10-20 at 08:36 -0700, Stephen Hemminger wrote:
> > On Thu, 20 Oct 2022 13:20:40 +0200
> > David Marchand <david.marchand@redhat.com> wrote:
> > 
> > > On Fri, Oct 14, 2022 at 9:54 AM Markus Theil <markus.theil@tu-ilmenau.de>
> > > wrote:
> > > > 
> > > > From: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>
> > > > 
> > > > Also expose the pthread id of each lcore, in
> > > > order to allow modification of pthread attributes,
> > > > for example use rte_thread_setname without executing
> > > > pthread_self() on the maybe already running lcore.
> > > > 
> > > > The rte_lcore_to_thread_id function is added to API.
> > > > 
> > > > Signed-off-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>  
> > > 
> > > We are trying to abstract the use of pthread in DPDK API.
> > > So I don't think this patch is going in the right direction.
> > 
> > Agree, exposing this will make Windows support harder
> > and who knows what next OS to come will need.
> 
> Hi,
> thanks for your feedback. I understand your concerns regarding abstraction and
> portability.
> 
> Markus and I ultimately use the function in the patch to call
> rte_thread_setname() (which takes the pthread id as an argument) to rename our
> lcore workers from "lcore-worker-X" to something more meaningful in the scope
> of our application. Having descriptive thread names makes debugging
> significantly easier. For example, verifying CPU pinning worked as intended
> with ps -T ..., or identifying threads in the Intel VTune profiler.
> 
> Would you consider something like
> - int rte_lcore_setname(unsigned int lcore_id, const char *name)
> - int rte_lcore_getname(unsigned int lcore_id, char *name, size_t len)
> a more appropriate API? That would still allow us to set names from the main
> lcore, but would not expose any pthread internals.

if only setname, getname are needed for rte_thread_t i imagine it
shouldn't be too objectionable to add them to rte_thread.h

you can either wait until i have time to do it (could be a while) or you
can put a new patch + unit test together.

if testing the windows implementation part of the addition is a barrier
i can apply and run the supplied unit test, assist with review.

> 
> Thanks & regards
> Michael

Re: [PATCH v2 1/2] eal: expose lcore pthread id

[Stephen Hemminger]

On Tue, 29 Nov 2022 14:04:45 -0800
Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:

> > Markus and I ultimately use the function in the patch to call
> > rte_thread_setname() (which takes the pthread id as an argument) to rename our
> > lcore workers from "lcore-worker-X" to something more meaningful in the scope
> > of our application. Having descriptive thread names makes debugging
> > significantly easier. For example, verifying CPU pinning worked as intended
> > with ps -T ..., or identifying threads in the Intel VTune profiler.

Why not have the worker threads rename themselves?

Re: [PATCH v2 1/2] eal: expose lcore pthread id

[Michael Pfeiffer]

On Wed, 2023-07-05 at 19:57 -0700, Stephen Hemminger wrote:
> On Tue, 29 Nov 2022 14:04:45 -0800
> Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
> 
> > > Markus and I ultimately use the function in the patch to call
> > > rte_thread_setname() (which takes the pthread id as an argument) to
> > > rename our
> > > lcore workers from "lcore-worker-X" to something more meaningful in the
> > > scope
> > > of our application. Having descriptive thread names makes debugging
> > > significantly easier. For example, verifying CPU pinning worked as
> > > intended
> > > with ps -T ..., or identifying threads in the Intel VTune profiler.
> 
> Why not have the worker threads rename themselves?

That's the way we ended up with, i.e. something like        
rte_thread_setname(pthread_self(), "...");
called by each worker.

I guess due to the possibility to set the name by pthread id, and obtain it for
ctrl threads, but not for regular workers, we got the impression of a "missing
piece" in the API. I understand the motivation to keep pthread internals out of
the API, so i guess we can drop the patch.

Thanks
Michael

-- 
Michael Pfeiffer

Technische Universität Ilmenau
Fakultät für Informatik und Automatisierung
Fachgebiet Telematik / Rechnernetze

E-Mail: michael.pfeiffer@tu-ilmenau.de
Telefon: +49 3677 69-4854
Web: https://www.tu-ilmenau.de/telematik