io: add scheduler #16633

dotnwat · 2024-02-18T19:56:50Z

Adds a simple scheduler to the I/O subsystem. The purpose of the scheduler is to manage all of the active I/O queues (e.g. implementing global policies like preventing starvation or priority). However, this initial version of the scheduler only implements a policy that limits the total number of open file handles.

Backports Required

Release Notes

none

Signed-off-by: Noah Watkins <noahwatkins@gmail.com>

This is done for consistency with other test helpers. Signed-off-by: Noah Watkins <noahwatkins@gmail.com>

vbotbuildovich · 2024-02-18T22:38:26Z

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/45102#018dbe0b-a897-4940-aefe-a041bf3a7fbc

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/45104#018dbf38-b7db-410e-bccc-5ec8bd6cb5ff

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/45173#018dc9a5-bdbd-432b-9643-f87be3bd89c5

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/45267#018dd39c-9af5-4561-ba4d-8eedb9f4c26a

dotnwat · 2024-02-19T15:55:33Z

Converting to draft while I fix a memory usage issue with a large test.

Signed-off-by: Noah Watkins <noahwatkins@gmail.com>

andrwng

Looking pretty good! I found it a bit tough to dive into, but having stared for a while I think it mostly makes sense. I added some naming suggestions that I felt would've helped.

andrwng · 2024-02-21T23:23:39Z

src/v/io/scheduler.h

+    size_t waiters_{0};
+    seastar::semaphore nofiles_;
+    intrusive_list<queue, &queue::cache_hook_> lru_;
+    seastar::future<> monitor(queue*) noexcept;


nit: mind moving this above the members? Not sure if others feel this way but I always find it way easier to navigate a header if the methods are above the members.

no preference at all. fixed. i wonder if this should be added to our style-guide.md ?

andrwng · 2024-02-22T02:21:20Z

src/v/io/scheduler.h

+     * detailed description.
+     */
+    size_t waiters_{0};
+    seastar::semaphore nofiles_;


question/nit: is this "number of files/no. files" or "no files"? A comment describing what the semaphore protects would be appreciated. Or maybe name it available_file_units_ or something?

Also it might be worth considering using ssx::semaphore for improved debuggability

its number of open files. i've renamed it to be clearler.

switched to ssx::semaphore. forgot it exists!

andrwng · 2024-02-22T02:24:29Z

src/v/io/scheduler.h

+         * see scheduler::monitor implementation for more details.
+         */
+        seastar::future<> monitor_{seastar::make_ready_future<>()};
+        seastar::semaphore sem_{0};


nit: could you add a comment describing what this semaphore is meant to be used for (and maybe any special considerations to be made when thinking about the nofile_ semaphore, if any)?

one thought: perhaps this should be named has_work_sem or somesuch?

addressed both comments

andrwng · 2024-02-22T02:37:00Z

src/v/io/scheduler.h

+         */
+        seastar::future<> monitor_{seastar::make_ready_future<>()};
+        seastar::semaphore sem_{0};
+        seastar::semaphore_units<> units_;


nit: at least jumping into this code for the first time, I think it'd be helpful to name this scheduler_units_ or nofiles_units_ or somesuch to set readers' expectations on what usage should look like

andrwng · 2024-02-22T02:56:47Z

src/v/io/scheduler.cc

+            co_return;
+        }
+
+        co_await queue->sem_.wait(std::max<size_t>(queue->sem_.current(), 1));


I'm a little confused by this. What's the significance of using a size-0 semaphore vs a condition variable?

added a comment into the code to explain. basically a condition variable will drop wake-ups if there are no waiters. fix that with a predicate, but in this case the predicate isn't a simple flag or two to check. so we can use a semaphore to ensure that the loop will run again if its signaled while the worker is busy doing other things.

Ah got it, thanks for clarifying. A while ago we found that a similar (maybe the same) concern had addressed in seastar recently.

https://github.com/scylladb/seastar/blob/f0b2971a24ab7716933a720572207fa9a1df38bd/tests/unit/condition_variable_test.cc#L44-L49

Is this the scenario you're thinking about?

Oh, that's interesting. Seems they addressed that 2 years ago and it hasn't yet solidified in my brain. Part of that is because I don't think pthread condition variable has this property. Anyway, that's good to know. I'll likely switch this back to condition variable then in a later PR.

andrwng · 2024-02-22T02:59:16Z

src/v/io/scheduler.h

+         * a queue to close in order to enforce the maximum open queue limit.
+         */
+        intrusive_list_hook sched_hook_;
+        intrusive_list_hook cache_hook_;


nit: again might just be a first-time-reader thing, but I think it'd be more easily followable naming this lru_hook_ or renaming lru_ to cache_

andrwng · 2024-02-22T03:14:59Z

src/v/io/scheduler.h

+     */
+    size_t waiters_{0};
+    seastar::semaphore nofiles_;
+    intrusive_list<queue, &queue::cache_hook_> lru_;


comment suggestion: "LRU-ordered list of currently opened queues"

andrwng · 2024-02-22T03:31:08Z

src/v/io/scheduler.cc

+                /*
+                 * this case is possible, if for example, `nofiles_` was
+                 * initialized with 0 units.
+                 */
+                waiters_++;


I'm a bit confused by this: I suspect a subsequent change will allow changing the semaphore to be adjustable dynamically, so for now, it's odd to see. Assuming adjusting the semaphore will be the way we'd be able to unblock the get_units call, shouldn't we also be decrementing waiters_ once we add ourselves to lru_?

i added a comment about waiters being used to handle cases that arise when the semaphore units are dynamically adjusted.

shouldn't we also be decrementing waiters_ once we add ourselves to lru_?

I think that the net effect is that this is happening, but we can't easily have a worker decrement uncondtionally when it gets added to the lru list (even if it knows it contributed a +1) because its hard to know if another worker woekup and woke up some waiters (the cooperative wake up at the top is a while loop that wakes up as much as possible). not being careful would have us double count decrements.

i'm not convinced its 100% correct, but here is a simple example:

say we have open file limit as 0 and three queues show up. lru is empty, so waiters will bump to 3. then we deposit one unit into the semaphpore and a waiter wakes up. it'll add itself to the lru and loop back around and wake up waiters. the waiters are still 3, but the lru only has 1 entry, so so that limits the evictions to 1 (correctly, the currently running worker is the only open file and now itll self-close so another worker runs). waiters is decremented to 2 reflecting reality.

Ah thanks for the example! Very helpful. I'd skipped over the fact that we can still proceed with opening without waiters_ being decremented, as long as open_file_limit_ is added to. I initially thought another monitor had to pop from the lru_ for us to proceed, which was confusing because lru_ here is empty!

right. i guess waiters has kind of a optimistic property in that it's incremented before the waiter waits, and decremented before the waiter releases.

andrwng · 2024-02-22T03:42:23Z

src/v/io/tests/scheduler_test.cc

+    for (auto& driver : drivers) {
+        try {
+            seastar::remove_file(driver->queue.path().string()).get();
+        } catch (...) {
+        }
+    }


Maybe stick this in a deferred block so it runs even if the test fails?

Signed-off-by: Noah Watkins <noahwatkins@gmail.com>

Lazin · 2024-02-27T10:36:42Z

src/v/io/scheduler.cc

+         */
+        while (waiters_ > 0 && !lru_.empty()) {
+            waiters_--;
+            auto& queue = lru_.front();


queue local var shadows the parameter

oh, gross. good catch! at least they are different types. i wonder if that is why clangd wasn't flagging this in my ide.

Lazin · 2024-02-27T10:40:01Z

src/v/io/scheduler.cc

+    queue->sem_.signal();
+    auto monitor = std::exchange(
+      queue->monitor_, seastar::make_ready_future<>());
+    co_await std::move(monitor);


nit: it feels like this could be easier to read if the gate is used to control the lifetime of the background fiber

do you mean use a gate instead?

Lazin · 2024-02-27T10:53:01Z

src/v/io/scheduler.cc

+                lru_.pop_front();
+                queue.sem_.signal();
+            }
+            units = co_await seastar::get_units(nofiles_, 1);


do I understand correctly that the invariant is that if the queue is in the lru_ list it should have units so if we are waking up the queue it's guaranteed that we will eventually acquire units on this line?

yes. but i think as you point out, a queue may still hold on to the units even if it isn't on the lru list. this is a temporary state, and not being on the list is a signal to the queue monitor that when it runs it should release its units after stopping the io_queue and closing the file.

dotnwat added 2 commits February 18, 2024 11:08

io: fix docstring test parameter ordering

a7b558f

Signed-off-by: Noah Watkins <noahwatkins@gmail.com>

io: relocate testing details under io namespace

9c24f26

This is done for consistency with other test helpers. Signed-off-by: Noah Watkins <noahwatkins@gmail.com>

