IPC Implementation Rewrite #114
Conversation
|
Thanks for this, I'll take a look later today. |
|
LGTM. |
|
sorry for the delay, looking at this now. Time works differently where I'm from 😅 |
There was a problem hiding this comment.
Writing is good overall, but there's a couple of design issues I see:
- The kernel is accessing the userspace heap (see the distinction you made about
kmalloc()vsmalloc())? This is a bad idea, as the kernel now relies on user code to 'do the right thing' and provide a workingmalloc()to allocate the recipients message buffers. Or alternatively the kernel has to be involved in userspace heap management (the user program might not even have a heap, or may have multiple, or some per-thread caches - how do you deal with that on the kernel side?).
This is a tricky one to deal with, the common approach is that userspace allocates the memory and informs the kernel of where the buffer is located as part of a system call. This is where a dedicated ipc_receive() function is handy, because the recipient can pass the buffer they wish the message to be copied into. You could also have a big block of memory attached to the endpoint and have that act as shared memory between the kernel and receiving process (this would need some synchronization if the receiving process isnt blocking on reads).
- Similar to 1, this version of message parsing combines IPC concepts and userspace/kernel interactions into one chapter. In a real implementation there may be some overlap, but I think it muddies the waters when it comes to explaining something new. It would be better to break this down into smaller ideas: how to move messages between two address spaces (all of this happens in the kernel, this is what the original message parsing example did), and then the API presented to userspace (not previously covered).
| - _Process 1_ wants to receive incoming messages on an endpoint, so it calls a function telling the kernel to create an endpoint in our IPC manager. This function will setup and return a block of (userspace) memory containing a message queue. We'll call this function `create_endpoint()`. | ||
| - _Process 2_ wants to send a message sometime later, so it allocates a buffer and writes some data there. | ||
| - _Process 2_ now calls a function to tell the kernel it wants to send this buffer as a message to an endpoint. We'll call this function `ipc_send()`. | ||
| - Inside `ipc_send()` the buffer is copied into kernel memory. In our example, we'll use the heap for this memory. We can then switch to process 1's address space and copy the buffer on the heap into the queue. |
There was a problem hiding this comment.
copy the buffer on the heap into the queue.
is a little ambiguous, I think relying less on context would clean this up:
copy the kernel's buffer into the endpoint's queue.
| uintptr_t next_message; | ||
| } ipc_message_t; | ||
|
|
||
| typedef struct ipc_message_queue{ |
There was a problem hiding this comment.
Is this type needed? since an endpoints and queues have a one-to-one relationship I dont think this is necessary. Instead embedding the list head in ipc_endpoint should be fine.
| void* msg_buffer; | ||
| size_t msg_length; | ||
| ipc_message_queue_t* queue; | ||
| uint64_t owner_pid; |
There was a problem hiding this comment.
I'd prefer tid_t over uint64_t here, it describes intent better. uint64_t may not always be a convenient type depending on the architecture.
| - What happens if there unread messages when destroying an endpoint? How do you handle them? | ||
| - Who is allowed to remove an endpoint? | ||
| - What happens if there are unread messages when destroying an endpoint? How do you handle them? | ||
| - Who is allowed to remove an endpoint? (`owner_pid` would be useful here) |
There was a problem hiding this comment.
what about:
(hint:
owner_pidis useful here)
|
|
||
| In theory this works, but we've overlooked one huge issue: what if there's already a message at the endpoint? You should handle this, and there's a couple of ways to go about it: | ||
| // Add the message to the queue |
|
|
||
| In theory this works, but we've overlooked one huge issue: what if there's already a message at the endpoint? You should handle this, and there's a couple of ways to go about it: | ||
| // Add the message to the queue | ||
| // Left for the reader to do, trivial linked list appending |
There was a problem hiding this comment.
this reads like a note I'd leave for myself rather than a hint. What about:
// append message struct to the endpoint's queue, implementation is left as an exercise for the reader.
| - We've described a double-copy implementation here, but you might want to try a single-copy implementation. Single-copy implementations *can* be faster, but they require extra logic. For example the kernel will need to access the recipient's address space from the sender's address space, how do you manage this? If you have all of physical memory mapped somewhere (like an identity map, or direct map (HHDM)) you could use this, otherwise you will need some way to access this memory. | ||
| - A process waiting on an endpoint (to either send or receive a message) could be waiting quite a while in some circumstances. This is time the cpu could be doing work instead of blocking and spinning on a lock. A simple optimization would be to put the thread to sleep, and have it be woken up whenever the endpoint is updated: a new message is sent, or the current message is read. | ||
| - In this example we've allowed for messages of any size to be sent to an endpoint, but you may want to set a maximum message size for each endpoint when creating it. This makes it easier to receive messages as you know the maximum possible size the message can be, and can allocate a buffer without checking the size of the message. This might seem silly, but when receiving a message from userspace the program has to make a system call each time it wants the kernel to do something. Having a maximum size allows for one-less system call. Enforcing a maximum size for messages also has security benefits. | ||
| - We've described a double-copy implementation here, but you might want to try a single-copy implementation. Single-copy implementations *can* be faster, but they require extra logic. For example, the kernel will need to access the recipient's address space from the sender's address space, how do you manage this? If you have all of the physical memory mapped somewhere (like an identity map, or direct map (HHDM)) you could use this, otherwise, you will need some way to access this memory. |
There was a problem hiding this comment.
, otherwise,
Too many commas, the second one can be omitted I think.
| - In this example we've allowed for messages of any size to be sent to an endpoint, but you may want to set a maximum message size for each endpoint when creating it. This makes it easier to receive messages as you know the maximum possible size the message can be, and can allocate a buffer without checking the size of the message. This might seem silly, but when receiving a message from userspace the program has to make a system call each time it wants the kernel to do something. Having a maximum size allows for one-less system call. Enforcing a maximum size for messages also has security benefits. | ||
| - We've described a double-copy implementation here, but you might want to try a single-copy implementation. Single-copy implementations *can* be faster, but they require extra logic. For example, the kernel will need to access the recipient's address space from the sender's address space, how do you manage this? If you have all of the physical memory mapped somewhere (like an identity map, or direct map (HHDM)) you could use this, otherwise, you will need some way to access this memory. | ||
| - A process waiting on an endpoint (to either send or receive a message) could be waiting quite a while in some circumstances. This is a time when the cpu could be doing work instead of blocking and spinning on a lock. A simple optimization would be to put the thread to sleep, and have it be woken up whenever the endpoint is updated: a new message is sent, or the current message is read. | ||
| - In this example we've allowed for messages of any size to be sent to an endpoint, but you may want to set a maximum message size for each endpoint when creating it. This makes it easier to receive messages as you know the maximum possible size the message can be, and can allocate a buffer without checking the size of the message. This might seem silly, but when receiving a message from userspace the program has to make a system call each time it wants the kernel to do something. Having a maximum size allows for one less system call. Enforcing a maximum size for messages also has security benefits. |
There was a problem hiding this comment.
the point about saving a system call is irrelevant for this implementation, since the message contents and metadata are already in the recipients address space. There's no system call in the example you give your receiving messages.
The rest is good.
|
@maxtyson123 any news on this PR? Do you want to make the changes requested? |
and he never responded... |
|
Sorry didn't see the earlier messages. Quite busy right now but I can begin working on it when I have spare time. |
Perfect thanks! :) Just let us know in case you don't have time at all to finish it, and you want someone else to take it over. |
|
Just to be clear before I begin working on this:
P.S. Is there a better place to talk with easier back and forth? That way we can keep the public github discussion to what is actually relevant to the PR and can also have a method smaller things |
|
Hey Max, all good - life is like that sometimes. Glad to see you're still interested in this :) As for your questions:
The other big benefit to the
|
resolves #113
As discussed in Issue #113 the current IPC message passing tutorial may be hard to follow for some users and the overall design could be reworked to be better.
This PR proposes a improved design:
(This IPC mecanhisim was a combination of the current one in the book and my implementation in Max OS