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
