[v4,0/6] add thread lifetime and attributes API

  add rte thread lifetime and attributes api. with these api additions
there is now sufficient platform abstracted thread api to remove the
use of pthread in the unit tests.

v4:
  * update version.map to show api from series added in 22.11 instead
    of 22.07.
  * fix missing parameter name in rte_thread_func declaration causing
    doxygen ci failure.

v3:
  * change rte_thread_func return type to uint32_t for exit value.
  * change rte_thread_join retval to be uint32_t (matched with the
    return value from rte_thread_func).
  * introduce a wrapper for rte_thread_func on posix platforms to
    adapt differences between rte_thread_func and pthread
    start_routine.
  * remove interpretation / dereference of result from pthread_join
    in posix implementation of rte_thread_join.
  * fix leak of dynamically allocated thread_routine_ctx on windows
    in error paths.
  * don't cast and truncate NULL to integer value for rte_thread_join
    when pthread_join returns no result.

v2:
  * split implementation of rte_thread_equal for windows / posix
    and use pthread_equal for posix platforms.
  * remove parameter validation assertions and instead return
    EINVAL for mandatory pointers to type that are NULL.
  * correct doxygen comment parameter name args -> arg

Tyler Retzlaff (6):
  eal: add thread attributes
  eal: add thread lifetime management
  eal: add basic rte thread ID equal API
  test/threads: add tests for thread lifetime API
  test/threads: add tests for thread attributes API
  test/threads: remove unit test use of pthread

 app/test/test_threads.c         | 134 ++++++++++++++++++++++--
 lib/eal/common/meson.build      |   1 +
 lib/eal/common/rte_thread.c     |  60 +++++++++++
 lib/eal/include/rte_thread.h    | 187 ++++++++++++++++++++++++++++++++++
 lib/eal/unix/rte_thread.c       | 141 ++++++++++++++++++++++++++
 lib/eal/version.map             |  10 ++
 lib/eal/windows/include/sched.h |   2 +-
 lib/eal/windows/rte_thread.c    | 219 ++++++++++++++++++++++++++++++++--------
 8 files changed, 705 insertions(+), 49 deletions(-)
 create mode 100644 lib/eal/common/rte_thread.c
  

2022-06-27 09:56 (UTC-0700), Tyler Retzlaff:
> add rte thread lifetime and attributes api. with these api additions
> there is now sufficient platform abstracted thread api to remove the
> use of pthread in the unit tests.

For series,
Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>

Sorry it took so long to review.
  

On Mon, Jun 27, 2022 at 6:56 PM Tyler Retzlaff
<roretzla@linux.microsoft.com> wrote:
>
> add rte thread lifetime and attributes api. with these api additions
> there is now sufficient platform abstracted thread api to remove the
> use of pthread in the unit tests.
>
> v4:
>   * update version.map to show api from series added in 22.11 instead
>     of 22.07.

Patch 1 still adds symbols in 22.07 section of the experimental block.

Compilation is broken when stopping at patch 4.

Newly added code can go to eal_common_thread.c rather than introduce a
new common/rte_thread.c file (or is there a rationale for this?).

There are guards for the new structs against RTE_HAS_CPUSET being defined.
Can you elaborate why those guards are needed?

Commitlogs / comments sentences must start with a capital letter.

Can you prepare a new revision for -rc1?

Thanks.


>   * fix missing parameter name in rte_thread_func declaration causing
>     doxygen ci failure.
  

On Wed, Sep 21, 2022 at 10:15 AM David Marchand
<david.marchand@redhat.com> wrote:
>
> On Mon, Jun 27, 2022 at 6:56 PM Tyler Retzlaff
> <roretzla@linux.microsoft.com> wrote:
> >
> > add rte thread lifetime and attributes api. with these api additions
> > there is now sufficient platform abstracted thread api to remove the
> > use of pthread in the unit tests.
> >
> > v4:
> >   * update version.map to show api from series added in 22.11 instead
> >     of 22.07.
>
> Patch 1 still adds symbols in 22.07 section of the experimental block.
>
> Compilation is broken when stopping at patch 4.
>
> Newly added code can go to eal_common_thread.c rather than introduce a
> new common/rte_thread.c file (or is there a rationale for this?).
>
> There are guards for the new structs against RTE_HAS_CPUSET being defined.
> Can you elaborate why those guards are needed?
>
> Commitlogs / comments sentences must start with a capital letter.
>
> Can you prepare a new revision for -rc1?

Any update?
  

hi David,

On Wed, Sep 21, 2022 at 10:15:36AM +0200, David Marchand wrote:
> On Mon, Jun 27, 2022 at 6:56 PM Tyler Retzlaff
> <roretzla@linux.microsoft.com> wrote:
> >
> > add rte thread lifetime and attributes api. with these api additions
> > there is now sufficient platform abstracted thread api to remove the
> > use of pthread in the unit tests.
> >
> > v4:
> >   * update version.map to show api from series added in 22.11 instead
> >     of 22.07.
> 
> Patch 1 still adds symbols in 22.07 section of the experimental block.

this looks like a goofup on my part, i will fix in the next revision.

> 
> Compilation is broken when stopping at patch 4.

this was because commit 72b452c5f2599f970f47fd17d3e8e5d60bfebe7a removed
#include of <errno.h> from rte_common.h

i will fix this in the next revision.

> 
> Newly added code can go to eal_common_thread.c rather than introduce a
> new common/rte_thread.c file (or is there a rationale for this?).

i will make this change in the next revision. if anyone does object i
hope they will do so quickly.

> 
> There are guards for the new structs against RTE_HAS_CPUSET being defined.
> Can you elaborate why those guards are needed?

eal uses this guard in a couple of places so the best explanation i can
provide is an existing pattern was being followed. that being said it's
clear that this code won't work without RTE_HAS_CPUSET.

i will remove the guard and test if the build breaks for the platforms
i have toolchains available. if it builds cleanly i'll remove the guard
in the next revision.

> 
> Commitlogs / comments sentences must start with a capital letter.

i will fix in next revision, but ew caps.

> 
> Can you prepare a new revision for -rc1?

assuming the above actions to address feedback are acceptable the new
revision should be up today (PST).

> 
> Thanks.

thank you!

ty
  

On Wed, Oct 05, 2022 at 09:11:26AM -0700, Tyler Retzlaff wrote:
> hi David,
> 
> 
> > 
> > Newly added code can go to eal_common_thread.c rather than introduce a
> > new common/rte_thread.c file (or is there a rationale for this?).
> 
> i will make this change in the next revision. if anyone does object i
> hope they will do so quickly.

looking at this more closely i'm going to back away from making the
adjustment here. if Thomas and/or Dmitry could comment it would be
appreciated.

it appears that functions placed in eal_common_xxx files are consumed
internally by the eal where rte_xxx files are functions that are exposed
through public api.

since these additions are public api it seems they should remain in
rte_thread.c
 
i won't change this in the next revision, but please do correct me if
i'm still not on track.
  

On Wed, Oct 5, 2022 at 6:34 PM Tyler Retzlaff
<roretzla@linux.microsoft.com> wrote:
>
> On Wed, Oct 05, 2022 at 09:11:26AM -0700, Tyler Retzlaff wrote:
> > hi David,
> >
> >
> > >
> > > Newly added code can go to eal_common_thread.c rather than introduce a
> > > new common/rte_thread.c file (or is there a rationale for this?).
> >
> > i will make this change in the next revision. if anyone does object i
> > hope they will do so quickly.
>
> looking at this more closely i'm going to back away from making the
> adjustment here. if Thomas and/or Dmitry could comment it would be
> appreciated.
>
> it appears that functions placed in eal_common_xxx files are consumed
> internally by the eal where rte_xxx files are functions that are exposed
> through public api.
>
> since these additions are public api it seems they should remain in
> rte_thread.c
>
> i won't change this in the next revision, but please do correct me if
> i'm still not on track.

This seems a fair argument.


I'll reply on the v5 series: compilation is still broken at patch 4.
  

05/10/2022 18:34, Tyler Retzlaff:
> On Wed, Oct 05, 2022 at 09:11:26AM -0700, Tyler Retzlaff wrote:
> > > Newly added code can go to eal_common_thread.c rather than introduce a
> > > new common/rte_thread.c file (or is there a rationale for this?).
> > 
> > i will make this change in the next revision. if anyone does object i
> > hope they will do so quickly.
> 
> looking at this more closely i'm going to back away from making the
> adjustment here. if Thomas and/or Dmitry could comment it would be
> appreciated.
> 
> it appears that functions placed in eal_common_xxx files are consumed
> internally by the eal where rte_xxx files are functions that are exposed
> through public api.

