core: Allocate threads on demand, not on scheduler startup

[Blei]

API change: rust_kernel::create_scheduler() or
rust_scheduler::rust_scheduler() respecitevly now take ownership of the
launch factory argument, it is needed to create new threads on demand.

Also renames rustrt::sched_threads() to rustrt::rust_sched_threads() for
consistency. Added rustrt::rust_max_sched_threads() to return the
maximal number of scheduled threads of the current scheduler.

Fixes #3493.

I'm not sure if the launch factory argument should be kernel-allocated (like the scheduler) or not. At the moment it's a simple heap allocation, please tell me if should change that.

[brson]

Thank you! This will take a little time to review.

[graydon]

Neat! Do I understand correctly that this allocates one thread per task until it hits the maximum, then assigns the new tasks to existing threads by round robin? This sounds reasonably fair, I can't think of many downsides off hand. Does it address a particular performance issue, or did you just not like seeing 7 idle threads when running on an 8-way machine? :)

[brson]

@graydon yes that is what it does.

Based on the conversation we had when we were discussing using a queue for scheduling tasks, I now think that round robin is probably the wrong way to assign tasks to threads, because it creates a consistent but somewhat arbitrary pattern, that some code could conceivably rely on. In the same way, picking unused threads to schedule on could have implications that I haven't considered.

The main motivation is that recently I was considering how many stacks we allocate just to start the runtime, and it's a lot.

FWIW, in a future where we have work stealing and task zygotes (tasks that begin running in their parent's 'context' in the hopes that they will be short lived) I expect all tasks to be spawned onto their parent's thread by default.

[brson]

@Blei my big concern with this patch is the locking around threads.size(). Now that the set of live threads can change dynamically we shouldn't be calling size() without a lock. In some cases it is already protected by the scheduler lock, but in others it is being called without the lock. The locking in the scheduler can be a little tricky, especially around kill_all_tasks. I'll put further comments inline.

[brson]

In rust_sched_loop.h we should probably add a comment saying that modifying threads must be protected by lock. I believe other fields mention their locking requirements already.

[brson]

Also, live_threads must be protected by the lock now, should be mentioned in the header

[brson]

@bblum you may be interested in taking a look at this

[Blei]

Thanks for the review!

I renamed rust_max_sched_threads to rust_sched_threads and rust_sched_threads in turn to rust_sched_current_nonlazy_threads.

The slightly weird if condition in the thread allocation is needed to catch the case that we allocate 1 thread in the constructor which doesn't have a task yet (this was covered by checking for "empty" threads in the previous patch). If we remove the allocation of a thread in the constructor, there is some code that fails with an assertion when it wants to get the driver out of a rust_manual_sched_launcher_factory right after creating the scheduler.

I don't know what the proper way to deal with "v2" patches is in github pull requests. Amending/squashing together with push -f destroys previous inline comments. Should I open a new push request with the squashed changes when all issues have been worked out?

[brson]

A second pull request would be nice if you plan to squash, but this two-commit history looks fine to me as-is. Besides the squashing are you happy with these commits? They look good to me now.

[Blei]

I was worried of knowingly introducing incorrect revisions into the repo. But I guess that shouldn't really be a big problem here. If you don't see any further problems, this branch is ready to pull :)

[brson]

Merged!

FWIW, if there were commits that didn't build or that were likely to cause significant test failures then I would not want to leave them in the history, but the locking in v2 mostly protects against some underrepresented corner-cases.