[Tarantool-patches] [PATCH] say: fix format for fiber()->fid

[Vladislav Shpilevoy]

On 24.02.2021 09:28, Cyrill Gorcunov wrote:
> On Tue, Feb 23, 2021 at 11:47:44PM +0100, Vladislav Shpilevoy wrote:
>>>>> diff --git a/src/lib/core/say.c b/src/lib/core/say.c
>>>>> index cbd10e107..6138752a7 100644
>>>>> --- a/src/lib/core/say.c
>>>>> +++ b/src/lib/core/say.c
>>>>> @@ -792,7 +792,7 @@ say_format_plain_tail(char *buf, int len, int level, const char *filename,
>>>>>  	if (cord) {
>>>>>  		SNPRINT(total, snprintf, buf, len, " %s", cord->name);
>>>>>  		if (fiber() && fiber()->fid != FIBER_ID_SCHED) {
>>>>> -			SNPRINT(total, snprintf, buf, len, "/%i/%s",
>>>>> +			SNPRINT(total, snprintf, buf, len, "/%u/%s",
>>>>>  				fiber()->fid, fiber_name(fiber()));
>>>>
>>>> I remember we had some issues about %u not being compatible with uint32_t so
>>>> we did manual casts to 'unsigned'. Not sure though what was the commit/ticket,
>>>> or if it really happened.
>>>
>>> I suspect you rather mean uint64_t, which indeed better convert to
>>> excplicit "long long".
>>
>> No, I mean exactly uint32_t. You don't have a guarantee that it is
>> the same as 'unsigned' AFAIK.
> 
> It is, on x86.

In the close future we will build on the new ARM Macs. Simply because many
of our developers use Macs and they want the new models. All our assumptions
about x86 being the only platform should not be made anymore.

>>> Strictly speaking if we really need to cover
>>> compatibility we would _had_ to use PRIx macros which are so ugly :/
>>
>> This is why I want us to use casts to 'unsigned' when we use %u. They
>> are less ugly.
> 
> We will have to cover a bunch of already existing code for nothing :(
> But OK, I'll add explicit cast.

It is ok to keep the old code intact where the compiler won't give any
errors when you enable the compile time checking.

>> After more consideration I also realized we should not use uint32_t
>> for fiber ids. It is monotonically growing. Which means it will overflow
>> in the foreseeable future on a long-running instance.
> 
>  - it is explected to be overlowable from the beginning

Why? It is not safe. There is no any protection in case of overflow
and ids clash.

>  - the IDs are recycled

Exactly. They start recycle on overflow. This means if you had a long
living fiber for 50 days, and the scenario described by me happens,
you will get id clash.

> I've been talking to Kostya about a year ago once I discovered that
> we're walking in cycle when reusing IDs. He assured me that this is
> fine and to get a problem we have to allocate 4294967195 fibers at
> once which will require ~1.5T of memory just to allocate fiber structure.

It is not about at once. It is about long running instances.

> Hardy to happen. Still I think we at least should add a painc in case of
> hash collision for future.

We should just make it int64_t and forget about it IMO.

>> - Use 64bit ids for fibers. Unless I am wrong somewhere in my thoughts
>>   above.
> 
> This is dubious. Personally I would prefer to have them as uint64_t too
> now and for all, to simply forget about this potential issue. +4 bytes
> to fiber structure seems to be acceptable trade off.

Exactly.