Re: [PATCH] sched: only allocate sched::thread objects on the heap

[Avi Kivity]

On 10/05/2016 12:45 PM, Nadav Har'El wrote:


On Tue, Sep 27, 2016 at 12:00 PM, Nadav Har'El > wrote:



On Mon, Sep 19, 2016 at 2:59 PM, Nadav Har'El mailto:n...@scylladb.com>> 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).


It's certainly not worth huge churn, we can go with your patch.
\




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 osv-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH] sched: only allocate sched::thread objects on the heap

[Nadav Har'El]

On Tue, Sep 27, 2016 at 12:00 PM, Nadav Har'El  wrote:

>
> On Mon, Sep 19, 2016 at 2:59 PM, Nadav Har'El  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 osv-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH] sched: only allocate sched::thread objects on the heap

[Nadav Har'El]

On Mon, Sep 19, 2016 at 2:59 PM, Nadav Har'El  wrote:

> In issue #790, Timmons C. Player discovered something dangerous with
> on-stack sched::thread objects: The stack may be mmap()ed, so the scheduler
> now needs to access an application's mmap()ed memory area. Those accesses
> can potentially have all sort of problems (like page faults in the
> scheduler,
> if it weren't for issue #143), but more concretely: The "lazy TLB flush"
> code added in commit 7e38453 means that the scheduler may see old pages
> for application-mmap()ed pages, so it cannot work with a sched::thread in
> an
> mmap()ed area.
>
> 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).

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 osv-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[PATCH] sched: only allocate sched::thread objects on the heap

[Nadav Har'El]

In issue #790, Timmons C. Player discovered something dangerous with
on-stack sched::thread objects: The stack may be mmap()ed, so the scheduler
now needs to access an application's mmap()ed memory area. Those accesses
can potentially have all sort of problems (like page faults in the scheduler,
if it weren't for issue #143), but more concretely: The "lazy TLB flush"
code added in commit 7e38453 means that the scheduler may see old pages
for application-mmap()ed pages, so it cannot work with a sched::thread in an
mmap()ed area.

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.

This patch is long but it more-or-less contains just the following
types of changes:

1. The change to sched.hh to hide the sched::thread constructor, and
   instead offer a sched::thread::make(...) function.

2. Every class which used a "sched::thread" member was replaced by
   a std::unique_ptr member.

   One "casualty" of this change is that a class that used to hold a
   sched::thread member will now hold a sched::thread pointer, and use an
   extra allocation and indirection - even if we know for sure that the
   containing object will always be allocated on the heap (such as the
   case in pthread.cc's pthread::_thread, for example). This can be worked
   around by making the exceptional class a friend of sched::thread to be
   able to use the hidden constructor - but for now I decided the tiny
   performance benefit isn't important enough to make this exception.

3. Every place which used "new sched::thread(...)" to create the thread,
   now uses "sched::thread::make(...)" instead.

   Sadly we no longer allow new sched::thread(...), although there is
   nothing wrong with it, because disabling the constructor also disables
   the new expression.

4. One test in misc-wake.cc (which wasn't enabled by default anyway)
   was commented out because it relied on creating thread objects in
   mmap()ed areas.

One of the places modified is rcu_flush(), whose on-stack sched::thread
objects are what caused issue #790.

Fixes #790.

Signed-off-by: Nadav Har'El 
---
 drivers/virtio-net.hh |  6 ++--
 drivers/virtio-rng.hh |  2 +-
 drivers/vmxnet3.hh|  4 +--
 include/osv/mempool.hh|  2 +-
 include/osv/percpu_xmit.hh|  2 +-
 include/osv/sched.hh  | 12 
 tests/tst-bsd-synch.hh| 12 
 tests/tst-malloc.hh   |  4 +--
 tests/tst-rwlock.hh   |  8 ++---
 tests/tst-threads.hh  |  4 +--
 tests/tst-timer.hh|  2 +-
 arch/x64/xen_intr.cc  |  2 +-
 bsd/porting/callout.cc|  2 +-
 bsd/porting/kthread.cc|  4 +--
 bsd/sys/net/netisr1.cc|  2 +-
 core/async.cc | 12 
 core/dhcp.cc  |  2 +-
 core/mempool.cc   | 28 -
 core/pagecache.cc |  6 ++--
 core/percpu-worker.cc |  2 +-
 core/rcu.cc   | 20 ++---
 core/sched.cc | 26 
 core/trace.cc |  8 ++---
 drivers/acpi.cc   | 12 
 drivers/ahci.cc   |  2 +-
 drivers/console-driver.cc |  2 +-
 drivers/virtio-assign.cc  |  2 +-
 drivers/virtio-blk.cc |  2 +-
 drivers/virtio-net.cc |  2 +-
 drivers/virtio-rng.cc |  6 ++--
 drivers/virtio-scsi.cc|  2 +-
 drivers/vmw-pvscsi.cc |  2 +-
 drivers/vmxnet3.cc|  4 +--
 libc/pthread.cc   | 28 -
 libc/signal.cc|  4 +--
 libc/timerfd.cc   | 10 +++
 tests/misc-free-perf.cc   |  2 +-
 tests/misc-leak.cc|  4 +--
 tests/misc-lfring.cc  | 12 
 tests/misc-loadbalance.cc |  2 +-
 tests/misc-mutex.cc   |  2 +-
 tests/misc-sockets.cc |  8 ++---
 tests/misc-tcp-hash-srv.cc|  2 +-
 tests/misc-wake.cc|  2 ++
 tests/tst-af-local.cc | 16 +-
 tests/tst-bsd-tcp1-zrcv.cc|  4 +--
 tests/tst-bsd-tcp1-zsnd.cc|  4 +--
 tests/tst-bsd-tcp1-zsndrcv.cc |  4 +--
 tests/tst-bsd-tcp1.cc |  4 +--
 tests/tst-condvar.cc  | 10 +++
 tests/tst-fpu.cc  |  6 ++--
 tests/tst-mmap.cc |  4 +--
 tests/tst-pin.cc  | 70 +--
 tests/tst-preempt.cc  |  2 +-
 tests/tst-rcu-hashtable.cc|  2 +-
 tests/tst-rcu-list.cc |  2 +-
 tests/tst-threadcomplete.cc   |  4 +--
 tests/tst-vfs.cc  |  2 +-
 tests/tst-wait-for.cc | 30 +--
 tests/tst-yield.cc|  2 +-
 60 files changed, 232 insertions(+), 218 deletions(-)

diff --git a/drivers/virtio-net.hh b/drivers/virtio-net.hh
index cf2d6f2..ad323e6 100644
--- a/drivers/virtio-net.hh
+++ b/drivers/virtio-net.hh
@@ -302,10 +302,10 @@ private: