Unify CPU scheduling groups and IO priority classes by xemul · Pull Request #1607 · scylladb/seastar

xemul · 2023-04-10T13:11:05Z

The unification means that priority classes cannot be registered at all. And any IO issued by the app happens in the context of current scheduling group, not in the context of explicitly (or implicitly) provided priority class. Unification implies deprecation of existing API that can be used to specify the priority class via e.g. function call argument.

The change is incompatible with the existing file API, so the next API level is introduced. In it, the file API methods that accept io_priority_class do not exist (in 6th level they do and are deprecated). The io-queue uses current_scheduling_group to derive the internal priority-class ID. Similarly, fstream options don't have the io_priority_class field in the 7th level. At the end the whole class io_priority_class is ifdef-ed out for the API level 7, all internals use opaque wrapping internal convenience classes for the transition period.

fixes: #1069

xemul · 2023-04-27T12:53:49Z

upd:

deprecated all API to maniopulate priority classes
patched the rest of seastar code not to use deprecated API
added update_io_bandwidth() to scheduling_group

avikivity · 2023-04-27T13:20:26Z

Shouldn't it be ON by default? So new applications (not that one is built every day) get the new behavior, and old ones will have to take action to use a deprecated API.

avikivity · 2023-04-27T13:22:46Z

I mean ON as a compile-time parameter, not run-time.

Do we even need run-time configuration?

If OFF, everything works as before, except you get deprecation warnings on the functions that register io_priority_class.
If ON, then the io_priority_class methods don't exist.

In six month we kill the old way.

avikivity · 2023-04-27T13:24:45Z

    static std::mutex _register_lock;
    static std::array<class_info, _max_classes> _infos;
+
+    friend std::tuple<unsigned, sstring> get_class_info(io_priority_class_id pc);


Better to use a struct so the return is self-documenting.

I kept it tuple in the next version as per your next comment

Don't understand. My next comment says that destructuring would still work with a struct as it does with a tuple.

Ah... I misunderstood your next comment in a way "ah, so you 'self-document' the return values here, so OK".
In fact, this helper is local to io_queue.cc and the above friendship goes away with API-level 6

avikivity · 2023-04-27T13:24:57Z

    if (!_priority_classes[id]) {
-        auto shares = pc.get_shares();
-        auto name = pc.get_name();
+        auto [ shares, name ] = get_class_info(pc.id());


(this would still work)

avikivity · 2023-04-27T13:29:11Z

        }

