Re: [PATCH v3 5/5] io_uring/netcmd: add tx timestamping cmd support

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

On 6/12/25 15:31, Jens Axboe wrote:
> On 6/12/25 8:26 AM, Pavel Begunkov wrote:
>>>> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
>>>> index cfd17e382082..5c89e6f6d624 100644
>>>> --- a/include/uapi/linux/io_uring.h
>>>> +++ b/include/uapi/linux/io_uring.h
>>>> @@ -968,6 +968,15 @@ enum io_uring_socket_op {
>>>>        SOCKET_URING_OP_SIOCOUTQ,
>>>>        SOCKET_URING_OP_GETSOCKOPT,
>>>>        SOCKET_URING_OP_SETSOCKOPT,
>>>> +    SOCKET_URING_OP_TX_TIMESTAMP,
>>>> +};
>>>> +
>>>> +#define IORING_CQE_F_TIMESTAMP_HW    ((__u32)1 << IORING_CQE_BUFFER_SHIFT)
>>>> +#define IORING_TIMESTAMP_TSTYPE_SHIFT    (IORING_CQE_BUFFER_SHIFT + 1)
>>>
>>> Don't completely follow this, would at the very least need a comment.
>>> Whether it's a HW or SW timestamp is flagged in the upper 16 bits, just
>>> like a provided buffer ID. But since we don't use buffer IDs here, then
>>> it's up for grabs. Do we have other commands that use the upper flags
>>> space for command private flags?
>>
>> Probably not, but the place is better than the lower half, which
>> has common flags like F_MORE, especially since the patch is already
>> using it to store the type.
> 
> Just pondering whether it should be formalized, but probably no point as
> each opcode should be free to use the space as it wants.

Right, that's what I insisted on long time ago, all fields except
user_data are opcode specific, even if some flags are reused for
user's convenience. There is no need to covert the upper half of
flags for provided buffers when the majority of opcodes doesn't
care about the feature.

>>> The above makes sense, but then what is IORING_TIMESTAMP_TSTYPE_SHIFT?
>>
>> It's a shift for where the timestamp type is stored, HW vs SW is
>> not a timestamp type. I don't get the question.
> 
> Please add a spec like comment on top of it explaining the usage of the
> upper bits in the flags field, then. I try to keep the io_uring.h uapi
> header pretty well commented and documented.

Ok

-- 
Pavel Begunkov