On Tue, Sep 27, 2016 at 12:00 PM, Nadav Har'El <[email protected]> wrote:
> > On Mon, Sep 19, 2016 at 2:59 PM, Nadav Har'El <[email protected]> wrote: > >> This patch prevents on-stack sched::thread construction by hiding the >> constructor, and only allowing a thread to be created through the function >> sched::thread::make(...), which creates the object on the heap. >> > > I had a chat about this with Avi. > > He would prefer a different solution, which I previously considered but > thought would be too much work to implement... > > The idea is that the sched::thread will contain nothing but a pointer to > the real thread structure. The user can create the sched::thread object > wherever he pleases, but the constructor will allocate the internal thread > structure in the way we want (on the heap). > Avi, I've been thinking about this. I've been thinking quite a bit about the "pimpl" style solution (user-visible thread holds pointer to an "internal" thread structure). This is not too hard to do conceptually, but in my thinking I reached the following problems: 1. It will require large amounts of changes to sched.cc/sched.hh. 2. And there is a be a big question of what to do with the detached_state substructure. I will be silly to have two levels of indirection (thread pointing to internal_thread pointing to detached_state) so it would make sense to conflate two, namely to make the internal_thread structure the "detached state". However, this would mean that most of the code (even in sched.cc) will still need to work on the external "thread" object, not the internal_thread structure. Maybe that's not a big problem, thought. It just sounds a bit less efficient. 3. You wanted to avoid changing the user-visible sched::thread API. However, most code (including, for example, mutexes) work with a "thread*". But now that a thread will only contain a pointer, why do we need a pointer to that pointer? Maybe like in pthreads, the user-visible object ("thread_t") should be officially a copyable and transfered-by-value handle. This will require tons of changes to OSv (even more than in my original patch). Because of all these reasons, I wonder if my patch isn't simpler and "good enough"? Avi, Timmons, what is your opinion on whether we should go with my patch (in this thread), or the other two approaches (Timmons' or Avi's). > > We actually already have exactly this sort of indirection in > sched::thread, and it is detached_state, which we needed to hold parts of > the thread that we needed to survive the thread just a bit, until the RCU > quiet period. So basically now we'll move *all* the thread's state into > this detached state. The sched::thread destructor will rcu_dispose() the > real thread structure rather than delete it immediately, but can call > immediately a new method which cleans up part of the thread (like its > stack) which we are sure we no longer need. > > Most of the private "thread" methods will become methods on the new > detached state structure (need a new name for it, maybe thread_impl) and > scheduler code which currently handles thread pointers - e.g., the > scheduler's run queue - will now have pointers directly to this thread_impl > - so the new scheme will not hurt scheduling performance, even the opposite > (currently a few of the thread's fields need to be indirected through > "detached_state" and this will go away). > -- You received this message because you are subscribed to the Google Groups "OSv Development" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
