Attention is currently required from: jolly, pespin.

Patch set 6:Code-Review -1

View Change

5 comments:

• File src/core/osmo_io_internal.h:

  • Patch Set #6, Line 116: /* ! array of rings the submitted read SQEs have been submitted */

    "have been submitted to"

    same comment like in the other patch regarding "*!" vs "* !"

• File src/core/osmo_io_uring.c:

  • Patch Set #2, Line 175:

    No, it doesn't, because I don't track the number of submissions and the number of completions. […]

    ok, then please mention this fact explicitly in this c ommit in a TODO-comment and also in the commit log. At least this way it's clear that there is no related cleanup yet. Implementation could then follow at a later patch, but at least it is known to everyone that this is the situation.

    I would consider having a single per-ring counter of the number of not-yet-completed sqe's would be sufficient. If that counter reaches zero, and we are not the currently active ring, we can destroy it?

• File src/core/osmo_io_uring.c:

  • Patch Set #6, Line 172: sqe = io_uring_get_sqe(&g_ring->ring);

    so here we end up with 2 read SQEs on 2 different queues. […]

    does it matter? Do we ever care? I think there's no such guarantee even with two read sqe's for the same fd on a single ring? In the end, they are all empty msgb's waiting for some incoming data. It doesn't matter which is used/filled/reported-completed first?

  • Patch Set #6, Line 527: while ((sqe = io_uring_get_sqe(iofd->u.uring.read_ring[idx])) == NULL);

    that looks like a really nice tight non-blocking loop filling the CPU?

    yeah. It's one problem stalling the process itself by waiting for something to happen eventually - it's another problem to waste a full CPU core while doing that, potentially slowing down the event that we're waiting for.

    It seems `io_uring_sqring_wait` was introduced for this: https://man7.org/linux/man-pages/man3/io_uring_sqring_wait.3.html

    • we may have to check
    • 1) at compile time that a sufficient enough liburing version is present
    • 2) at runtime that the kernel supports it (-EINVAL is returned otherwise)

    It might also make sense to log something just before we call io_uiring_sqring_wait... that might help debugging if we ever ended up spending significant time in it.

    So my strategy would be:

    • fast path trying io_uring_get_sqe
    • only if that fails, log the situation and use io_uring_sqring_wait
    • if io_uring_sqring_wait fails, fall-back to busy-waiting, but maybe with some usleep in the loop

    Alternatively, we could try calling io_uring_sqring_wait once at ring creation time (when we know it will never block as there are always free SQEs inside), and cache whether or not it is supported (positive value) or unsupported (-EINVAL) in a global variable. The code here and below could then avoid the trial-and-error every time we unregister an fd.

  • Patch Set #6, Line 543: while ((sqe = io_uring_get_sqe(iofd->u.uring.write_ring)) == NULL);

    that looks like a really nice tight non-blocking loop filling the CPU?

    same as above

To view, visit change 40725. To unsubscribe, or for help writing mail filters, visit settings.