It is not so clear.
There is already eal_common_thread.c which implements the same kind of functions,
so I think you should move your new functions here.

> since these additions are public api it seems they should remain in
> rte_thread.c

Let's not have 2 .c files for the same purpose in the same directory.

> i won't change this in the next revision, but please do correct me if
> i'm still not on track.

Please I would prefer to keep all in one file: eal_common_thread.c
Thank you
  

On Thu, Oct 06, 2022 at 03:36:12PM +0200, Thomas Monjalon wrote:
> 05/10/2022 18:34, Tyler Retzlaff:
> > On Wed, Oct 05, 2022 at 09:11:26AM -0700, Tyler Retzlaff wrote:
> > > > Newly added code can go to eal_common_thread.c rather than introduce a
> > > > new common/rte_thread.c file (or is there a rationale for this?).
> > > 
> > > i will make this change in the next revision. if anyone does object i
> > > hope they will do so quickly.
> > 
> > looking at this more closely i'm going to back away from making the
> > adjustment here. if Thomas and/or Dmitry could comment it would be
> > appreciated.
> > 
> > it appears that functions placed in eal_common_xxx files are consumed
> > internally by the eal where rte_xxx files are functions that are exposed
> > through public api.
> 
> It is not so clear.
> There is already eal_common_thread.c which implements the same kind of functions,
> so I think you should move your new functions here.
> 
> > since these additions are public api it seems they should remain in
> > rte_thread.c
> 
> Let's not have 2 .c files for the same purpose in the same directory.

just as another point there seem to be several other rte_xxx.c files
here can we clarify why they were not subject to the same requirement?
as a follow on does it mean that the code in those files should also be
moved to eal_common_xxx files?

please let me know if the justification is not the same i'll move the
functions to the eal_common file as requested. i just want to make sure
it is being done for the consistent/correct reason.

thanks.
  

On Thu, Oct 06, 2022 at 08:52:02AM +0200, David Marchand wrote:
> On Wed, Oct 5, 2022 at 6:34 PM Tyler Retzlaff
> <roretzla@linux.microsoft.com> wrote:
> >
> > On Wed, Oct 05, 2022 at 09:11:26AM -0700, Tyler Retzlaff wrote:
> > > hi David,
> > >
> > >
> > > >
> > > > Newly added code can go to eal_common_thread.c rather than introduce a
> > > > new common/rte_thread.c file (or is there a rationale for this?).
> > >
> > > i will make this change in the next revision. if anyone does object i
> > > hope they will do so quickly.
> >
> > looking at this more closely i'm going to back away from making the
> > adjustment here. if Thomas and/or Dmitry could comment it would be
> > appreciated.
> >
> > it appears that functions placed in eal_common_xxx files are consumed
> > internally by the eal where rte_xxx files are functions that are exposed
> > through public api.
> >
> > since these additions are public api it seems they should remain in
> > rte_thread.c
> >
> > i won't change this in the next revision, but please do correct me if
> > i'm still not on track.
> 
> This seems a fair argument.
> 
> 
> I'll reply on the v5 series: compilation is still broken at patch 4.

can you share what the break is? and which platform/target/toolchain?

the only thing I found was that the removal of errno.h broke compilation
but that is fixed in v5. the CI run for the series doesn't indicate any
compilation failures. assuming i'm looking at the correct results.

https://lab.dpdk.org/results/dashboard/patchsets/23789/

it does show a unit test failure but it looks like it is related to a
service test that you mailed about within the past few days.

thanks
  

06/10/2022 17:10, Tyler Retzlaff:
> On Thu, Oct 06, 2022 at 03:36:12PM +0200, Thomas Monjalon wrote:
> > 05/10/2022 18:34, Tyler Retzlaff:
> > > On Wed, Oct 05, 2022 at 09:11:26AM -0700, Tyler Retzlaff wrote:
> > > > > Newly added code can go to eal_common_thread.c rather than introduce a
> > > > > new common/rte_thread.c file (or is there a rationale for this?).
> > > > 
> > > > i will make this change in the next revision. if anyone does object i
> > > > hope they will do so quickly.
> > > 
> > > looking at this more closely i'm going to back away from making the
> > > adjustment here. if Thomas and/or Dmitry could comment it would be
> > > appreciated.
> > > 
> > > it appears that functions placed in eal_common_xxx files are consumed
> > > internally by the eal where rte_xxx files are functions that are exposed
> > > through public api.
> > 
> > It is not so clear.
> > There is already eal_common_thread.c which implements the same kind of functions,
> > so I think you should move your new functions here.
> > 
> > > since these additions are public api it seems they should remain in
> > > rte_thread.c
> > 
> > Let's not have 2 .c files for the same purpose in the same directory.
> 
> just as another point there seem to be several other rte_xxx.c files
> here can we clarify why they were not subject to the same requirement?
> as a follow on does it mean that the code in those files should also be
> moved to eal_common_xxx files?

That's just history.

> please let me know if the justification is not the same i'll move the
> functions to the eal_common file as requested. i just want to make sure
> it is being done for the consistent/correct reason.

Some file names are not correct, we could rename them.

I think David is already doing the last minor changes on this series
while merging, so no need to do anything on your side.
  

On Thu, Oct 06, 2022 at 05:14:55PM +0200, Thomas Monjalon wrote:
> 06/10/2022 17:10, Tyler Retzlaff:
> > On Thu, Oct 06, 2022 at 03:36:12PM +0200, Thomas Monjalon wrote:
> > > 05/10/2022 18:34, Tyler Retzlaff:
> > > > On Wed, Oct 05, 2022 at 09:11:26AM -0700, Tyler Retzlaff wrote:
> > > > > > Newly added code can go to eal_common_thread.c rather than introduce a
> > > > > > new common/rte_thread.c file (or is there a rationale for this?).
> > > > > 
> > > > > i will make this change in the next revision. if anyone does object i
> > > > > hope they will do so quickly.
> > > > 
> > > > looking at this more closely i'm going to back away from making the
> > > > adjustment here. if Thomas and/or Dmitry could comment it would be
> > > > appreciated.
> > > > 
> > > > it appears that functions placed in eal_common_xxx files are consumed
> > > > internally by the eal where rte_xxx files are functions that are exposed
> > > > through public api.
> > > 
> > > It is not so clear.
> > > There is already eal_common_thread.c which implements the same kind of functions,
> > > so I think you should move your new functions here.
> > > 
> > > > since these additions are public api it seems they should remain in
> > > > rte_thread.c
> > > 
> > > Let's not have 2 .c files for the same purpose in the same directory.
> > 
> > just as another point there seem to be several other rte_xxx.c files
> > here can we clarify why they were not subject to the same requirement?
> > as a follow on does it mean that the code in those files should also be
> > moved to eal_common_xxx files?
> 
> That's just history.
> 
> > please let me know if the justification is not the same i'll move the
> > functions to the eal_common file as requested. i just want to make sure
> > it is being done for the consistent/correct reason.
> 
> Some file names are not correct, we could rename them.
> 
> I think David is already doing the last minor changes on this series
> while merging, so no need to do anything on your side.
> 

Thomas, David with this just a final confirmation no need for a v6?
you're tweaking the series as is for final comments?

thanks
  

