lkml.org 
[lkml]   [2019]   [Dec]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 3/3] io_uring: move *queue_link_head() from common path
From
Date
On 17/12/2019 21:07, Jens Axboe wrote:
> On 12/17/19 11:05 AM, Pavel Begunkov wrote:
>> On 17/12/2019 21:01, Jens Axboe wrote:
>>> On 12/17/19 10:52 AM, Pavel Begunkov wrote:
>>>> On 17/12/2019 20:37, Jens Axboe wrote:
>>>>> On 12/17/19 9:45 AM, Jens Axboe wrote:
>>>>>> On 12/16/19 4:38 PM, Pavel Begunkov wrote:
>>>>>>> On 17/12/2019 02:22, Pavel Begunkov wrote:
>>>>>>>> - } else if (req->sqe->flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) {
>>>>>>>> +
>>>>>>>> + /* last request of a link, enqueue the link */
>>>>>>>> + if (!(sqe_flags & IOSQE_IO_LINK)) {
>>>>>>>
>>>>>>> This looks suspicious (as well as in the current revision). Returning back
>>>>>>> to my questions a few days ago can sqe->flags have IOSQE_IO_HARDLINK, but not
>>>>>>> IOSQE_IO_LINK? I don't find any check.
>>>>>>>
>>>>>>> In other words, should it be as follows?
>>>>>>> !(sqe_flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK))
>>>>>>
>>>>>> Yeah, I think that should check for both. I'm fine with either approach
>>>>>> in general:
>>>>>>
>>>>>> - IOSQE_IO_HARDLINK must have IOSQE_IO_LINK set
>>>>>>
>>>>>> or
>>>>>>
>>>>>> - IOSQE_IO_HARDLINK implies IOSQE_IO_LINK
>>>>>>
>>>>>> Seems like the former is easier to verify in terms of functionality,
>>>>>> since we can rest easy if we check this early and -EINVAL if that isn't
>>>>>> the case.
>>>>>>
>>>>>> What do you think?
>>>>>
>>>>> If you agree, want to send in a patch for that for 5.5? Then I can respin
>>>>> for-5.6/io_uring on top of that, and we can apply your cleanups there.
>>>>>
>>>> Yes, that's the idea. Already got a patch, if you haven't done it yet.
>>>
>>> I haven't.
>>>
>>>> Just was thinking, whether to add a check for not setting both flags
>>>> at the same moment in the "imply" case. Would give us 1 state in 2 bits
>>>> for future use.
>>>
>>> Not sure I follow what you're saying here, can you elaborate?
>>>
>>
>> Sure
>>
>> #define IOSQE_IO_LINK (1U << 2) /* links next sqe */
>> #define IOSQE_IO_HARDLINK (1U << 3) /* like LINK, but stronger */
>>
>> That's 2 consequent bits, so 4 states:
>> 0,0 -> not a link
>> 1,0 -> common link
>> 0,1 -> hard link
>> 1,1 -> reserved, space for another link-quirk type
>>
>> But that would require additional check, i.e.
>>
>> if (flags&(LINK|HARDLINK) == (LINK|HARDLINK)) ...
>
> Ah, I see. In terms of usability, I think it makes more sense to have
>
> IOSQE_LINK | IOSQE_HARDLINK
>
> be the same as just IOSQE_LINK. It would be nice to retain that for

Probably, you meant it to be the same as __IOSQE_HARDLINK__

> something else, but I think it'll be more confusing to users.
>

Yeah, and it's easier for something like:

sqe->flags |= IOSQE_LINK;
[some code]
if (timer_or_whatever())
sqe->flags |= IOSQE_HARDLINK;

--
Pavel Begunkov

[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2019-12-17 19:13    [W:0.045 / U:1.128 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site