Make thread creation work for CloudABI processes.

[ed]

Summary

Remove the stub system call that was put in place during the system call
import and replace it by a target-dependent version stored in sys/amd64.
Initialize the thread in a way similar to cpu_set_upcall_kse(). We
provide the entry point with two arguments: the thread ID and the
argument pointer.

Test Plan

Thread creation still seems to work, both for FreeBSD and CloudABI
binaries.

[kib]

I like the changes to the kern_thr.c in principle. Also, I think they are low risk, so it is desirable to have them merged to stable/10 for easier MFCs.

Please split the kern_thr.c (and required changes e.g. to proc.h) into separate commit.

sys/kern/kern_thr.c

102 ↗

(On Diff #7022)

The function was very confusing to understand.

From some time, the FreeBSD conventions is that sys_something implements something syscall. Do not use sys_ prefix.

104 ↗

(On Diff #7022)

Style. No initialization at the declaration.

[ed]

In D3110#61749, @kib wrote:

I like the changes to the kern_thr.c in principle. Also, I think they are low risk, so it is desirable to have them merged to stable/10 for easier MFCs.

Please split the kern_thr.c (and required changes e.g. to proc.h) into separate commit.

That sounds like a good idea. I'll make sure to do that.

sys/kern/kern_thr.c

102 ↗

(On Diff #7022)

I've removed the kern_ and sys_ prefixes from the function and struct names. Does this look any better?

[ed]

Remove confusing prefixes from the function names.

[ed]

Remove another initialization.

[kib]

I am fine with the kern_thr.c part of the change.

[ed]

Remove changes to kern_thr.c and proc.h that have been committed.

[ed]

In D3110#61842, @kib wrote:

I am fine with the kern_thr.c part of the change.

Perfect. I've just committed those, so this code review now only contains the CloudABI specific bits.

[kib]

I have two notes about the rest of the patch. I do not like both issues, but this is your code, so I do not insist on any change.

First, the cloudabi64_sys_thread_create() is MI, but it is located in arch-specific file.

Second, the initialize_thread() is the copy of cpu_set_upcall_kse(), with very minor editing.

[ed]

In D3110#61880, @kib wrote:

I have two notes about the rest of the patch. I do not like both issues, but this is your code, so I do not insist on any change.

First, the cloudabi64_sys_thread_create() is MI, but it is located in arch-specific file.

Exactly. It's suboptimal, but right now it's still compact enough that I think that it doesn't warrant a separate source + header file.

Second, the initialize_thread() is the copy of cpu_set_upcall_kse(), with very minor editing.

Yes. I'm still torn what would be the best approach here. For x86-64 we could just call into cpu_set_upcall_kse() and fix up the registers later on. The problem is that this approach is that it does become more sloppy for architectures where we need to push the arguments on the stack instead.

We could likely solve this by decomposing cpu_set_upcall_kse() into a function that just clears the registers and a separate one to do the setting of the arguments. At the same time we could consider renaming cpu_set_upcall_kse(), because it looks like this function is in no way related to KSE. Does that make sense, or is it not worth the effort?

[kib]

In D3110#61881, @ed wrote:

In D3110#61880, @kib wrote:

I have two notes about the rest of the patch. I do not like both issues, but this is your code, so I do not insist on any change.

First, the cloudabi64_sys_thread_create() is MI, but it is located in arch-specific file.

Exactly. It's suboptimal, but right now it's still compact enough that I think that it doesn't warrant a separate source + header file.

Why do you need some separate source ? And header ? Even if you need, it is better to introduce them, than to have confusing arrangements, IMO.

Second, the initialize_thread() is the copy of cpu_set_upcall_kse(), with very minor editing.

Yes. I'm still torn what would be the best approach here. For x86-64 we could just call into cpu_set_upcall_kse() and fix up the registers later on. The problem is that this approach is that it does become more sloppy for architectures where we need to push the arguments on the stack instead.

We could likely solve this by decomposing cpu_set_upcall_kse() into a function that just clears the registers and a separate one to do the setting of the arguments. At the same time we could consider renaming cpu_set_upcall_kse(), because it looks like this function is in no way related to KSE. Does that make sense, or is it not worth the effort?

Renaming cpu_set_upcall_kse() is a lot of work, you should change all arches, for almost no return. I do not object against it, but I also would not spend time.

Reducing duplication by splitting amd64' cpu_set_upcall_kse() might be useful, might be not. It depends on the outcome.

My reaction on this part of the patch is mostly caused by MI/MD confusion for the syscall.

[ed]

Make the system call machine-independent.

Introduce a new function, cloudabi64_thread_setregs(), that will be
called to initialize the thread registers.

[ed]

In D3110#61997, @kib wrote:

My reaction on this part of the patch is mostly caused by MI/MD confusion for the syscall.

Does this version look better?

[kib]

sys/compat/cloudabi64/cloudabi64_thread.c

66 ↗

(On Diff #7132)

if (error == 0) td->td_retval[0] = args.tid; ?