On 10/24/24 8:49 AM, Pavel Begunkov wrote:
> On 10/24/24 15:40, Jens Axboe wrote:
>> On 10/24/24 8:26 AM, Pavel Begunkov wrote:
>>> On 10/24/24 15:18, Jens Axboe wrote:
>>>> On 10/23/24 8:38 PM, hexue wrote:
>>>>> On 9/25/2024 12:12, Pavel Begunkov wrote:
>>> ...
>>>>> When the number of threads exceeds the number of CPU cores,the
>>>>> database throughput does not increase significantly. However,
>>>>> hybrid polling can releasing some CPU resources during the polling
>>>>> process, so that part of the CPU time can be used for frequent
>>>>> data processing and other operations, which speeds up the reading
>>>>> process, thereby improving throughput and optimizaing database
>>>>> performance.I tried different compression strategies and got
>>>>> results similar to the above table.(~30% throughput improvement)
>>>>>
>>>>> As more database applications adapt to the io_uring engine, I think
>>>>> the application of hybrid poll may have potential in some scenarios.
>>>>
>>>> Thanks for posting some numbers on that part, that's useful. I do
>>>> think the feature is useful as well, but I still have some issues
>>>> with the implementation. Below is an incremental patch on top of
>>>> yours to resolve some of those, potentially. Issues:
>>>>
>>>> 1) The patch still reads a bit like a hack, in that it doesn't seem to
>>>> care about following the current style. This reads a bit lazy/sloppy
>>>> or unfinished. I've fixed that up.
>>>>
>>>> 2) Appropriate member and function naming.
>>>>
>>>> 3) Same as above, it doesn't care about proper placement of additions to
>>>> structs. Again this is a bit lazy and wasteful, attention should be
>>>> paid to where additions are placed to not needlessly bloat
>>>> structures, or place members in cache unfortunate locations. For
>>>> example, available_time is just placed at the end of io_ring_ctx,
>>>> why? It's a submission side member, and there's room with other
>>>> related members. Not only is the placement now where you'd want it to
>>>> be, memory wise, it also doesn't add 8 bytes to io_uring_ctx.
>>>>
>>>> 4) Like the above, the io_kiocb bloat is, by far, the worst. Seems to me
>>>> that we can share space with the polling hash_node. This obviously
>>>> needs careful vetting, haven't done that yet. IOPOLL setups should
>>>> not be using poll at all. This needs extra checking. The poll_state
>>>> can go with cancel_seq_set, as there's a hole there any. And just
>>>> like that, rather than add 24b to io_kiocb, it doesn't take any extra
>>>> space at all.
>>>>
>>>> 5) HY_POLL is a terrible name. It's associated with IOPOLL, and so let's
>>>> please use a name related to that. And require IOPOLL being set with
>>>> HYBRID_IOPOLL, as it's really a variant of that. Makes it clear that
>>>> HYBRID_IOPOLL is really just a mode of operation for IOPOLL, and it
>>>> can't exist without that.
>>>>
>>>> Please take a look at this incremental and test it, and then post a v9
>>>> that looks a lot more finished. Caveat - I haven't tested this one at
>>>> all. Thanks!
>>>>
>>>> diff --git a/include/linux/io_uring_types.h b/include/linux/io_uring_types.h
>>>> index c79ee9fe86d4..6cf6a45835e5 100644
>>>> --- a/include/linux/io_uring_types.h
>>>> +++ b/include/linux/io_uring_types.h
>>>> @@ -238,6 +238,8 @@ struct io_ring_ctx {
>>>> struct io_rings *rings;
>>>> struct percpu_ref refs;
>>>> + u64 poll_available_time;
>>>> +
>>>> clockid_t clockid;
>>>> enum tk_offsets clock_offset;
>>>> @@ -433,9 +435,6 @@ struct io_ring_ctx {
>>>> struct page **sqe_pages;
>>>> struct page **cq_wait_page;
>>>> -
>>>> - /* for io_uring hybrid poll*/
>>>> - u64 available_time;
>>>> };
>>>> struct io_tw_state {
>>>> @@ -647,9 +646,22 @@ struct io_kiocb {
>>>> atomic_t refs;
>>>> bool cancel_seq_set;
>>>> + bool poll_state;
>>>
>>> As mentioned briefly before, that can be just a req->flags flag
>>
>> That'd be even better, I generally despise random bool addition.
>>
>>>> struct io_task_work io_task_work;
>>>> - /* for polled requests, i.e. IORING_OP_POLL_ADD and async armed poll */
>>>> - struct hlist_node hash_node;
>>>> + union {
>>>> + /*
>>>> + * for polled requests, i.e. IORING_OP_POLL_ADD and async armed
>>>> + * poll
>>>> + */
>>>> + struct hlist_node hash_node;
>>>> + /*
>>>> + * For IOPOLL setup queues, with hybrid polling
>>>> + */
>>>> + struct {
>>>> + u64 iopoll_start;
>>>> + u64 iopoll_end;
>>>
>>> And IIRC it doesn't need to store the end as it's used immediately
>>> after it's set in the same function.
>>
>> Nice, that opens up the door for less esoteric sharing as well. And
>> yeah, I'd just use:
>>
>> runtime = ktime_get_ns() - req->iopoll_start - sleep_time;
>>
>> in io_uring_hybrid_poll() and kill it entirely, doesn't even need a
>> local variable there. And then shove iopoll_start into the union with
>> comp_list/apoll_events.
>
> That's with what the current request is hooked into the list,
> IOW such aliasing will corrupt the request
Ah true, well some other spot then, should be pretty easy to find 8
bytes for iopoll_start. As mentioned, the point is really just to THINK
about where it should go, rather than lazily just shove it at the end
like no thought has been given to it.
--
Jens Axboe
