Re: [PATCH 1/1] io_uring/zctx: separate notification user_data

[Jens Axboe <axboe@kernel.dk>]

On 2/17/26 4:15 AM, Pavel Begunkov wrote:
> On 2/16/26 17:27, Jens Axboe wrote:
> ...
>>> There are already 6, it'll be 7th. I also have one or two more in mind,
>>> that's already over the half. The same was probably thought about
>>> sqe->flags, and even though it's twice as many bits for net, those
>>> are taken faster as potential cost of redesign is lower.
>>>
>>> Fwiw, the code is nastier as well, more branchy and away from
>>> other notification init because of dependency on reading the
>>> flags.
>>>
>>> @@ -1331,7 +1333,7 @@ int io_send_zc_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>>>         zc->done_io = 0;
>>>   -    if (unlikely(READ_ONCE(sqe->__pad2[0]) || READ_ONCE(sqe->addr3)))
>>> +    if (unlikely(READ_ONCE(sqe->__pad2[0])))
>>>           return -EINVAL;
>>>       /* we don't support IOSQE_CQE_SKIP_SUCCESS just yet */
>>>       if (req->flags & REQ_F_CQE_SKIP)
>>> @@ -1358,6 +1360,13 @@ int io_send_zc_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>>>           }
>>>       }
>>>   +    if (zc->flags & IORING_SEND_ZC_NOTIF_USER_DATA) {
>>> +        notif->cqe.user_data = READ_ONCE(sqe->addr3);
>>> +    } else {
>>> +        if (READ_ONCE(sqe->addr3))
>>> +            return -EINVAL;
>>> +    }
>>> +
>>
>> I think just remove the else part here - addr3 is valid now that
>> IORING_SEND_ZC_NOTIF_USER_DATA is supported, and if you mess it up in
>> your applications, you'll find this via development anyway. Since addr3
>> == 0 is a valid value, it doesn't make much sense to check for it being
>> non-zero.
> 
> Gating it on a separate flag but not checking when not set makes
> it only more confusing in terms of why would you do a flag in
> the first place.
> 
> It's not like a flags field where any value set would be an
>> -EINVAL case. Doesn't even exclude having another flag for using addr3
>> for something else anyway.
> 
> You can override the behaviour with another flag in either case,
> but realistically it's better to avoid as it's always messy,
> unless the features are clearly exclusive.
> 
> I know there is no way to convince you, but v2 already degraded
> the uapi as per requested, can we have that one? The "else" branch
> doesn't make the api worse, on the opposite.

The else ties into all of it though, as it perpetuates the "user_data is
zero is not valid" part. The reason we have the addr3 check in the first
place is to have a way of saying "this field isn't used for this opcode,
may be used in the future". Now it is used/supported, and I don't think
we should be checking it. If we end up with future flags that also need
addr3 and 0 is valid, then it'll end up with more checking for that,
based on which flags are set and which are not.

The patch should just be removing that addr3 -EINVAL case, and adding
the two lines that check IORING_SEND_ZC_NOTIF_USER_DATA, and if set, assign
notif->cqe.user_data from addr3.

But I object to saying this is a "degraded" uapi, to me it's very much a
better one as it allows all values of user_data, rather than have some
magic 0 value that's not valid for no other reason than force policy.

-- 
Jens Axboe