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

Message ID 20221014075421.10300-1-markus.theil@tu-ilmenau.de (mailing list archive)
State Rejected, archived
Delegated to: Thomas Monjalon
Headers
Series [v2,1/2] eal: expose lcore pthread id |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Markus Theil Oct. 14, 2022, 7:54 a.m. UTC
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(+)
  

Comments

David Marchand Oct. 20, 2022, 11:20 a.m. UTC | #1
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.
  
Stephen Hemminger Oct. 20, 2022, 3:36 p.m. UTC | #2
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.
  
Michael Pfeiffer Oct. 20, 2022, 8:03 p.m. UTC | #3
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
  
Tyler Retzlaff Nov. 12, 2022, 12:34 a.m. UTC | #4
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.
  
Tyler Retzlaff Nov. 29, 2022, 10:04 p.m. UTC | #5
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
  
Stephen Hemminger July 6, 2023, 2:57 a.m. UTC | #6
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?
  
Michael Pfeiffer July 6, 2023, 5:50 a.m. UTC | #7
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
  

Patch

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 {