Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
To:     Olivier Langlois <olivier@trillion01.com>,
        Jens Axboe <axboe@kernel.dk>, io-uring@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <67c806d0bcf2e096c1b0c7e87bd5926c37231b87.1624387080.git.olivier@trillion01.com>
 <60d23218.1c69fb81.79e86.f345SMTPIN_ADDED_MISSING@mx.google.com>
 <dcc24da6-33d6-ce71-8c87-f0ef4e7f8006@gmail.com>
 <b00eb9407276f54e94ec80e6d80af128de97f10c.camel@trillion01.com>
 <169899caad96c3214d6e380ac7686d054eed3b12.camel@trillion01.com>
 <2603ffd4-c318-66ed-9807-173159536f6a@gmail.com>
 <a2ade714df72c0adeb19897811133a0e0244a729.camel@trillion01.com>
From:   Pavel Begunkov <asml.silence@gmail.com>
Subject: Re: [PATCH 1/2 v2] io_uring: Fix race condition when sqp thread goes
 to sleep
Message-ID: <6d2c2777-cce6-6633-c937-fd3a56ffbd73@gmail.com>
Date:   Wed, 23 Jun 2021 16:58:47 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.10.1
MIME-Version: 1.0
In-Reply-To: <a2ade714df72c0adeb19897811133a0e0244a729.camel@trillion01.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 6/23/21 2:52 PM, Olivier Langlois wrote:
> On Wed, 2021-06-23 at 00:03 +0100, Pavel Begunkov wrote:
>> On 6/22/21 11:42 PM, Olivier Langlois wrote:
>>> On Tue, 2021-06-22 at 18:37 -0400, Olivier Langlois wrote:
>>>> On Tue, 2021-06-22 at 21:45 +0100, Pavel Begunkov wrote:
>>>>
>>>>
>>>> I can do that if you want but considering that the function is
>>>> inline
>>>> and the race condition is a relatively rare occurence, is the
>>>> cost
>>>> coming with inline expansion really worth it in this case?
>>>>>
>>> On hand, there is the inline expansion concern.
>>>
>>> OTOH, the benefit of going with your suggestion is that completions
>>> generally precedes new submissions so yes, it might be better that
>>> way.
>>>
>>> I'm really unsure about this. I'm just raising the concern and I'll
>>> let
>>> you make the final decision...
>>
>> It seems it may actually loop infinitely until it gets a signal,
>> so yes. And even if not, rare stalls are nasty, they will ruin
>> some 9s of latency and hard to catch.
>>
>> That part is quite cold anyway, would generate some extra cold
>> instructions, meh
>>
> I'm not 100% sure to see the infinite loop possibility but I guess that
> with some badly placed preemptions, it could take few iterations before
> entering the block:
> 
> 		if (sqt_spin || !time_after(jiffies, timeout)) {

Had a case in mind, but looking through the branches it can't
really happen. Agree that won't be infinite in real life, until
we start using (and there was an RFC) finer grained timeouts.

In any case for several reasons think it's the right thing to do.

> So I will go ahead with your suggestion.
> 
> I'll retest the new patch version (it should be a formality) and I'll
> resend an update once done.

Perfect

-- 
Pavel Begunkov