Initialize main thread pthread structure #59

alexhoppus · 2023-07-03T09:22:15Z

It seems some steps were missing in pthread structure initialization. This patch does the following:

zeroes pthread memory (previously potentially contained garbage after uk_mem_align allocation)
initializes tsd
sets detach_state to joinable

It seems some steps were missing in pthread structure initialization. This patch does the following: - zeroes pthread memory (previously potentially contained garbage after uk_mem_align allocation) - initializes tsd - sets detach_state to joinable Signed-off-by: Aleksandr Iashchenko <Aleksandr.Iashchenko@opensynergy.com>

alexhoppus · 2023-07-03T09:22:43Z

previously discussed here unikraft/unikraft#961 (review)

regarding joinable for main thread please grep here https://pubs.opengroup.org/onlinepubs/009695399/functions/exec.html

skuenzer · 2023-07-04T23:49:43Z

__uk_init_tls.c

@@ -84,6 +84,7 @@
 * map + size ----------------------------------------------------------
 */

+static const size_t __uk_tsd_size = sizeof(void *) * PTHREAD_KEYS_MAX;


This could be also provided as a macro, no symbol is effectively required.

#define UK_TSD_SIZE (sizeof(void *) * PTHREAD_KEYS_MAX)

I did not check the whole musl code yet, sorry for these questions:

Is sizeof(void *) big enough to hold a TSD entry?

Isn't somewhere else a size definition that we should use instead (to avoid code-out-of-sync type of errors in the future)?

Do we need to allocate TSD here already? The dangerous part is, because __uk_copy_tls() is called on every TLS memory allocation that we crash at any TLS creation (which happens at any thread creation) if we run out of memory instead of returning ENOMEM.

Hi @skuenzer . Thank you for the comment.

This could be also provided as a macro, no symbol is effectively required.

Is sizeof(void *) big enough to hold a TSD entry?

Isn't somewhere else a size definition that we should use instead (to avoid code-out-of-sync type of errors in the future)?

Actually, I have not introduced this, that was before me, I just moved this line from below. That is how TSD was created for other threads. If we are not sure if that logic is correct, maybe we should target a different PR to fix this? The macro I can do of course if needed, but again I have not invented this.

Do we need to allocate TSD here already? The dangerous part is, because __uk_copy_tls() is called on every TLS memory allocation that we crash at any TLS creation (which happens at any thread creation) if we run out of memory instead of returning ENOMEM.

Sorry, I don't understand how __uk_copy_tls() relates to this. It is in __uk_init_tp()

Re: macro vs static const: I think it hardly matters; in general I'd go for the option of least change.
Re: __uk_copy_tls indeed it's not relevant; all the changed code is in __uk_init_tp which gets called only for the main thread.

Actually, I have not introduced this, that was before me, I just moved this line from below. That is how TSD was created for other threads. If we are not sure if that logic is correct, maybe we should target a different PR to fix this? The macro I can do of course if needed, but again I have not invented this.

True, sorry, it is a pure style preference. I usually do not like introducing a static const variable where a macro achieves the same. But it is true, you just moved it up. I leave it up to you if you want to change it or leave it. I am just less hesitant for changes here (but normally I agree with @andreittr) because this file is purely our code.

Do we need to allocate TSD here already? The dangerous part is, because __uk_copy_tls() is called on every TLS memory allocation that we crash at any TLS creation (which happens at any thread creation) if we run out of memory instead of returning ENOMEM.

Sorry, I don't understand how __uk_copy_tls() relates to this. It is in __uk_init_tp()

In fact, my fault. It seems I was in a rush last time. Forget this comment. Btw, is there a way to return and propagate an error out of __uk_init_tp() instead of crashing?

Btw, is there a way to return and propagate an error out of __uk_init_tp() instead of crashing?

If we propagate this error up, it will end up in ukarch_tls_tcb_init, which then requires changes to both lib-musl and the Unikraft code that calls it. I am also not sure what the calling code could do to fix this, other then crash in Unikraft.

Btw, is there a way to return and propagate an error out of __uk_init_tp() instead of crashing?

We can propagate, but it is not clear for what purpose. If we can't allocate tsd even for init thread, then it is a fatal error and we can't recover from that.

I usually do not like introducing a static const variable where a macro achieves the same. But it is true, you just moved it up. I leave it up to you if you want to change it or leave it.

I prefer to leave as is.

Yes, this makes sense, and I think we should change the return type of __uk_init_tp from int to void.

razvand · 2023-08-05T05:52:09Z

@alexhoppus , @kubanrob , could you please ensure this PR is updated in the next days to have it part of the 0.14 release?

kubanrob · 2023-08-05T07:40:13Z

@razvand To be honest I am not sure what exactly needs to be changed given @alexhoppus and @andreittr answers.

@skuenzer Please review again given the answers. To me it seems like the requested three changes are not really needed / desired.

macro

@skuenzer

Is sizeof(void *) big enough to hold a TSD entry?

Isn't somewhere else a size definition that we should use instead (to avoid code-out-of-sync type of errors in the future)?

@alexhoppus

Actually, I have not introduced this, that was before me, I just moved this line from below. That is how TSD was created for other threads.

@andreittr

macro vs static const: I think it hardly matters; in general I'd go for the option of least change.

TSD

@skuenzer

Do we need to allocate TSD here already? The dangerous part is, because __uk_copy_tls() is called on every TLS memory allocation that we crash at any TLS creation

@alexhoppus

__uk_copy_tls() relates to this. It is in __uk_init_tp()

@andreittr

__uk_copy_tls indeed it's not relevant; all the changed code is in __uk_init_tp which gets called only for the main thread.

andreittr · 2023-08-08T17:32:35Z

@razvand To be honest I am not sure what exactly needs to be changed given @alexhoppus and @andreittr answers.

No changes per se, but there are some pending questions in the latest comment from @skuenzer and in my review above.
@alexhoppus could you please have a look?

alexhoppus · 2023-08-08T19:20:29Z

but there are some pending questions in the latest comment from @skuenzer and in my review above.

@andreittr I answered about error propagation and macro vs static const questions. I don't see any other questions. Please let me know if I missed something. Thank you.

andreittr · 2023-07-26T08:08:28Z

__uk_init_tls.c

+	 * - information about stack
+	 * - canary
+	 */
+	memset(td, 0, sizeof(*td));


Is a complete zeroing out of memory ok here? The init function called for other threads (uk_thread_uktcb_init) seems to set a few important fields (stack & self pointer); are these set somewhere else for the main thread?

In the interest of not repeating oneself, we shouldn't duplicate the tcb/tsd init code here; we likely can't piggyback on top of uk_thread_uktcb_init and call it directly, but can we factor out the tcb/td init code into a subroutine called by both?

The init function called for other threads (uk_thread_uktcb_init) seems to set a few important fields (stack & self pointer)

Yes, there are several things which are not initialized, that is specified in the FIXME block above memset. self pointer is set here. The stack is not set, I have not found an easy way to propagate this information to __uk_init_tp and that will affect related pthread api which retrieves stack information, however that also didn't work before. There is a way to do that, and that relates to your second question.

we likely can't piggyback on top of uk_thread_uktcb_init

I think that is exactly what we should do, but to do that properly, I assume we should also propagate / initialize earlier uk_thread for main thread Ideally, the mechanic for creating init thread & native threads should be the same and that might be the target for the refactoring. The other approaches like creating some subroutine unifying parts of uk_thread_uktcb_init and __uk_init_tp I don't like. My suggestion is to merge this and then to target a separate pr for refactoring. But in case if you insist, I can do that in this PR, however, as I said I don't see it as a small change,so I'm not sure that will be in line with the purpose of PR (bugfix).

Hum... it sounds like that we probably should brainstorm and discuss the initialization unification. I am not sure what you have in mind for unifying it for the init thread with a refactoring of uk_thread_uktcb_init but I am curious. I agree with you that this would be beyond the purpose of this PR. On the other hand, I think, @andreittr just commented about a smaller deduplication work within this file for improving the code readability without changing the model right now which also sounds okay to me. Maybe you can explain a bit more why you don't like it.

For full transparency of my opinion: I rather accept and merge the PR "as is" before not having any fix as part of the upcoming release. If some changes need bigger discussions and brainstorming, I am all for a meeting after the release to discuss a solution. On the other hand, minor things that are clear to do, we should try to fix with this PR.

What do you think @alexhoppus , @andreittr , @eduardvintila ?

On the other hand, I think, @andreittr just commented about a smaller deduplication work within this file for improving the code readability without changing the model right now which also sounds okay to me. Maybe you can explain a bit more why you don't like it.

There should be possible to make common API (unified function) which could be used for all native threads tcb initialization (including init thread). Maybe that will be just uk_thread_uktcb_init or some other function it is not clear to me yet. Also it makes sense to check what else could we unify on the paths of native threads/init thread initialization. However, that requires some work with external code also, so can't be done in isolated manner. Unifying parts of uk_thread_uktcb_init and __uk_init_tp now looks like a half-measure to me. Anyway, as I said, if you or @andreittr think differently I will do. I don't have particular plan, all above just generic thoughts.

Thank you.

Okay, from my side I think it makes sense to investigate more time after the release for finding a solution. You have my okay for this PR. @andreittr would you prefer restructuring the functions with this PR or we also move this to be discussed as part of the brainstorming as @alexhoppus suggested?

I'm ok with merging this fix as-is, seeing that it doesn't break anything, and revisit refactoring later.

andreittr · 2023-08-09T07:13:17Z

but there are some pending questions in the latest comment from @skuenzer and in my review above.

@andreittr I answered about error propagation and macro vs static const questions. I don't see any other questions. Please let me know if I missed something. Thank you.

Oops, my bad, I had left my review in draft form and it wouldn't show to others; fixed now, my 2 questions should now be visible.

skuenzer

As discussed, let's revisit this code base after a brainstorming discussion about TLS initialization. I am okay with integrating it "as is".

