Message ID | 1656348966-10194-1-git-send-email-roretzla@linux.microsoft.com (mailing list archive) |
---|---|
Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@inbox.dpdk.org Delivered-To: patchwork@inbox.dpdk.org Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 4CF8DA054F; Mon, 27 Jun 2022 18:56:10 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id D32D240A8B; Mon, 27 Jun 2022 18:56:09 +0200 (CEST) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id D458240685 for <dev@dpdk.org>; Mon, 27 Jun 2022 18:56:08 +0200 (CEST) Received: by linux.microsoft.com (Postfix, from userid 1086) id 0433520CD168; Mon, 27 Jun 2022 09:56:08 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 0433520CD168 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1656348968; bh=fCnpJF5BbR1vko4wD56gsucEPLC53IgGlff9DgCEM8k=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=HOfmy7nC601du6UHGTwDTYpfzN7mJ5PURT67gatBds6fK6yZSIBI0DR4aFpiMtmv2 4Nvss2tQ10WOdWIouN9uLGn24dXFnr8/l6WBklSknhwszGxLT3hAnMwUyTUWrlMmfD a48mQGE9Er3pqNqsKFeB3sPkYN/eFasxgjJ/4nmY= From: Tyler Retzlaff <roretzla@linux.microsoft.com> To: dev@dpdk.org Cc: thomas@monjalon.net, dmitry.kozliuk@gmail.com, anatoly.burakov@intel.com, Tyler Retzlaff <roretzla@linux.microsoft.com> Subject: [PATCH v4 0/6] add thread lifetime and attributes API Date: Mon, 27 Jun 2022 09:56:00 -0700 Message-Id: <1656348966-10194-1-git-send-email-roretzla@linux.microsoft.com> X-Mailer: git-send-email 1.8.3.1 In-Reply-To: <1654783134-13303-1-git-send-email-roretzla@linux.microsoft.com> References: <1654783134-13303-1-git-send-email-roretzla@linux.microsoft.com> X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions <dev.dpdk.org> List-Unsubscribe: <https://mails.dpdk.org/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://mails.dpdk.org/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <https://mails.dpdk.org/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org |
Series |
add thread lifetime and attributes API
|
|
Message
Tyler Retzlaff
June 27, 2022, 4:56 p.m. UTC
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
Comments
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