Make HTTP Server multi threaded #52
Conversation
amphi
force-pushed
the
http-multi-threaded
branch
from
3f0615d to
01458e7
Compare
There was a problem hiding this comment.
Thanks for working on this! I think this can be massively simplified with
- a single sender for requests (does not even has to be a sync sender but could be)
- a single receiver for requests that is shared by all worker threads
It would remove a lot of complexity, remove the manual round robin implementation, etc.
vmm/src/api/http/mod.rs
Outdated
|
|
||
| /// Returns a channel to a worker thread that is ready for more work, if | ||
| /// any workers are ready. | ||
| fn next_worker(&mut self) -> std::option::Option<&std::sync::mpsc::SyncSender<ServerRequest>> { |
There was a problem hiding this comment.
to me, this looks overly complicated. I actually thought about a solution where all worker threads blockingly receiver.recv() on a channel with Http Requests.
Then we also don't need a round robin algorithm. The thread that is ready picks up the request from the channel. That's a typical pattern for thread pools in Rust.
Is there a reason you decided against this?
There was a problem hiding this comment.
Because I didn't know that you can use an MPSC channel with multiple consumers. But your idea is very good, it made everything easier!
vmm/src/api/http/mod.rs
Outdated
| threads_ready: Vec<bool>, | ||
| // An MPSC channel for every worker. We use this to send server requests | ||
| // to the workers. | ||
| request_txs: Vec<std::sync::mpsc::SyncSender<ServerRequest>>, |
There was a problem hiding this comment.
why is this a vector of senders? I thought about one single Sender (does not even has to be syncsender) and a shared receiver by all worker threads. This would remove most of the complexity you have here
amphi
force-pushed
the
http-multi-threaded
branch
from
01458e7 to
8ffaea8
Compare
phip1611
left a comment
phip1611
left a comment
There was a problem hiding this comment.
Nice, definitely better! I think we can also entirely get rid of the epoll stuff and just use standard Rust channels for everything, further simplifying things.
| response_rx: std::sync::mpsc::Receiver<ServerResponse>, | ||
| // Workers signal this eventfd when they have a response for the HTTP | ||
| // server thread. | ||
| response_event: EventFd, |
There was a problem hiding this comment.
theoretically we could also get rid of this and use a channel from the workers to the server, right? Then all thredas would have the same sender (without the need for arc<mutex> as this works out of the box) and the main thread the receiver.
Thoughts?
There was a problem hiding this comment.
We have a channel from the worker threads to the main thread, but I don't understand why that means that we can get rid of this eventfd. The main thread has to monitor two things:
- Requests from the API, that it has to distribute to the workers
- Responses from the workers, that it has to forward to the http_server.
The only usable mechanism here (that I know of) is the epoll mechanism, and we need this eventfd for that.
There was a problem hiding this comment.
from offline discussion. it is okay to keep as we need eventfd handling with the underlying http server anyway
amphi
force-pushed
the
http-multi-threaded
branch
from
8ffaea8 to
b402674
Compare
phip1611
changed the title
olivereanderson
left a comment
olivereanderson
left a comment
There was a problem hiding this comment.
LGTM
I think this is the simplest way to make the existing HTTP server multi-threaded :)
On-behalf-of: SAP sebastian.eydam@sap.com Signed-off-by: Sebastian Eydam <sebastian.eydam@cyberus-technology.de>
phip1611
force-pushed
the
http-multi-threaded
branch
from
b402674 to
14f71be
Compare
3 participants
This PR makes the REST API multi threaded, i.e. if a REST API call blocks, you can still send requests that the VMM will respond to (if possible).