On Thu, Oct 6, 2022 at 5:20 PM Tyler Retzlaff
<roretzla@linux.microsoft.com> wrote:
>
> On Thu, Oct 06, 2022 at 05:14:55PM +0200, Thomas Monjalon wrote:
> > 06/10/2022 17:10, Tyler Retzlaff:
> > > On Thu, Oct 06, 2022 at 03:36:12PM +0200, Thomas Monjalon wrote:
> > > > 05/10/2022 18:34, Tyler Retzlaff:
> > > > > On Wed, Oct 05, 2022 at 09:11:26AM -0700, Tyler Retzlaff wrote:
> > > > > > > Newly added code can go to eal_common_thread.c rather than introduce a
> > > > > > > new common/rte_thread.c file (or is there a rationale for this?).
> > > > > >
> > > > > > i will make this change in the next revision. if anyone does object i
> > > > > > hope they will do so quickly.
> > > > >
> > > > > looking at this more closely i'm going to back away from making the
> > > > > adjustment here. if Thomas and/or Dmitry could comment it would be
> > > > > appreciated.
> > > > >
> > > > > it appears that functions placed in eal_common_xxx files are consumed
> > > > > internally by the eal where rte_xxx files are functions that are exposed
> > > > > through public api.
> > > >
> > > > It is not so clear.
> > > > There is already eal_common_thread.c which implements the same kind of functions,
> > > > so I think you should move your new functions here.
> > > >
> > > > > since these additions are public api it seems they should remain in
> > > > > rte_thread.c
> > > >
> > > > Let's not have 2 .c files for the same purpose in the same directory.
> > >
> > > just as another point there seem to be several other rte_xxx.c files
> > > here can we clarify why they were not subject to the same requirement?
> > > as a follow on does it mean that the code in those files should also be
> > > moved to eal_common_xxx files?
> >
> > That's just history.
> >
> > > please let me know if the justification is not the same i'll move the
> > > functions to the eal_common file as requested. i just want to make sure
> > > it is being done for the consistent/correct reason.
> >
> > Some file names are not correct, we could rename them.
> >
> > I think David is already doing the last minor changes on this series
> > while merging, so no need to do anything on your side.
> >
>
> Thomas, David with this just a final confirmation no need for a v6?
> you're tweaking the series as is for final comments?

No need for a v6, the code move is trivial, and for the rest, I'm finished.
I'll restart the per patch build all over again to be sure, and then
merge the series probably tonight (CET+2).
  

On Thu, Oct 06, 2022 at 05:26:10PM +0200, David Marchand wrote:
> On Thu, Oct 6, 2022 at 5:20 PM Tyler Retzlaff
> <roretzla@linux.microsoft.com> wrote:
> >
> > On Thu, Oct 06, 2022 at 05:14:55PM +0200, Thomas Monjalon wrote:
> > > 06/10/2022 17:10, Tyler Retzlaff:
> > > > On Thu, Oct 06, 2022 at 03:36:12PM +0200, Thomas Monjalon wrote:
> > > > > 05/10/2022 18:34, Tyler Retzlaff:
> > > > > > On Wed, Oct 05, 2022 at 09:11:26AM -0700, Tyler Retzlaff wrote:
> > > > > > > > Newly added code can go to eal_common_thread.c rather than introduce a
> > > > > > > > new common/rte_thread.c file (or is there a rationale for this?).
> > > > > > >
> > > > > > > i will make this change in the next revision. if anyone does object i
> > > > > > > hope they will do so quickly.
> > > > > >
> > > > > > looking at this more closely i'm going to back away from making the
> > > > > > adjustment here. if Thomas and/or Dmitry could comment it would be
> > > > > > appreciated.
> > > > > >
> > > > > > it appears that functions placed in eal_common_xxx files are consumed
> > > > > > internally by the eal where rte_xxx files are functions that are exposed
> > > > > > through public api.
> > > > >
> > > > > It is not so clear.
> > > > > There is already eal_common_thread.c which implements the same kind of functions,
> > > > > so I think you should move your new functions here.
> > > > >
> > > > > > since these additions are public api it seems they should remain in
> > > > > > rte_thread.c
> > > > >
> > > > > Let's not have 2 .c files for the same purpose in the same directory.
> > > >
> > > > just as another point there seem to be several other rte_xxx.c files
> > > > here can we clarify why they were not subject to the same requirement?
> > > > as a follow on does it mean that the code in those files should also be
> > > > moved to eal_common_xxx files?
> > >
> > > That's just history.
> > >
> > > > please let me know if the justification is not the same i'll move the
> > > > functions to the eal_common file as requested. i just want to make sure
> > > > it is being done for the consistent/correct reason.
> > >
> > > Some file names are not correct, we could rename them.
> > >
> > > I think David is already doing the last minor changes on this series
> > > while merging, so no need to do anything on your side.
> > >
> >
> > Thomas, David with this just a final confirmation no need for a v6?
> > you're tweaking the series as is for final comments?
> 
> No need for a v6, the code move is trivial, and for the rest, I'm finished.
> I'll restart the per patch build all over again to be sure, and then
> merge the series probably tonight (CET+2).

acknowledged.

thanks very much!

> 
> 
> -- 
> David Marchand