-        return _file.dma_write(pos, p, buf_size, _options.io_priority_class).then(
+        return _file.dma_write_impl(pos, reinterpret_cast<const uint8_t*>(p), buf_size, _options.io_priority_class, nullptr).then(


I didn't understand the problem in the patch changelog.

This place needs to call some file method that accept explicit priority class argument, but those methods are marked as deprecated and are going to be removed soon. So the fstream API need to switch to using something else.

avikivity · 2023-04-27T13:31:12Z

    size_t buffer_size = 8192;    ///< I/O buffer size
    unsigned read_ahead = 0;      ///< Maximum number of extra read-ahead operations
-    ::seastar::io_priority_class io_priority_class = default_priority_class();
+    internal::wrapped_io_priority_class io_priority_class = {};


Couldn't we just mark it as [[deprecated]]? Or will it generate deprecation warnings whenever someone instantiates file_input_stream_options?

With compile-time solution, it would just not be there.

It will generate deprecation warning every time it's instantiated and copied/moved, but the latter could (probably) be solved with un-deprecated explicit copy/move constructors and assign operators. Anyway...

I dropped this place from the new version. Instead, the io_priority_class is ifdef-ed out for API level 7. So users that ignore API level update and only use fstream options won't notice it :( I don't think it's bad

They'll notice deprecated create_io_priority_class(), no?

Yes, they'll notice register_one_priority_class and default_priority_class deprecation and then would need to find where they actually use it. Not big deal either, of course

avikivity · 2023-04-27T13:35:06Z

@emaxerrno be advised
@tchaikov please mention relevant people in the Ceph community

avikivity · 2023-04-27T13:35:56Z

After review, I'm more convinced this should be compile-time. Our goal is not to have to support two ways of doing the same thing for long.

xemul · 2023-04-27T13:49:02Z

Shouldn't it be ON by default? So new applications (not that one is built every day) get the new behavior, and old ones will have to take action to use a deprecated API.

It should be backward compatible, so unless APP knows what it does it should work the old way.

Do we even need run-time configuration?

Well, I first started with N+1 API level, but then noticed that we could go without it.

If OFF, everything works as before, except you get deprecation warnings on the functions that register io_priority_class. If ON, then the io_priority_class methods don't exist.

It almost like that here. If OFF (default) everything works as before, except that you get deprecation warnings. If ON, the io_priority_class methods ~~don't exists~~ throw/resolve with exception

In six month we kill the old way.

After review, I'm more convinced this should be compile-time. Our goal is not to have to support two ways of doing the same thing for long.

OK. In that case I'd stuff more changes into the N+1 API level.

We have io_priority_class passed as const reference throughout file_impl inheritance tree. That's annoying, I'd change that to be value.
The file_impl methods that don't want to implement support for io_intent argument continue to compile at the cost of extra default-impemented methods on our side. I'd drop that burden too

avikivity · 2023-04-27T13:54:44Z

I think we can split the change into two.

First, compile-time removal of register_io_priority_class(), and selection of unified scheduling. Doesn't require N+1.
Second, removal to io_priority_class completely; does require N+1.

I don't think we should do them at the same time.

xemul · 2023-04-27T14:21:07Z

First, compile-time removal of register_io_priority_class(), and selection of unified scheduling.

... with the special flag to configure.py, like e.g. --scheduling-groups-count one?

avikivity · 2023-04-27T14:25:53Z

First, compile-time removal of register_io_priority_class(), and selection of unified scheduling.

... with the special flag to configure.py, like e.g. --scheduling-groups-count one?

Yes (and the cmake magic).

xemul · 2023-04-27T14:32:43Z

Then the notion of API level is not clear for me :( The doc says, that API level bumps when we do "breaking changes". Killing the ability to register a priority class sounds like a breaking change, but it still doesn't deserve new API level

avikivity · 2023-04-27T14:36:20Z

Then the notion of API level is not clear for me :( The doc says, that API level bumps when we do "breaking changes". Killing the ability to register a priority class sounds like a breaking change, but it still doesn't deserve new API level

I guess my idea with a compile-time decision is equivalent to an API level (which is just a more standardized way to do it).

tchaikov · 2023-04-28T07:11:29Z

@athanatos and @cyx1231st hi Sam and Yingxin, just a heads up that Seastar is going to drop the io_priority_class in favor of respecting the CPU scheduling group from which the io requests are issued. i just checked crimson's i/o path by grepping io_priority_class in the latest source tree and auditing all dma_read() and dma_write() calls , it seems we are not using io_priority_class yet, so we should be safe. but just in case we plan to use it for QoS, we might need to consider turning to the scheduling groups instead.

athanatos · 2023-04-29T18:53:35Z

@tchaikov Ah, thanks for the heads up.

avikivity · 2023-04-29T20:36:09Z

Ah, I see ceph isn't using scheduling groups either. So if/when you do, it will control both CPU and I/O resources.

cyx1231st · 2023-04-30T02:47:50Z

Thanks for the update! I don't see they are used in Ceph yet.

nyh · 2023-04-30T10:35:21Z

+                    return _file.dma_read_bulk_impl(start, len, _options.io_priority_class, &_intent);
            }).then_wrapped(
-                    [this, start, pos = _pos, remain = _remain] (future<temporary_buffer<char>> ret) {
+                    [this, start, pos = _pos, remain = _remain] (future<temporary_buffer<uint8_t>> ret) {


Are all these char/uint8_t changes and casts necessary? I guess dma_read_bulk_impl uses uint8_t but for some reason we really want this function to work with char? Do we have to?

Are all these char/uint8_t changes and casts necessary?

Yes, because compiler fails to auto-convert buffers of and <uint8_t> to each other

I guess dma_read_bulk_impl uses uint8_t but for some reason we really want this function to work with char? Do we have to?

That's a good question and I've no answer for it. The guess is that dma_read_bulk_impl() is called from template <CharType> dma_read_bulk() so we have no guarantee that the _impl thing will match any CharType from the API method. Thus it was selected to be 'generic enough' (88b4fab)

nyh · 2023-04-30T10:54:47Z

+    ///
+    /// \param bandwidth the new bandwidth value in bytes/second
+    /// \return a future that is ready when the bandwidth update is applied
+    future<> update_io_bandwidth(uint64_t bandwidth) const;


A pre-existing problem, but the comment should explain what is the default io bandwidth setting (I assume it's "unlimited"?) and how can you return it to unlimit (set it to 0? maxint? or what?).

nyh · 2023-04-30T10:55:51Z

+        return make_exception_future<>(std::runtime_error("Bandwidth should be updated directly on io_priority_class (deprecated)"));
+    }
+
+    return internal::make_priority_class_from_sched_group(*this).update_bandwidth(bandwidth);


The name "make_priority_class..." makes it sounds like this is creating a new object. But IIUC, it actually gets the existing one. So maybe should be called "get_priority_class_from_sched_group"?

In fact it's creating a new object, because io_priority_class is handle-like one that only contains an ID and can be copied multiple times. But this comment is no longer relevant :( Avi suggested that unification is done compile-time, so I've a new branch where this function doesn't exist

nyh · 2023-04-30T11:12:32Z

-        app_template app;
+        app_template::config cfg;
+        if (::getenv("SEASTAR_NATIVE_TEST") != nullptr && std::string(::getenv("SEASTAR_NATIVE_TEST")) == "yes") {
+            cfg.use_scheduling_groups_as_io_classes = true;


I don't think relying on environment variables is a good idea. You don't have to run tests via cmake. I often run a single test by just running the single executable - now this won't work?

I think that the solution should be different: Most Seastar tests don't care at all if this flag is enabled or disabled, so it can be left the default (disabled, Avi asked for enabled). Only the very few tests that are related to this flag should set this flag explicitly, in the individual test's code.

nyh · 2023-04-30T11:15:43Z

@@ -206,17 +206,26 @@ shard_id reactor::cpu_id() const {

 io_priority_class
 reactor::register_one_priority_class(sstring name, uint32_t shares) {


Shouldn't these functions be marked deprecated as well? Maybe it is, in a different patch?

They were (well -- should have been) in a different patch, yes. However now I'm reworking the PR and this function if ifdef-ed out

avikivity · 2023-05-02T10:39:48Z

+    future<size_t> do_write_dma(uint64_t pos, std::vector<iovec> iov, internal::maybe_priority_class_ref pc, io_intent* intent) noexcept;
+    future<size_t> do_read_dma(uint64_t pos, void* buffer, size_t len, internal::maybe_priority_class_ref pc, io_intent* intent) noexcept;
+    future<size_t> do_read_dma(uint64_t pos, std::vector<iovec> iov, internal::maybe_priority_class_ref pc, io_intent* intent) noexcept;
+    future<temporary_buffer<uint8_t>> do_dma_read_bulk(uint64_t offset, size_t range_size, internal::maybe_priority_class_ref pc, io_intent* intent) noexcept;


Since these are exposed to external file_impl implementations, please check that scylladb still compiles. I expect it should.

Scylla still compiles

@xemul hi Pavel, sorry for commenting in a closed pull request. do you have any plan on s/io_priority/scheduling_group/ in scylladb? usually, i compile scylladb without -Wno-error=deprecated-declarations. and i hope we can drop this option in configure.py as well. also from the practical perspective, we need to address the problem explained in #1069 (comment) in scylla sooner or latter.

i take a quick look at the code paths where io_priority is used, and found the task non-trivial. in addition to replacing all occurrences of io priorities with scheduling groups, we also need to create and destroy them. but unlike ::io_priority_class::register_one, create_scheduling_group() is a coroutine, so we cannot create them in a plain constructor like priority_manager::priority_manager, not to mention having a static thread_local priority_manager. probably we can create a sharded service in main() which provides the scheduling_groups for different i/o ops, or we should instead leave it to the tasks which actually issue io and let them to use dedicated a scheduling_group in certain cases where the current one does not suit the needs.

ahh, we do have different scheduling_groups in database_config, so what we need is probably to audit all the code paths and populate these sg down to where the io ops are issued if the current sg does not suit.

@tchaikov , my plan (nearest, Avi had already refreshed seastar in Scylla) is to remove the io_priority_class at all from Scylla code and 🤞 hope that whatever sched group happens to be current in each IO would be "correct" one. Likely we'll step on some bugs, but I guess there's no more reliable way to spot them.

Also, I do know several IO places in Scylla code that occur with default io-class, while should go in some other one, and those places do have correct current sched groups, so these places will be auto-fixed

thank you @xemul, sounds great! much appreciated! i spent a while the other day to do a mechanical compile-search-replace loop, but ended up giving it up. there were just too many of them, and the more places i changed the more scared -- i was afraid that i was introducing "some bugs" without being aware of them. as you put,

there's no more reliable way to spot them.

anyway, thank you! sometimes, the harmless warning messages are kind of distracting =)

There's just one mismatch in scylladb where scheduling groups and io_priority_class don't match, and that's commitlog. We'll need to create a commitlog scheduling group.

avikivity · 2023-05-02T10:47:43Z

Very good, had some nitpick comments.

The mimics the kernel change from cgroup v1 to cgroup v2. We're less flexible as shares for I/O and CPU are the same, but I think that's reasonable.

The one would return currently configured shares on the group. Implemented after the .name() method of the same class. Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>

Other file public API methods are inline and call <same_name>_impl() ones. The dma_read() and dma_write() are not like that, they are originally out-lined. This makes further patching look clumsy wrt API-level ifdefs, adding the _impl peers for those two makes the whole API be switched to next level in a uniform manner. Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>

There are two blocks of them -- with and without io_intent argument. The dma_read_bulk pair stays aside, there will be extra API-level ifdefs to cover them all. Better to reshuffle them to keep blocks compact. Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>

All existing file IO calls accept the io_priority_class argument which defaults to default_priority_class() if omitted. The plan is to deprecate the ability to pass explicit priority class in this call in faror of using priority class derived from current_scheduling_group(). To make the deprecation possible all the file methods are explicitly overloaded to accept priorty class argument and not to (and use default). The former set would later be deprecated, and the latter one will change its semantics (in a controllable way). Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>

The fstream code needs to specify the priority class to call file IO methods with. This is because IO priority fstream works with is specified on its options and have to be explicit. However, file public methods that accept explicit priority class argument are going to be deprecated and removed, so the fstream need to use some other way to pass priority class to file. The best option here seems to be the file::..._impl() methods. Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>

When a io-queue registers per-class data it needs to get shares and name for the class. Currently it uses two separate calls to get those, but future patching would benfit from doing it in one call. Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>

They are such since 8a2123d (reactor: Move submit_io_...() into io_queue). Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>

Spoiler: the IO-queue level is going to operate in two modes. First, legacy, when the priority class is the one explicitly pass by upper layers (file). Second, new one, when the priority class is derived from the current CPU scheduling group. Said that, the io-queue API should be able to accept both -- explicit io_priotity_class and implicit current_scheduling_group. In order not to re-implement all io-queue API twice, here's the wrapper that abstracts priority class io-queue needs from whatever the caller may provide. For now it can only be explicitly constructed from the io_priotity_class. Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>

All file_impl inheritants are going to have two implementations -- one legacy and one for the +1 API level. The difference is in the way the io_priority_class argument is passed around. In the legacy case it's the explicit const reference, in the latter case the priority_class won't even exist. However, _implementations_ of the methods do manipulate the "pc" argument internally and it's not nice to code them twice or wrap internals of the methods with ifdefs. So this patch wraps all overriden methods of file_impl inheritants into private calls that accept maybe_priority_class_ref argument. The overriden methods stay legacy by accepting const io_priority_class& one and constructing the maybe-ref from it. Also, to make the patch compile, recently introduced priority_class for io-queue level gains the ability to be constructed directly from the maybe-ref thing. Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>

A difficulty described in the previous patch also exists in a light form at the file-level, so it makes sense to patch it to use the maybe-ref as well. At this point the file methods that instantiate default_priority_class() behind the scenes construct the maybe-ref with no arguments. For now this means "reference default priority class". Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>

It's not yet declared, but implementing file API for it in advance makes the patch with this level much shorter and earier to review. In the new level the file API changes in two ways - IO priority class argument goes away - io_intent* arguments is mandatory for _impl-s First about the 2nd change. When the io_intent was introduced the file_impl got default implementation for the methods accepting the intent pointer -- it called "legacy" methods dropping the intent thus implementing the idea -- if the foo_file_impl wants to support requests cancellation, it has to do it voluntarily and explicitly. Now it's gone, all foo_file_impl will need to ignore the intent explicitly if they don't want to support it. Next, regarding the 1st change. The idea is that the priority_class doesn't exist in the file layer. By the time request gets to the io_queue::queue_request() the latter would simply pick up the current scheduling group and will work with it. Previous patches prepare the ground for this change by introducing the maybe_priority_class_ref thing. This patch implements the maybe-ref for the next API level by making it an empty struct. This lets file_impl-s pass it transparently down to the io-queue level, the latter would try to construct its internal::priority_class instance from it and will pick current sched class. Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>

A "continuation" of the previous patch. When sched group is renamed or needs to change its shares, all queues that managed to pick up name and shares before it needs to be updated. Not a major change, current IO priority classes renaming and shares-changing code does so, this patch just kicks the io-queues from another API call. Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>

That's a bit weird, but since all IO scheduling happens in the context of scheduling group, the latter must have a way to update IO bandwidth on it. Thus the new method, that only exists in the new API level. Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>

Since the top-most API level becomes the default for tests, demos and apps, patch them instantly to support new file and io-queue APIs. Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>

The methods that accept io_priority_class explicitly are marked as deprecated. Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>

Now when the priority-class manipulations are properly wrapped with maybe-ref-s and io-queue internal priority_class, the io_priority_class thing can be compiled-out for the 7th API level to avoid any attempt to use it in the brave new world. Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>

xemul · 2023-05-03T06:23:11Z

upd:

fixed review comments (default level in CMakeFile, introduced date, doc for scheduling_group::get_shares)

xemul requested a review from avikivity April 10, 2023 13:11

xemul force-pushed the br-unify-sched-classes branch from 1069a71 to 4101cdb Compare April 27, 2023 12:52

xemul marked this pull request as ready for review April 27, 2023 12:53

avikivity reviewed Apr 27, 2023

View reviewed changes

Comment thread src/core/file.cc

avikivity reviewed Apr 27, 2023

View reviewed changes

Comment thread src/core/file.cc

avikivity reviewed Apr 27, 2023

View reviewed changes

Comment thread include/seastar/core/fstream.hh Outdated

nyh requested changes Apr 30, 2023

View reviewed changes

xemul force-pushed the br-unify-sched-classes branch from 4101cdb to 96e0ca5 Compare May 2, 2023 07:59

xemul requested review from avikivity and nyh May 2, 2023 08:03

avikivity reviewed May 2, 2023

View reviewed changes

Comment thread include/seastar/core/scheduling.hh

avikivity reviewed May 2, 2023

View reviewed changes

Comment thread CMakeLists.txt Outdated

avikivity reviewed May 2, 2023

View reviewed changes

Comment thread doc/compatibility.md Outdated

xemul added 16 commits May 3, 2023 09:21

scheduling_group: Add get_shares method

e168c35

The one would return currently configured shares on the group. Implemented after the .name() method of the same class. Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>

io_queue: Turn queue_request() calls private

069f75c

They are such since 8a2123d (reactor: Move submit_io_...() into io_queue). Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>

seastar: Introduce API level 7

ea95dd8

Since the top-most API level becomes the default for tests, demos and apps, patch them instantly to support new file and io-queue APIs. Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>

file: Deprecate old file API

5086daf

The methods that accept io_priority_class explicitly are marked as deprecated. Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>

xemul force-pushed the br-unify-sched-classes branch from 96e0ca5 to 5fb7bef Compare May 3, 2023 06:22

xemul requested a review from avikivity May 3, 2023 06:23

avikivity merged commit f94b1bb into scylladb:master May 4, 2023

		@@ -206,17 +206,26 @@ shard_id reactor::cpu_id() const {

		io_priority_class
		reactor::register_one_priority_class(sstring name, uint32_t shares) {

Uh oh!

Conversation

xemul commented Apr 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xemul commented Apr 27, 2023

Uh oh!

avikivity commented Apr 27, 2023

Uh oh!

avikivity commented Apr 27, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

avikivity commented Apr 27, 2023

Uh oh!

avikivity commented Apr 27, 2023

Uh oh!

xemul commented Apr 27, 2023

Uh oh!

avikivity commented Apr 27, 2023

Uh oh!

xemul commented Apr 27, 2023

Uh oh!

avikivity commented Apr 27, 2023

Uh oh!

xemul commented Apr 27, 2023

Uh oh!

avikivity commented Apr 27, 2023

Uh oh!

tchaikov commented Apr 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

athanatos commented Apr 29, 2023

Uh oh!

avikivity commented Apr 29, 2023

Uh oh!

cyx1231st commented Apr 30, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

xemul commented Apr 10, 2023 •

edited

Loading

tchaikov commented Apr 28, 2023 •

edited

Loading

tchaikov May 5, 2023 •

edited

Loading