Re: [Tarantool-patches] [PATCH v7 1/2] fiber: set diagnostics at madvise/mprotect failure

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the answers!

>>> @@ -1007,6 +1035,8 @@ fiber_stack_watermark_create(struct fiber *fiber)
>>>  static void
>>>  fiber_stack_destroy(struct fiber *fiber, struct slab_cache *slabc)
>>>  {
>>> +	static const int mprotect_flags = PROT_READ | PROT_WRITE;
>>> +
>>
>> Why is it static? From what I know, when it is static we don't
>> have a guarantee, that it won't occupy memory in the .data section.
> 
> it is not about memory occupation but rather init it once instead
> of doing so during function prologue. It actually depends on compiler
> and modern ones would simply optimize this assignment.

Sorry, I don't understand. What do you mean init it only once?
PROT_READ | PROT_WRITE is a constant. There is nothing to init or
to calculate. This is just a one constant integer, which does not
need to be saved into a variable before being passed to a function,
especially into a static variable.

> Actually there is one hidden idea -- R|W is initial flags the 'small'
> uses when allocates memory that's why I mention this variable in
> comment.

You don't really need a static variable to mention its value in a
comment. Just say that you use READ|WRITE because this is what
usually can be done with normal memory.

Otherwise I am missing something.

>> Even though here it is clearly not necessary. Wouldn't just const
>> be enough? Why was not it possible to leave these flags inlined in
>> fiber_mprotect() call? They are not used anywhere else. PROT_NONE,
>> for example, is left inlined.
> 
> PROT_NONE is special, it is unrelated to 'small'.

Strictly speaking, core/fiber.c can't assume anything about small
internals, including READ|WRITE flags. Because fiber is not a
part of small. But I propose not to overcomplicate this.

> 	Cyrill