From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <kostja@tarantool.org>
Date: Tue, 5 Mar 2019 11:30:48 +0300
From: Konstantin Osipov <kostja@tarantool.org>
Subject: Re: [tarantool-patches] [rfc 4/4] core/fiber: Shrink stack when
 recycling
Message-ID: <20190305083048.GM21955@chai>
References: <20190302125524.279852704@gmail.com>
 <20190302130034.125890957@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190302130034.125890957@gmail.com>
To: tarantool-patches@freelists.org
Cc: gorcunov@gmail.com, vdavydov.dev@gmail.com
List-ID: <tarantool-patches.dev.tarantool.org>

* Cyrill Gorcunov <gorcunov@gmail.com> [19/03/03 23:25]:
> +#define wmark_freeze(_pp) do { *((uintptr_t *)(_pp)) |= (uintptr_t)1; } while (0)
> +#define wmark_frozen(_p) ((uintptr_t)(_p) & (uintptr_t)1)

Why not static inline? A comment would help. Usually the name for
function returning true/false contains "is", e.g.
wmark_is_frozen().


>  
>  static void
> +stack_shrink(struct fiber *fiber)
> +{
> +	void *hi, *lo;
> +
> +	if (stack_direction < 0) {
> +		hi = fiber->stack + fiber->stack_size;
> +		lo = fiber->stack_wmark_ofl + page_size;
> +	} else {
> +		hi = fiber->stack_wmark_ofl - page_size;
> +		lo = fiber->stack - fiber->stack_size;
> +	}
> +
> +	if (fiber->stack_wmark <= lo ||
> +	    fiber->stack_wmark >= hi)
> +		return;
> +
> +	madvise(fiber->stack_wmark, page_size, MADV_DONTNEED);
> +	if (stack_direction < 0)
> +		fiber->stack_wmark += page_size;
> +	else
> +		fiber->stack_wmark -= page_size;
> +	stack_put_wmark(fiber);
> +}
> +
> +static void
>  fiber_wmark_recycle(struct fiber *fiber)
>  {
>  	static bool overflow_warned = false;
> @@ -772,20 +800,30 @@ fiber_wmark_recycle(struct fiber *fiber)
>  
>  	/*
>  	 * We are watching for stack overflow in one shot way:
> -	 * firstly to not spam a user with messages and secondly
> -	 * to relax system from additional operations while our
> -	 * watermark is not dynamic.
> +	 * to not spam a user with messages.
>  	 */
> -	if (overflow_warned)
> -		return;
> -
> -	if (!stack_has_wmark(fiber->stack_wmark_ofl)) {
> -		if (!overflow_warned) {
> +	if (!overflow_warned) {
> +		if (!stack_has_wmark(fiber->stack_wmark_ofl)) {
>  			say_warn("fiber %d is close to a stack limit",
>  				 fiber->name, fiber->fid);
>  			overflow_warned = true;
>  		}
>  	}
> +
> +	if (wmark_frozen(fiber->stack_wmark))
> +		return;
> +
> +	/*
> +	 * On recycle we're trying to shrink stack
> +	 * as much as we can until first mark overwrite
> +	 * detected, then we simply freeze watermark and
> +	 * assume the stack is balanced and won't change
> +	 * much in future.
> +	 */
> +	if (!stack_has_wmark(fiber->stack_wmark))
> +		wmark_freeze(&fiber->stack_wmark);

I don't see how "frozen" watermark concept is more powerful than
already existing "overflow warned" flag. I think the idea is to
have some sort of smart threshold after which we stop trying to
shrink the stack.

Right now this threshold is not too smart (which is OK), but
complicated (both overflow_warned and wmark_frozen() are
considered), which doesn't make too much sense to me.

> +	else
> +		stack_shrink(fiber);

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