feat(kernel): `RwLock` registry and add helpers #267

hawkw · 2023-09-03T17:41:26Z

If we replace the u32 counter with an AtomicU32, we can make the
various connect methods on the registry borrow it immutably. This
allows us to store it in a RwLock rather than a Mutex, with the
write lock only being needed to register a new driver. I've done this,
as well as replacing Kernel::with_registry with Kernel::registry and
Kernel::registry_mut accessors.

Additionally, I've added new Registry::bind and Registry::bind_konly
helpers. These create a new listener for a service and register it with
the registry. This can be implemented using existing code, but now it
can be done in a single function call. One thing to note is that doing
this in one call means that the service is added to the registry
before the server task is actually spawned, because we need to create
a listener to give to the server task. However, I don't actually believe
that's a problem, since the the listener is a queue. If some other task
connects to the service before the server is actually running, that's
fine; the request will just go in the queue for use later. This also has
the substantial advantage that server tasks don't get spawned if there's
already a registration for that service. With the current design, we
spawn tasks first and then register, and if the registration fails,
those tasks are still running and nothing will get rid of them. So,
using bind also avoids a potential task leak when spawning a duplicate
service.

If we replace the `u32` counter with an `AtomicU32`, we can make the various `connect` methods on the registry borrow it immutably. This will allow us to store it in a `RwLock` rather than a `Mutex`, with the write lock only being needed to register a new driver.

This lets us replace `with_registry` with `registry`/`registry_mut` accessors. I've also simplified existing instances of registration logic.

This reduces registration boilerplate a little.

@jamesmunns

Depends on #267 Currently, the `Registry::connect_*` methods return an error immediately if the requested service is not found in the registry. Most client types implement a `from_registry` method which retry in a loop when the requested service is not found in the registry. These methods will wait for a fixed (short) amount of time and then try again until the registry returns the service. This approach is quite inefficient, as we have to run a bunch of retry loops that keep trying to access a service that may not be there. This may happen several times before the service actually is registered, especially when registering a service requires connecting to another service. This branch improves the efficiency of waiting for a service to be registered. Now, rather than retrying with a fixed-duration sleep, we instead have the `Registry` own a `WaitCell` which is woken whenever a new service is registered. This wakes all takes potentially waiting to connect, allowing them to re-check whether the service they want is in the registry. This idea was initially proposed by @jamesmunns in a [comment] on PR #259 Connections are now established using either `Registry::connect`, which retries whenever a new service is registered, or `Registry::try_connect` which never retries. Additionally, we now have the capacity to indicate that a service is not found *and* that the registry is full, by closing the `WaitCell`. In this case, retrying will never succeed, because the registry is full and if the service isn't already there, it will never be added. In this case, the retrying methods will also return an error, rather than never completing, so we avoid a potential task leak. In order to make this change, we need to move the `RwLock` from being around the entire `Registry` to being inside the registry, around `items`. This allows the `WaitCell` to be accessed regardless. It also allows us to shorten the duration for which the lock is held. This requires changing all methods on `Registry` to take `&self`. Therefore, I've removed the wrapper methods on `Kernel` for connecting and registering, since they can now just be called on `kernel.registry` without a bunch of extra boilerplate for lock management. I've also simplified the API surface of the registry a bit by removing the `connect` methods that don't take a `Hello`, and just using `Registry::connect(())` in those cases. IMO, those methods weren't really pulling their weight, and they required us to have a method named `Registry::try_connect_userspace_with_hello` if we were going to add a non-retrying `connect` variant. Now, we can just have `Registry::try_connect_userspace`, `Registry::connect_userspace`, `Registry::connect`, and `Registry::try_connect`, which feels much less egregious. [comment]: #258 (comment)

jamesmunns

hell yeah

@jamesmunns

Depends on #267 Currently, the `Registry::connect_*` methods return an error immediately if the requested service is not found in the registry. Most client types implement a `from_registry` method which retry in a loop when the requested service is not found in the registry. These methods will wait for a fixed (short) amount of time and then try again until the registry returns the service. This approach is quite inefficient, as we have to run a bunch of retry loops that keep trying to access a service that may not be there. This may happen several times before the service actually is registered, especially when registering a service requires connecting to another service. This branch improves the efficiency of waiting for a service to be registered. Now, rather than retrying with a fixed-duration sleep, we instead have the `Registry` own a `WaitCell` which is woken whenever a new service is registered. This wakes all takes potentially waiting to connect, allowing them to re-check whether the service they want is in the registry. This idea was initially proposed by @jamesmunns in a [comment] on PR #259 Connections are now established using either `Registry::connect`, which retries whenever a new service is registered, or `Registry::try_connect` which never retries. Additionally, we now have the capacity to indicate that a service is not found *and* that the registry is full, by closing the `WaitCell`. In this case, retrying will never succeed, because the registry is full and if the service isn't already there, it will never be added. In this case, the retrying methods will also return an error, rather than never completing, so we avoid a potential task leak. In order to make this change, we need to move the `RwLock` from being around the entire `Registry` to being inside the registry, around `items`. This allows the `WaitCell` to be accessed regardless. It also allows us to shorten the duration for which the lock is held. This requires changing all methods on `Registry` to take `&self`. Therefore, I've removed the wrapper methods on `Kernel` for connecting and registering, since they can now just be called on `kernel.registry` without a bunch of extra boilerplate for lock management. I've also simplified the API surface of the registry a bit by removing the `connect` methods that don't take a `Hello`, and just using `Registry::connect(())` in those cases. IMO, those methods weren't really pulling their weight, and they required us to have a method named `Registry::try_connect_userspace_with_hello` if we were going to add a non-retrying `connect` variant. Now, we can just have `Registry::try_connect_userspace`, `Registry::connect_userspace`, `Registry::connect`, and `Registry::try_connect`, which feels much less egregious. [comment]: #258 (comment)

@jamesmunns

Depends on #267 Currently, the `Registry::connect_*` methods return an error immediately if the requested service is not found in the registry. Most client types implement a `from_registry` method which retry in a loop when the requested service is not found in the registry. These methods will wait for a fixed (short) amount of time and then try again until the registry returns the service. This approach is quite inefficient, as we have to run a bunch of retry loops that keep trying to access a service that may not be there. This may happen several times before the service actually is registered, especially when registering a service requires connecting to another service. This branch improves the efficiency of waiting for a service to be registered. Now, rather than retrying with a fixed-duration sleep, we instead have the `Registry` own a `WaitCell` which is woken whenever a new service is registered. This wakes all takes potentially waiting to connect, allowing them to re-check whether the service they want is in the registry. This idea was initially proposed by @jamesmunns in a [comment] on PR #259 Connections are now established using either `Registry::connect`, which retries whenever a new service is registered, or `Registry::try_connect` which never retries. Additionally, we now have the capacity to indicate that a service is not found *and* that the registry is full, by closing the `WaitCell`. In this case, retrying will never succeed, because the registry is full and if the service isn't already there, it will never be added. In this case, the retrying methods will also return an error, rather than never completing, so we avoid a potential task leak. In order to make this change, we need to move the `RwLock` from being around the entire `Registry` to being inside the registry, around `items`. This allows the `WaitCell` to be accessed regardless. It also allows us to shorten the duration for which the lock is held. This requires changing all methods on `Registry` to take `&self`. Therefore, I've removed the wrapper methods on `Kernel` for connecting and registering, since they can now just be called on `kernel.registry` without a bunch of extra boilerplate for lock management. I've also simplified the API surface of the registry a bit by removing the `connect` methods that don't take a `Hello`, and just using `Registry::connect(())` in those cases. IMO, those methods weren't really pulling their weight, and they required us to have a method named `Registry::try_connect_userspace_with_hello` if we were going to add a non-retrying `connect` variant. Now, we can just have `Registry::try_connect_userspace`, `Registry::connect_userspace`, `Registry::connect`, and `Registry::try_connect`, which feels much less egregious. [comment]: #258 (comment)

hawkw requested a review from jamesmunns September 3, 2023 17:41

hawkw added 5 commits September 3, 2023 11:01

refac(kernel): take &self in Registry::get

9676c78

feat(kernel): put the registry in a RwLock

5f4a8b1

This lets us replace `with_registry` with `registry`/`registry_mut` accessors. I've also simplified existing instances of registration logic.

feat(kernel): add bind/bind_konly helpers

21ed47a

This reduces registration boilerplate a little.

refac: use bind helpers to reduce boilerplate

463d70b

hawkw force-pushed the eliza/shared-registry branch from d00a14f to 463d70b Compare September 3, 2023 18:01

hawkw mentioned this pull request Sep 3, 2023

feat(kernel): wake tasks on service registration #268

Merged

hawkw added area: kernel Related to the cross-platform kernel kind: enhancement New feature or request labels Sep 3, 2023

hawkw self-assigned this Sep 3, 2023

jamesmunns approved these changes Sep 3, 2023

View reviewed changes

hawkw merged commit 672e3dd into main Sep 3, 2023
7 checks passed

hawkw deleted the eliza/shared-registry branch September 3, 2023 21:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(kernel): `RwLock` registry and add helpers #267

feat(kernel): `RwLock` registry and add helpers #267

hawkw commented Sep 3, 2023

jamesmunns left a comment

feat(kernel): RwLock registry and add helpers #267

feat(kernel): RwLock registry and add helpers #267

Conversation

hawkw commented Sep 3, 2023

jamesmunns left a comment

Choose a reason for hiding this comment

feat(kernel): `RwLock` registry and add helpers #267

feat(kernel): `RwLock` registry and add helpers #267