github-actions bot added the area/redpanda label Feb 18, 2024

dotnwat force-pushed the io-scheduler branch from 3a19d2e to 618ea33 Compare February 18, 2024 19:57

dotnwat requested a review from andrwng February 18, 2024 20:14

dotnwat requested review from Lazin, abhijat, nvartolomei and rockwotj February 19, 2024 00:55

dotnwat marked this pull request as draft February 19, 2024 15:55

dotnwat added 3 commits February 20, 2024 17:16

io: add scheduler

0c934c5

Signed-off-by: Noah Watkins <noahwatkins@gmail.com>

io: add scheduler test

42b59fe

Signed-off-by: Noah Watkins <noahwatkins@gmail.com>

io: add module readme

1375e4f

Signed-off-by: Noah Watkins <noahwatkins@gmail.com>

dotnwat force-pushed the io-scheduler branch from e9b14de to 1375e4f Compare February 21, 2024 01:17

dotnwat marked this pull request as ready for review February 21, 2024 01:55

andrwng reviewed Feb 22, 2024

View reviewed changes

dotnwat added 10 commits February 22, 2024 16:14

io: move method above members

72ffe4e

Signed-off-by: Noah Watkins <noahwatkins@gmail.com>

io: rename nofiles_ to open_file_limit_

ae7c85f

Signed-off-by: Noah Watkins <noahwatkins@gmail.com>

io: use ssx::semaphore for debuggability

f59d448

Signed-off-by: Noah Watkins <noahwatkins@gmail.com>

io: use better naming for monitor work semaphore

f599791

Signed-off-by: Noah Watkins <noahwatkins@gmail.com>

io: rename semaphore units

cc84731

Signed-off-by: Noah Watkins <noahwatkins@gmail.com>

io: add comment about using semaphore instead of condition variable

537d5ea

Signed-off-by: Noah Watkins <noahwatkins@gmail.com>

io: rename cache_hook to lru_hook

d0de59b

Signed-off-by: Noah Watkins <noahwatkins@gmail.com>

io: add clarifying comment to member

2d3b504

Signed-off-by: Noah Watkins <noahwatkins@gmail.com>

io: add clarifying comment about waiters

200ead6

Signed-off-by: Noah Watkins <noahwatkins@gmail.com>

io: make test cleanup more robust

dd82bae

Signed-off-by: Noah Watkins <noahwatkins@gmail.com>

dotnwat requested a review from andrwng February 23, 2024 06:00

andrwng approved these changes Feb 23, 2024

View reviewed changes

dotnwat merged commit 7a3b3d5 into redpanda-data:dev Feb 23, 2024
16 checks passed

Lazin reviewed Feb 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

io: add scheduler #16633

io: add scheduler #16633

dotnwat commented Feb 18, 2024

vbotbuildovich commented Feb 18, 2024 •

edited

dotnwat commented Feb 19, 2024

andrwng left a comment

andrwng Feb 21, 2024

dotnwat Feb 22, 2024

andrwng Feb 22, 2024

andrwng Feb 22, 2024

dotnwat Feb 22, 2024

andrwng Feb 22, 2024

andrwng Feb 22, 2024

dotnwat Feb 22, 2024

andrwng Feb 22, 2024

dotnwat Feb 22, 2024

andrwng Feb 22, 2024

dotnwat Feb 22, 2024

andrwng Feb 23, 2024

dotnwat Feb 23, 2024

andrwng Feb 22, 2024

dotnwat Feb 22, 2024

andrwng Feb 22, 2024

dotnwat Feb 22, 2024

andrwng Feb 22, 2024

dotnwat Feb 23, 2024

andrwng Feb 23, 2024

dotnwat Feb 23, 2024

andrwng Feb 22, 2024

dotnwat Feb 23, 2024

Lazin Feb 27, 2024

dotnwat Feb 28, 2024 •

edited

Lazin Feb 27, 2024

dotnwat Feb 28, 2024

Lazin Feb 27, 2024

dotnwat Feb 28, 2024

io: add scheduler #16633

io: add scheduler #16633

Conversation

dotnwat commented Feb 18, 2024

Backports Required

Release Notes

vbotbuildovich commented Feb 18, 2024 • edited

dotnwat commented Feb 19, 2024

andrwng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dotnwat Feb 28, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbotbuildovich commented Feb 18, 2024 •

edited

dotnwat Feb 28, 2024 •

edited