Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Cyrill Gorcunov <gorcunov@gmail.com>
Cc: tml <tarantool-patches@dev.tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH] say: fix format for fiber()->fid
Date: Wed, 24 Feb 2021 23:39:20 +0100	[thread overview]
Message-ID: <334c9ace-2e92-5b8b-f727-ac33f69a25d9@tarantool.org> (raw)
In-Reply-To: <YDYOR2UYsVki0Nfh@grain>

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.

  reply	other threads:[~2021-02-24 22:39 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-22 18:20 Cyrill Gorcunov via Tarantool-patches
2021-02-22 18:23 ` Cyrill Gorcunov via Tarantool-patches
2021-02-23 20:41 ` Vladislav Shpilevoy via Tarantool-patches
2021-02-23 21:49   ` Cyrill Gorcunov via Tarantool-patches
2021-02-23 22:47     ` Vladislav Shpilevoy via Tarantool-patches
2021-02-24  8:28       ` Cyrill Gorcunov via Tarantool-patches
2021-02-24 22:39         ` Vladislav Shpilevoy via Tarantool-patches [this message]
2021-02-24 15:36 ` [Tarantool-patches] [PATCH v2 00/10] say: fix format strings Cyrill Gorcunov via Tarantool-patches
2021-02-24 15:36   ` [Tarantool-patches] [PATCH v2 01/10] say: use unsigned format for fiber()->fid Cyrill Gorcunov via Tarantool-patches
2021-02-24 15:36   ` [Tarantool-patches] [PATCH v2 02/10] popen: fix say_x format arguments Cyrill Gorcunov via Tarantool-patches
2021-02-24 22:39     ` Vladislav Shpilevoy via Tarantool-patches
2021-02-25  7:27       ` Cyrill Gorcunov via Tarantool-patches
2021-02-24 15:36   ` [Tarantool-patches] [PATCH v2 03/10] raft: fix say_x arguments Cyrill Gorcunov via Tarantool-patches
2021-02-24 22:39     ` Vladislav Shpilevoy via Tarantool-patches
2021-02-25  7:50       ` Cyrill Gorcunov via Tarantool-patches
2021-02-24 15:36   ` [Tarantool-patches] [PATCH v2 04/10] box/error: fix argument for CustomError Cyrill Gorcunov via Tarantool-patches
2021-02-24 15:36   ` [Tarantool-patches] [PATCH v2 05/10] xlog: fix say_x format Cyrill Gorcunov via Tarantool-patches
2021-02-24 22:40     ` Vladislav Shpilevoy via Tarantool-patches
2021-02-24 15:36   ` [Tarantool-patches] [PATCH v2 06/10] box/vynil: " Cyrill Gorcunov via Tarantool-patches
2021-02-24 22:43     ` Vladislav Shpilevoy via Tarantool-patches
2021-02-26  8:13       ` Nikita Pettik via Tarantool-patches
2021-02-24 15:36   ` [Tarantool-patches] [PATCH v2 07/10] txn: " Cyrill Gorcunov via Tarantool-patches
2021-02-24 15:36   ` [Tarantool-patches] [PATCH v2 08/10] limbo: " Cyrill Gorcunov via Tarantool-patches
2021-02-24 15:36   ` [Tarantool-patches] [PATCH v2 09/10] wal: " Cyrill Gorcunov via Tarantool-patches
2021-02-24 22:41     ` Vladislav Shpilevoy via Tarantool-patches
2021-02-25  7:52       ` Cyrill Gorcunov via Tarantool-patches
2021-02-24 15:36   ` [Tarantool-patches] [PATCH v2 10/10] say: fix CFORMAT specification Cyrill Gorcunov via Tarantool-patches
2021-02-24 22:41     ` Vladislav Shpilevoy via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=334c9ace-2e92-5b8b-f727-ac33f69a25d9@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=gorcunov@gmail.com \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH] say: fix format for fiber()->fid' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox