[PATCH] lib/core/fiber: Initialize stack_watermark where appropriate

[PATCH] lib/core/fiber: Initialize stack_watermark where appropriate

[Cyrill Gorcunov]

The stack_watermark member declared with HAVE_MADV_DONTNEED wrap,
so need to guard it here the same way.

Fixes 553dc562342a52cb44d74a7521c9c8bec70c96a5
---
 src/lib/core/fiber.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
index 243a73bf8..b5033a32e 100644
--- a/src/lib/core/fiber.c
+++ b/src/lib/core/fiber.c
@@ -1076,7 +1076,10 @@ cord_create(struct cord *cord, const char *name)
 	cord->sched.stack = NULL;
 	cord->sched.stack_size = 0;
 #endif
+
+#ifdef HAVE_MADV_DONTNEED
 	cord->sched.stack_watermark = NULL;
+#endif
 }
 
 void
-- 
2.20.1

Re: [PATCH] lib/core/fiber: Initialize stack_watermark where appropriate

[Vladimir Davydov]

On Mon, Mar 18, 2019 at 08:23:52PM +0300, Cyrill Gorcunov wrote:
> The stack_watermark member declared with HAVE_MADV_DONTNEED wrap,
> so need to guard it here the same way.

Yep, overlooked it, my bad. I also lost fiber_stack_recycle() stub.
Weird, but it did compile on OS X as is. Looks like OS X supports
MADV_DONTNEED after all :)

Applied to 2.1 and 1.10 with fiber_stack_recycle() fix squashed.
Thanks!

> 
> Fixes 553dc562342a52cb44d74a7521c9c8bec70c96a5
> ---
>  src/lib/core/fiber.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
> index 243a73bf8..b5033a32e 100644
> --- a/src/lib/core/fiber.c
> +++ b/src/lib/core/fiber.c
> @@ -1076,7 +1076,10 @@ cord_create(struct cord *cord, const char *name)
>  	cord->sched.stack = NULL;
>  	cord->sched.stack_size = 0;
>  #endif
> +
> +#ifdef HAVE_MADV_DONTNEED
>  	cord->sched.stack_watermark = NULL;
> +#endif
>  }
>  
>  void

Re: [PATCH] lib/core/fiber: Initialize stack_watermark where appropriate

[Cyrill Gorcunov]

On Mon, Mar 18, 2019 at 08:52:46PM +0300, Vladimir Davydov wrote:
> On Mon, Mar 18, 2019 at 08:23:52PM +0300, Cyrill Gorcunov wrote:
> > The stack_watermark member declared with HAVE_MADV_DONTNEED wrap,
> > so need to guard it here the same way.
> 
> Yep, overlooked it, my bad. I also lost fiber_stack_recycle() stub.
> Weird, but it did compile on OS X as is. Looks like OS X supports
> MADV_DONTNEED after all :)
> 
> Applied to 2.1 and 1.10 with fiber_stack_recycle() fix squashed.
> Thanks!

Thanks! It was actually my nit in first place ;)

Re: [tarantool-patches] Re: [PATCH] lib/core/fiber: Initialize stack_watermark where appropriate

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/03/18 20:56]:

> On Mon, Mar 18, 2019 at 08:23:52PM +0300, Cyrill Gorcunov wrote:
> > The stack_watermark member declared with HAVE_MADV_DONTNEED wrap,
> > so need to guard it here the same way.

What is the reason to keep the poison pool 8 elements now that it
is only used to save on madvise() invocations and is put in a
random position? Shouldn't 1 element be enough? 

What is the actual stack size for 99.9% of cases? Why not put the
poison at 16kb of stack?  

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

Re: [tarantool-patches] Re: [PATCH] lib/core/fiber: Initialize stack_watermark where appropriate

[Cyrill Gorcunov]

On Mon, Mar 18, 2019 at 09:55:29PM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [19/03/18 20:56]:
> 
> > On Mon, Mar 18, 2019 at 08:23:52PM +0300, Cyrill Gorcunov wrote:
> > > The stack_watermark member declared with HAVE_MADV_DONTNEED wrap,
> > > so need to guard it here the same way.
> 
> What is the reason to keep the poison pool 8 elements now that it
> is only used to save on madvise() invocations and is put in a
> random position? Shouldn't 1 element be enough?

