Re: [PATCH 3/3] io_uring: move *queue_link_head() from common path

[Pavel Begunkov <asml.silence@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2828 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]