Small question

[MikuChan03]

I'm wondering, do we really need the io_uring_smp_load_acquire barrier? As I understand it, while the code in the following if statement might be executed speculatively, the side effects of that execution are only supposed to be visible to other threads when it is clear that the condition is met.

Removing the barrier, I have compiled liburing on a weakly ordered armv7 and run the tests repeatedly. No error occurred.

/*
 * Return an sqe to fill. Application must later call io_uring_submit()
 * when it's ready to tell the kernel about it. The caller may call this
 * function multiple times before calling io_uring_submit().
 *
 * Returns a vacant sqe, or NULL if we're full.
 */
IOURINGINLINE struct io_uring_sqe *_io_uring_get_sqe(struct io_uring *ring)
{
        struct io_uring_sq *sq = &ring->sq;
        unsigned int head, next = sq->sqe_tail + 1;
        int shift = 0;

        if (ring->flags & IORING_SETUP_SQE128)
                shift = 1;
        if (!(ring->flags & IORING_SETUP_SQPOLL))
                head = *sq->khead;
        else
                head = io_uring_smp_load_acquire(sq->khead);

        if (next - head <= sq->ring_entries) {
                struct io_uring_sqe *sqe;

                sqe = &sq->sqes[(sq->sqe_tail & sq->ring_mask) << shift];
                sq->sqe_tail = next;
                io_uring_initialize_sqe(sqe);
                return sqe;
        }

        return NULL;
}

[axboe]

Don't think it's needed in practice, io_uring_get_sqe() isn't thread safe to begin with. Care to send a patch?

[MikuChan03]

Right away, you'll have it shortly!

[MikuChan03]

@axboe It's here.

[calebsander]

Don't the consumer and producer of an atomic SPSC queue always need to use release ordering when incrementing their index and acquire ordering when reading their counterpart's? This ensures that each element has been fully written or read before it is made available to the other thread. On the SQ poll thread (consumer side), the update of sq.head is done with a release ordering to keep it after the consumption of the SQEs:

int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr)
	__must_hold(&ctx->uring_lock)
{
	unsigned int entries = io_sqring_entries(ctx);
	unsigned int left;
	int ret;

	if (unlikely(!entries))
		return 0;
	/* make sure SQ entry isn't read before tail */
	ret = left = min(nr, entries);
	io_get_task_refs(left);
	io_submit_state_start(&ctx->submit_state, left);

	do {
		const struct io_uring_sqe *sqe;
		struct io_kiocb *req;

		if (unlikely(!io_alloc_req(ctx, &req)))
			break;
		if (unlikely(!io_get_sqe(ctx, &sqe))) {
			io_req_add_to_cache(req, ctx);
			break;
		}

		/*
		 * Continue submitting even for sqe failure if the
		 * ring was setup with IORING_SETUP_SUBMIT_ALL
		 */
		if (unlikely(io_submit_sqe(ctx, req, sqe)) &&
		    !(ctx->flags & IORING_SETUP_SUBMIT_ALL)) {
			left--;
			break;
		}
	} while (--left);

	if (unlikely(left)) {
		ret -= left;
		/* try again if it submitted nothing and can't allocate a req */
		if (!ret && io_req_cache_empty(ctx))
			ret = -EAGAIN;
		current->io_uring->cached_refs += left;
	}

	io_submit_state_end(ctx);
	 /* Commit SQ ring head once we've consumed and submitted all SQEs */
	io_commit_sqring(ctx);
	return ret;
}

static void io_commit_sqring(struct io_ring_ctx *ctx)
{
	struct io_rings *rings = ctx->rings;

	/*
	 * Ensure any loads from the SQEs are done at this point,
	 * since once we write the new head, the application could
	 * write new data to them.
	 */
	smp_store_release(&rings->sq.head, ctx->cached_sq_head);
}

This release ordering only has any effect when paired with the corresponding acquire ordering in _io_uring_get_sqe().

The only situation where the release-acquire pairing isn't necessary is If there is no concurrently executing SQ poll thread (that's the existing if (!(ring->flags & IORING_SETUP_SQPOLL)) case).

[isilence]

That's right, without "acquire" user writes to an SQE might get reordered before the load instruction, which means the kernel could read new updated (inconsistent) values while still processing the old batch of SQEs, i.e. before the store release.