Even 8 elements are not guarantee us from false positives and
with a sole element the situation would be even worse.

> What is the actual stack size for 99.9% of cases? Why not put the
> poison at 16kb of stack?

Well, I think we can't answer this question without gathering
statistics.

Re: [tarantool-patches] Re: [PATCH] lib/core/fiber: Initialize stack_watermark where appropriate

[Konstantin Osipov]

* Cyrill Gorcunov <gorcunov@gmail.com> [19/03/18 22:07]:
> On Mon, Mar 18, 2019 at 09:55:29PM +0300, Konstantin Osipov wrote:
> > * Vladimir Davydov <vdavydov.dev@gmail.com> [19/03/18 20:56]:
> > 
> > > On Mon, Mar 18, 2019 at 08:23:52PM +0300, Cyrill Gorcunov wrote:
> > > > The stack_watermark member declared with HAVE_MADV_DONTNEED wrap,
> > > > so need to guard it here the same way.
> > 
> > What is the reason to keep the poison pool 8 elements now that it
> > is only used to save on madvise() invocations and is put in a
> > random position? Shouldn't 1 element be enough?
> 
> Even 8 elements are not guarantee us from false positives and
> with a sole element the situation would be even worse.

Well, then let's increase the number of elements to 32 or 64, or,
better yet, 128, to make the situation better.

Why did you guys choose 8?


-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

Re: [tarantool-patches] Re: [PATCH] lib/core/fiber: Initialize stack_watermark where appropriate

[Cyrill Gorcunov]

On Mon, Mar 18, 2019 at 10:10:08PM +0300, Konstantin Osipov wrote:
> > 
> > Even 8 elements are not guarantee us from false positives and
> > with a sole element the situation would be even worse.
> 
> Well, then let's increase the number of elements to 32 or 64, or,
> better yet, 128, to make the situation better.
> 
> Why did you guys choose 8?

To cover low 1024 bytes. Maximal path is about 4K limit
so I think in average ~1/4 of page should catch paths
on stack.

Re: [tarantool-patches] Re: [PATCH] lib/core/fiber: Initialize stack_watermark where appropriate

[Konstantin Osipov]

* Cyrill Gorcunov <gorcunov@gmail.com> [19/03/18 22:20]:
> On Mon, Mar 18, 2019 at 10:10:08PM +0300, Konstantin Osipov wrote:
> > > 
> > > Even 8 elements are not guarantee us from false positives and
> > > with a sole element the situation would be even worse.
> > 
> > Well, then let's increase the number of elements to 32 or 64, or,
> > better yet, 128, to make the situation better.
> > 
> > Why did you guys choose 8?
> 
> To cover low 1024 bytes. Maximal path is about 4K limit
> so I think in average ~1/4 of page should catch paths
> on stack.

Okay, well, that at least a plausible cause, I guess I'd be nice to
see it in the comment for the poison array. However, you could use
4 integers and a larger poison offset?-))

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

Re: [tarantool-patches] Re: [PATCH] lib/core/fiber: Initialize stack_watermark where appropriate

[Cyrill Gorcunov]

On Tue, Mar 19, 2019 at 12:19:19AM +0300, Konstantin Osipov wrote:
> > > 
> > > Well, then let's increase the number of elements to 32 or 64, or,
> > > better yet, 128, to make the situation better.
> > > 
> > > Why did you guys choose 8?
> > 
> > To cover low 1024 bytes. Maximal path is about 4K limit
> > so I think in average ~1/4 of page should catch paths
> > on stack.
> 
> Okay, well, that at least a plausible cause, I guess I'd be nice to
> see it in the comment for the poison array. However, you could use
> 4 integers and a larger poison offset?-))

Which decreases probability, but yes :) Kostya, do you think
4 writes vs 8 writes would give us that much benefit? Technically
it will be a couple of lines patch but... in the worst case where
we have all marks present but stack actually consumes more pages,
(and instead of shrinking it we wrongly "think" that we don't need
to call madvise), will lead to very bad consequences -- rss will
bloat as hell (just like Volodya pointed me about a stack-eager task
which jumps over all fibers in a system).