Re: Deduplicate io_*_prep calls?

[Jens Axboe <axboe@kernel.dk>]

On 2/24/20 8:56 AM, Pavel Begunkov wrote:
> On 24/02/2020 18:53, Jens Axboe wrote:
>> On 2/24/20 8:50 AM, Pavel Begunkov wrote:
>>> On 24/02/2020 18:46, Jens Axboe wrote:
>>>> On 2/24/20 8:44 AM, Pavel Begunkov wrote:
>>>>>> Fine like this, though easier if you inline the patches so it's easier
>>>>>> to comment on them.
>>>>>>
>>>>>> Agree that the first patch looks fine, though I don't quite see why
>>>>>> you want to pass in opcode as a separate argument as it's always
>>>>>> req->opcode. Seeing it separate makes me a bit nervous, thinking that
>>>>>> someone is reading it again from the sqe, or maybe not passing in
>>>>>> the right opcode for the given request. So that seems fragile and it
>>>>>> should go away.
>>>>>
>>>>> I suppose it's to hint a compiler, that opcode haven't been changed
>>>>> inside the first switch. And any compiler I used breaks analysis there
>>>>> pretty easy.  Optimising C is such a pain...
>>>>
>>>> But if the choice is between confusion/fragility/performance vs obvious
>>>> and safe, then I'll go with the latter every time. We should definitely
>>>> not pass in req and opcode separately.
>>>
>>> Yep, and even better to go with the latter, and somehow hint, that it won't
>>> change. Though, never found a way to do that. Have any tricks in a sleeve?
>>
>> We could make it const and just make the assignment a bit hackier... Apart
>> from that, don't have any tricks up my sleeve.
> 
> Usually doesn't work because of such possible "hackier assignments".
> Ok, I have to go and experiment a bit. Anyway, it probably generates a lot of
> useless stuff, e.g. for req->ctx

Tried this, and it generates the same code...


diff --git a/fs/io_uring.c b/fs/io_uring.c
index ba8d4e2d9f99..8de5863aa749 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -598,7 +598,7 @@ struct io_kiocb {
 
 	struct io_async_ctx		*io;
 	bool				needs_fixed_file;
-	u8				opcode;
+	const u8			opcode;
 
 	struct io_ring_ctx	*ctx;
 	struct list_head	list;
@@ -5427,6 +5427,8 @@ static bool io_get_sqring(struct io_ring_ctx *ctx, struct io_kiocb *req,
 	 */
 	head = READ_ONCE(sq_array[ctx->cached_sq_head & ctx->sq_mask]);
 	if (likely(head < ctx->sq_entries)) {
+		u8 *op;
+
 		/*
 		 * All io need record the previous position, if LINK vs DARIN,
 		 * it can be used to mark the position of the first IO in the
@@ -5434,7 +5436,8 @@ static bool io_get_sqring(struct io_ring_ctx *ctx, struct io_kiocb *req,
 		 */
 		req->sequence = ctx->cached_sq_head;
 		*sqe_ptr = &ctx->sq_sqes[head];
-		req->opcode = READ_ONCE((*sqe_ptr)->opcode);
+		op = (void *) req + offsetof(struct io_kiocb, opcode);
+		*op = READ_ONCE((*sqe_ptr)->opcode);
 		req->user_data = READ_ONCE((*sqe_ptr)->user_data);
 		ctx->cached_sq_head++;
 		return true;

-- 
Jens Axboe