Reviewed-by: Simon Kuenzer simon@unikraft.io

andreittr

As discussed, approved as-is, will pursue refactoring later.

Reviewed-by: Andrei Tatar andrei@unikraft.io

kubanrob

Thank you @alexhoppus

Reviewed-by: Robert Kuban robert.kuban@opensynergy.com

razvand

Approved-by: Razvan Deaconescu razvand@unikraft.io

alexhoppus mentioned this pull request Jul 3, 2023

lib/ukboot: Zero out main thread TLS unikraft/unikraft#961

Closed

4 tasks

razvand requested a review from eduardvintila July 4, 2023 09:37

razvand assigned eduardvintila Jul 4, 2023

razvand requested review from andreittr and flpostolache and removed request for eduardvintila July 4, 2023 09:38

razvand added the enhancement New feature or request label Jul 4, 2023

razvand added this to the v0.14.0 (Prometheus) milestone Jul 4, 2023

skuenzer reviewed Jul 4, 2023

View reviewed changes

kubanrob requested a review from skuenzer July 11, 2023 09:38

andreittr reviewed Aug 9, 2023

View reviewed changes

kubanrob requested a review from andreittr August 9, 2023 11:11

skuenzer approved these changes Aug 10, 2023

View reviewed changes

andreittr approved these changes Aug 10, 2023

View reviewed changes

kubanrob approved these changes Aug 10, 2023

View reviewed changes

razvand assigned razvand and unassigned eduardvintila Aug 10, 2023

razvand approved these changes Aug 10, 2023

View reviewed changes

unikraft-bot closed this in 46ae9e3 Aug 10, 2023

unikraft-bot added the ci/merged label Aug 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initialize main thread pthread structure #59

Initialize main thread pthread structure #59

alexhoppus commented Jul 3, 2023

alexhoppus commented Jul 3, 2023 •

edited

Loading

skuenzer Jul 4, 2023 •

edited

Loading

alexhoppus Jul 5, 2023 •

edited

Loading

andreittr Jul 26, 2023

skuenzer Aug 8, 2023

kubanrob Aug 8, 2023 •

edited

Loading

alexhoppus Aug 8, 2023

skuenzer Aug 9, 2023

razvand commented Aug 5, 2023

kubanrob commented Aug 5, 2023

andreittr commented Aug 8, 2023

alexhoppus commented Aug 8, 2023 •

edited

Loading

andreittr Jul 26, 2023

alexhoppus Aug 9, 2023 •

edited

Loading

skuenzer Aug 9, 2023 •

edited

Loading

alexhoppus Aug 10, 2023 •

edited

Loading

skuenzer Aug 10, 2023

andreittr Aug 10, 2023

andreittr commented Aug 9, 2023

skuenzer left a comment

andreittr left a comment

kubanrob left a comment •

edited

Loading

razvand left a comment

Initialize main thread pthread structure #59

Initialize main thread pthread structure #59

Conversation

alexhoppus commented Jul 3, 2023

alexhoppus commented Jul 3, 2023 • edited Loading

skuenzer Jul 4, 2023 • edited Loading

Choose a reason for hiding this comment

alexhoppus Jul 5, 2023 • edited Loading

Choose a reason for hiding this comment

andreittr Jul 26, 2023

Choose a reason for hiding this comment

skuenzer Aug 8, 2023

Choose a reason for hiding this comment

kubanrob Aug 8, 2023 • edited Loading

Choose a reason for hiding this comment

alexhoppus Aug 8, 2023

Choose a reason for hiding this comment

skuenzer Aug 9, 2023

Choose a reason for hiding this comment

razvand commented Aug 5, 2023

kubanrob commented Aug 5, 2023

macro

TSD

andreittr commented Aug 8, 2023

alexhoppus commented Aug 8, 2023 • edited Loading

andreittr Jul 26, 2023

Choose a reason for hiding this comment

alexhoppus Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

skuenzer Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

alexhoppus Aug 10, 2023 • edited Loading

Choose a reason for hiding this comment

skuenzer Aug 10, 2023

Choose a reason for hiding this comment

andreittr Aug 10, 2023

Choose a reason for hiding this comment

andreittr commented Aug 9, 2023

skuenzer left a comment

Choose a reason for hiding this comment

andreittr left a comment

Choose a reason for hiding this comment

kubanrob left a comment • edited Loading

Choose a reason for hiding this comment

razvand left a comment

Choose a reason for hiding this comment

alexhoppus commented Jul 3, 2023 •

edited

Loading

skuenzer Jul 4, 2023 •

edited

Loading

alexhoppus Jul 5, 2023 •

edited

Loading

kubanrob Aug 8, 2023 •

edited

Loading

alexhoppus commented Aug 8, 2023 •

edited

Loading

alexhoppus Aug 9, 2023 •

edited

Loading

skuenzer Aug 9, 2023 •

edited

Loading

alexhoppus Aug 10, 2023 •

edited

Loading

kubanrob left a comment •

edited

Loading