On 2/7/24 02:18, Jens Axboe wrote:
On 2/6/24 5:43 PM, Pavel Begunkov wrote:
On 2/6/24 16:22, Jens Axboe wrote:
We're out of space here, and none of the flags are easily reclaimable.
Bump it to 64-bits and re-arrange the struct a bit to avoid gaps.
Add a specific bitwise type for the request flags, io_request_flags_t.
This will help catch violations of casting this value to a smaller type
on 32-bit archs, like unsigned int.
No functional changes intended in this patch.
Signed-off-by: Jens Axboe <axboe@xxxxxxxxx>
---
...
typedef void (*io_req_tw_func_t)(struct io_kiocb *req, struct io_tw_state *ts);
@@ -592,15 +595,14 @@ struct io_kiocb {
* and after selection it points to the buffer ID itself.
*/
u16 buf_index;
- unsigned int flags;
- struct io_cqe cqe;
With the current layout the min number of lines we touch per
request is 2 (including the op specific 64B), that's includes
setting up cqe at init and using it for completing. Moving cqe
down makes it 3.
+ atomic_t refs;
We're pulling it refs, which is not touched at all in the hot
path. Even if there's a hole I'd argue it's better to leave it
at the end.
+
+ io_req_flags_t flags;
struct io_ring_ctx *ctx;
struct task_struct *task;
- struct io_rsrc_node *rsrc_node;
It's used in hot paths, registered buffers/files, would be
unfortunate to move it to the next line.
Yep I did feel a bit bad about that one... Let me take another stab at
it.
-
union {
/* store used ubuf, so we can prevent reloading */
struct io_mapped_ubuf *imu;
@@ -615,18 +617,23 @@ struct io_kiocb {
struct io_buffer_list *buf_list;
};
+ /* for polled requests, i.e. IORING_OP_POLL_ADD and async armed poll */
+ struct hlist_node hash_node;
+
And we're pulling hash_node into the hottest line, which is
used only when we arm a poll and remove poll. So, it's mostly
for networking, sends wouldn't use it much, and multishots
wouldn't normally touch it.
As for ideas how to find space:
1) iopoll_completed completed can be converted to flags2
That's a good idea, but won't immediately find any space as it'd just
leave a hole anyway. But would be good to note in there perhaps, you
never know when it needs re-arranging again.
struct io_kiocb {
unsigned flags;
...
u8 flags2;
};
I rather proposed to have this, which is definitely borderline
ugly but certainly an option.
2) REQ_F_{SINGLE,DOUBLE}_POLL is a weird duplication. Can
probably be combined into one flag, or removed at all.
Again, sends are usually not so poll heavy and the hot
path for recv is multishot.
Normal receive is also a hot path, even if multishot should be preferred
The degree of hotness is arguable. It's poll, which takes a
spinlock (and disables irqs), does an indirect call, goes into
he socket internals there touching pretty contended parts
like sock_wq. The relative overhead of looking at f_ops should
be nothing.
But the thought was more about combining them,
REQ_F_POLL_ACTIVE, and clear only if it's not double poll.
in general. Ditto on non-sockets but still pollable files, doing eg read
for example.
3) we can probably move req->task down and replace it with
get_task() {
if (req->ctx->flags & DEFER_TASKRUN)
task = ctx->submitter_task;
else
task = req->task;
}
Assuming ctx flags is hot, which is would generally be, that's not a bad
idea at all.
As mentioned, task_work_add would be the main user, and
there is already a different branch for DEFER_TASKRUN,
to it implicitly knows that ctx->submitter_task is correct.
I'll do another loop over this one.
--
Pavel Begunkov
