Expose the scheduling API to Rust

[patjed41]

fixes: #33
depends on and finishes: #32

This PR exposes a simplified version of the API implemented in scheduling.hh and uses it to finish timer specializations.

Scheduling API

The scheduling_group class is exposed as a struct called SchedulingGroup along with its methods except get_specific. Operators == and != are expressed in Rust by implementing the Eq trait. The public constructor and the default_scheduling_group function (both of them do the same thing) are expressed by implementing the Default trait.

Apart from the scheduling_group class, the following functions are exposed:

• create_scheduling_group,
• delete_scheduling_group,
• rename_scheduling_group,
• max_scheduling_groups (we lose constexpr but I don't think it's a problem),
• current_scheduling_group.

All of them are implemented in Rust as methods or associated functions of the SchedulingGroup struct. C++ passes scheduling_group by value to every function but I think we want to use references in Rust (edit: I think this approach is wrong, I've explained it in a new comment).

Class scheduling_group has a method set_shares that is not const. I don't think expressing this in Rust by requiring &mut self is a good idea. This single method would make our API much harder to use. I think that using &self everywhere is completely fine because SchedulingGroup can be considered as just a flag that doesn't change and only provides an interface.

Finishing timer

This PR also adds a method called set_callback_under_groupto all 3 timer specializations. To implement this method, it was necessary to expose scheduling_group first. This feature makes seastar::timer's functionality fully exposed.

The rest of the scheduling API

As described here #33, we don't want to expose the rest of the scheduling API to Rust for now.

[patjed41]

I've promised to write a comment explaining problems with this PR on last meeting. I think there are two main problems:

1. The first issue is that the asynchronous rename and destroy methods take reference to SchedulingGroup. I think it would be much better if they took SchedulingGroup by value instead, because references can cause problems with lifetimes for users. This could be achieved by implementing the Clone trait for SchedulingGroup. To do it, we could, for example, change the type of the inner field from UniquePtr<scheduling_group> to SharedPtr<scheduling_group>. So, this problem is solvable.
2. The second issue is that the API pretends to be safe when in fact it is not. If I'm not wrong, the only reason for this is that the seastar::destroy_scheduling_group function has some assumptions:

/// ...
/// The destroyed group must not be currently in use and must not be used later.
/// ...
future<> destroy_scheduling_group(scheduling_group sg) noexcept;

We could make this function unsafe in Rust but it is not enough, because some other functions get infected by this and become unsafe too. For example, if we created a group, then destroyed it, and finally used set_shares on it, the program would crash during the set_shares call. What we can do about it:

• As said above, we could make the destroy function and all infected functions unsafe.
• If we did not expose seastar::destroy_scheduling_group to Rust, the whole API would be safe. The question is how important is this function. Afaik, it's quite important.
• We could try to make destroy the only unsafe part of the API. To achieve this, we would have to prevent using a group after its destruction. Instead of crashing, the infected functions would return an error if used with a destroyed group. Implementing this seems possible. We would have to remember somewhere which groups are destroyed. For example, if we removed the default and current functions, changed the type of the inner field to SharedPtr<scheduling_group>, and implemented the Clone trait, the only way to get a SchedulingGroup instance would be by calling create and then cloning it. This would allow us to store a new destroyed: Arc<Mutex<bool>> field which would be set to true during the destroy call. Other functions would check its value and return an error when it equals true. The only functions/methods that need this are: destroy, rename, set_shares, and Timer::set_callback_under_group which are already expensive, so I don't think that a single mutex use is tragic here.
• We could try to make the whole API safe, but this seems really hard. For example, destroy would have to return an error when there exists a timer that currently uses a callback under the group to destroy. Checking this might be very complicated.

Of course, I don't know the scheduling API well, so there might be other reasons why this API is unsafe. If that's the case, then making the whole API unsafe might be the only option.

I'm still waiting for a review and decision what to do with this.

[piodul]

I'm extremely sorry for taking so much time before responding to this.

I've promised to write a comment explaining problems with this PR on last meeting. I think there are two main problems:

1. The first issue is that the asynchronous rename and destroy methods take reference to SchedulingGroup. I think it would be much better if they took SchedulingGroup by value instead, because references can cause problems with lifetimes for users. This could be achieved by implementing the Clone trait for SchedulingGroup. To do it, we could, for example, change the type of the inner field from UniquePtr<scheduling_group> to SharedPtr<scheduling_group>. So, this problem is solvable.

I'm not sure, what kind of lifetime problems do these methods cause? I think taking self by reference should be perfectly fine. If by issues you mean that the Rust future stops being polled and the C++ side has a dangling reference, then we only need to make sure that the C++ side of the implementation immediately makes a copy of the scheduling_group (it is a very cheap object to clone).

About implementing Clone for SchedulingGroup - I think this is a good idea anyway. If you look at the scheduling_group type it is basically a glorified wrapper over an integer. It was meant to be cheaply copied around. It would be perfect if SchedulingGroup could be Copy, too, but I'm not sure how to do it cleanly, so let's not go there yet.

By the way, I'm not sure if it already is, but it looks like SchedulingGroup could be Send and Sync:

/// Creates a scheduling group with a specified number of shares.
///
/// The operation is global and affects all shards. The returned scheduling
/// group can then be used in any shard.

2. The second issue is that the API pretends to be safe when in fact it is not. If I'm not wrong, the only reason for this is that the seastar::destroy_scheduling_group function has some assumptions:

/// ...
/// The destroyed group must not be currently in use and must not be used later.
/// ...
future<> destroy_scheduling_group(scheduling_group sg) noexcept;

We could make this function unsafe in Rust but it is not enough, because some other functions get infected by this and become unsafe too. For example, if we created a group, then destroyed it, and finally used set_shares on it, the program would crash during the set_shares call.

Remember that by calling an unsafe function you are responsible for making sure that all invariants that the compiler and the other, possibly safe code may rely on are upheld. If you don't, then some code that would otherwise be safe may crash.

In this case, your safe code depends on this invariant: "SchedulingGroup points is a valid scheduling group that wasn't destroyed". The only way in which you can break the invariant is by deleting a SchedulingGroup - you cannot guarantee in compile time that there aren't other objects like this that point to the same scheduling group. This is what makes this function unsafe. The conditions for when it is safe to call destroy_scheduling_group must be documented in its docstring.

Making destroy_scheduling_group unsafe shouldn't be too painful, or at least I hope so. In case of Scylla, scheduling groups are created at the very beginning and then never deleted. So, at least for now, this sounds like a palatable solution.

What we can do about it:

• As said above, we could make the destroy function and all infected functions unsafe.
• If we did not expose seastar::destroy_scheduling_group to Rust, the whole API would be safe. The question is how important is this function. Afaik, it's quite important.
• We could try to make destroy the only unsafe part of the API. To achieve this, we would have to prevent using a group after its destruction. Instead of crashing, the infected functions would return an error if used with a destroyed group. Implementing this seems possible. We would have to remember somewhere which groups are destroyed. For example, if we removed the default and current functions, changed the type of the inner field to SharedPtr<scheduling_group>, and implemented the Clone trait, the only way to get a SchedulingGroup instance would be by calling create and then cloning it. This would allow us to store a new destroyed: Arc<Mutex<bool>> field which would be set to true during the destroy call. Other functions would check its value and return an error when it equals true. The only functions/methods that need this are: destroy, rename, set_shares, and Timer::set_callback_under_group which are already expensive, so I don't think that a single mutex use is tragic here.
• We could try to make the whole API safe, but this seems really hard. For example, destroy would have to return an error when there exists a timer that currently uses a callback under the group to destroy. Checking this might be very complicated.

Of course, I don't know the scheduling API well, so there might be other reasons why this API is unsafe. If that's the case, then making the whole API unsafe might be the only option.

I'm still waiting for a review and decision what to do with this.

[patjed41]

About implementing Clone for SchedulingGroup - I think this is a good idea anyway. If you look at the scheduling_group type it is basically a glorified wrapper over an integer. It was meant to be cheaply copied around. It would be perfect if SchedulingGroup could be Copy, too, but I'm not sure how to do it cleanly, so let's not go there yet.

I changed the type of inner to SharedPtr<scheduling_group> and added #[derive(Clone)] to the declaration of SchedulingGroup.

Making destroy_scheduling_group unsafe shouldn't be too painful, or at least I hope so. In case of Scylla, scheduling groups are created at the very beginning and then never deleted. So, at least for now, this sounds like a palatable solution.

I made it unsafe.

By the way, I'm not sure if it already is, but it looks like SchedulingGroup could be Send and Sync:

These traits are auto implemented with the unsafe keyword.