[vsock] Add method to send credit update manually #110
Conversation
This PR addresses a bug in the vsock connection manager. Prior to the PR, the guest doesn't update the host after it processes the local buffer. As a result, when the receiving message length reaches its buffer capacity, the host could mistakenly stop sending the message, unaware that the guest still had available buffer. This fix ensures that the credit is correctly updated processing the local buffer, allowing for continuous message transmission between the host and the guest.
|
It looks like there's some connection issues with clippy in CI. Can someone take a look at this PR? |
| connection.info.done_forwarding(bytes_read); | ||
| if bytes_read > 0 { | ||
| connection.info.done_forwarding(bytes_read); | ||
| self.driver.credit_update(&connection.info)?; |
There was a problem hiding this comment.
I don't think this is a good idea. It makes what was previously a non-blocking call (except in the case that the connection is in the process of shutdown) into a blocking call. This will significantly reduce performance, especially if the client is calling recv many times in a row with small buffers. It's also sending lots of credit updates that the host didn't ask for, which while allowed by the specification is unnecessary and will fill up the transmit queue and waste time on the host.
Credit is already updated for the connection whenever we send anything else on it (e.g. sending some data), which is likely to be enough a fair bit of the time in practice. If the host is blocked waiting for credit then it should send a credit request, which VsockConnectionManager::poll will automatically respond to. So all the client needs to do is ensure they are regularly calling poll, which is anyway necessary to avoid other deadlocks.
There was a problem hiding this comment.
Credit is already updated for the connection whenever we send anything else on it (e.g. sending some data), which is likely to be enough a fair bit of the time in practice.
That's true. But it is still blocked when the guest just wants to receive a message longer than 1024 bytes and it sends nothing to the host till it receives the whole message.
If the host is blocked waiting for credit then it should send a credit request ...
It looks like in practice the host doesn't always send credit request when it gets blocked. You can reproduce the failure without this fix by increasing the message length in this test.
There was a problem hiding this comment.
What about I just add a new API to update the credit to allow users to call it by themselves?
There was a problem hiding this comment.
Hmm, that's weird. Are you seeing this behaviour both with QEMU and crosvm?
Adding an API to send an unrequested credit update is alright I guess. I still think you shouldn't call it for every receive, as that will have a significant impact on performance. Perhaps you could just call it once you've emptied the receive buffer? To achieve that you could expose a new method on VsockConnectionManager to return the number of bytes currently in the receive buffer (perhaps call it available), and if after receiving a non-zero number of bytes available returns 0, then you know that the buffer has just become empty. Further optimisations are probably possible.
There was a problem hiding this comment.
There appears to be a potential issue with the driver itself, as both crosvm and qemu are experiencing a bug when sending long messages. However, it's worth noting that the problem encountered in qemu is slightly different. To test this issue with qemu, I modified the message sent from the host to be 3000 bytes long and adjusted the receiver buffer accordingly. As a result, the following error was observed on the host:
thread 'main' panicked at 'write_all: Os { code: 32, kind: BrokenPipe, message: "Broken pipe" }', src/main.rs:31:14
qwandor
changed the title
| @@ -226,6 +226,18 @@ impl<H: Hal, T: Transport> VsockConnectionManager<H, T> { | |||
| Ok(bytes_read) | |||
| } | |||
|
|
|||
| /// Returns whether the receive buffer is empty. | |||
| pub fn is_recv_buffer_empty(&mut self, peer: VsockAddr, src_port: u32) -> Result<bool> { | |||
There was a problem hiding this comment.
I think an available method which returns the number of bytes available to read (as I was suggesting before) would be better, as it's useful for other things as well.
|
Hi @qwandor can we release a new patch with these APIs? |
Done. |
This PR addresses a bug in the vsock connection manager. Prior to the PR, the guest doesn't update the host after it processes the local buffer. As a result, when the receiving message length reaches the buffer capacity, the host could mistakenly stop sending the message, unaware that the guest still had available buffer.
This fix ensures that the credit is correctly updated after processing the local buffer, allowing for continuous message transmission between the host and the guest.