Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <85be94ca-3ecd-a054-1b6c-a7561bde93ba@kernel.dk>
Date:   Wed, 12 Jul 2023 09:05:20 -0600
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux aarch64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: [PATCH 3/7] io_uring: add support for futex wake and wait
Content-Language: en-US
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     io-uring@vger.kernel.org, linux-kernel@vger.kernel.org,
        tglx@linutronix.de, mingo@redhat.com
References: <20230712004705.316157-1-axboe@kernel.dk>
 <20230712004705.316157-4-axboe@kernel.dk>
 <20230712085116.GC3100107@hirez.programming.kicks-ass.net>
From:   Jens Axboe <axboe@kernel.dk>
In-Reply-To: <20230712085116.GC3100107@hirez.programming.kicks-ass.net>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 7/12/23 2:51?AM, Peter Zijlstra wrote:
> On Tue, Jul 11, 2023 at 06:47:01PM -0600, Jens Axboe wrote:
>> Add support for FUTEX_WAKE/WAIT primitives.
>>
>> IORING_OP_FUTEX_WAKE is mix of FUTEX_WAKE and FUTEX_WAKE_BITSET, as
>> it does support passing in a bitset.
>>
>> Similary, IORING_OP_FUTEX_WAIT is a mix of FUTEX_WAIT and
>> FUTEX_WAIT_BITSET.
>>
>> FUTEX_WAKE is straight forward, as we can always just do those inline.
>> FUTEX_WAIT will queue the futex with an appropriate callback, and
>> that callback will in turn post a CQE when it has triggered.
>>
>> Cancelations are supported, both from the application point-of-view,
>> but also to be able to cancel pending waits if the ring exits before
>> all events have occurred.
>>
>> This is just the barebones wait/wake support. PI or REQUEUE support is
>> not added at this point, unclear if we might look into that later.
>>
>> Likewise, explicit timeouts are not supported either. It is expected
>> that users that need timeouts would do so via the usual io_uring
>> mechanism to do that using linked timeouts.
>>
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> 
> I'm not sure I'm qualified to review this :/ I really don't know
> anything about how io-uring works. And the above doesn't really begin to
> explain things.

It's definitely catered to someone who knows how that works already, I
feel like it'd be a bit beyond the scope of a commit message like that
to explain io_uring internals. Then we'd have to do that every time we
add a new request type.

But I can certainly expand it a bit and hopefully make it a bit clearer.
I'll do that.

>> +static void io_futex_wake_fn(struct wake_q_head *wake_q, struct futex_q *q)
>> +{
>> +	struct io_futex_data *ifd = container_of(q, struct io_futex_data, q);
>> +	struct io_kiocb *req = ifd->req;
>> +
>> +	__futex_unqueue(q);
>> +	smp_store_release(&q->lock_ptr, NULL);
>> +
>> +	io_req_set_res(req, 0, 0);
>> +	req->io_task_work.func = io_futex_complete;
>> +	io_req_task_work_add(req);
>> +}
> 
> I'm noting the WARN from futex_wake_mark() went walk-about.

True.

> Perhaps something like so?

I like that, sharing more code is always better. Should be a separate
patch though I think, or folded into patch 2. Which would you prefer?
I'll do a separate patch for now.

-- 
Jens Axboe