Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass server->client command over a separate socket pair #762

Merged
merged 15 commits into from
Sep 15, 2023

Conversation

mnissler-rivos
Copy link
Contributor

This pull request changes the VFIO-user protocol to optionally use a separate pair of sockets for server->client commands. Doing so simplifies command / reply processing for the case where both server and client send commands concurrently, which has proven problematic per issue #279.

Included are the functional changes, new pytests, as well as protocol specification updates.

This change adds support for a separate socket to carry commands in the
server-to-client direction. It has proven problematic to send commands
in both directions over a single socket, since matching replies to
commands can become non-trivial when both sides send commands at the same
time and adds significant complexity. See issue nutanix#279 for details.

To set up the reverse communication channel, the client indicates
support for it via a new capability flag in the version message. The
server will then create a fresh pair of sockets and pass one end to the
client in its version reply. When the server wishes to send commands to
the client at a later point, it now uses its end of the new socket pair
rather than the main socket. Corresponding replies are also passed back
over the new socket pair.

Signed-off-by: Mattias Nissler <mnissler@rivosinc.com>
Many tests have a `sock` global holding the client socket. In order to
allow additional client context to be managed, introduce a Client class
and replace the `sock` global with `client` global object.

Signed-off-by: Mattias Nissler <mnissler@rivosinc.com>
The new tests verify behavior of vfu_sgl_{read,write} for success and
error cases.

Signed-off-by: Mattias Nissler <mnissler@rivosinc.com>
Include rationale and operation of separate server-to-client command
channel in the protocol description and indicate which commands should
be processed on the separate socket.

Signed-off-by: Mattias Nissler <mnissler@rivosinc.com>
@mnissler-rivos
Copy link
Contributor Author

Rebased after #763 was merged.

@mnissler-rivos
Copy link
Contributor Author

Now that I understand you're squashing, let me know whether I should break out any of the individual commits here into additional separate PRs - my intention was to make small self-contained commits in order to simplify reviews and keep the history clean for folks coming after us.

Copy link
Collaborator

@jlevon jlevon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this! This generally looks great, especially the new test code!
Comments below.

docs/vfio-user.rst Outdated Show resolved Hide resolved
docs/vfio-user.rst Outdated Show resolved Hide resolved
lib/tran.c Show resolved Hide resolved
lib/tran.c Outdated Show resolved Hide resolved
lib/tran.c Outdated Show resolved Hide resolved
lib/tran.c Outdated Show resolved Hide resolved
lib/tran.c Outdated Show resolved Hide resolved
lib/tran.c Outdated Show resolved Hide resolved
lib/tran.c Outdated Show resolved Hide resolved
lib/tran.c Show resolved Hide resolved
Copy link
Contributor Author

@mnissler-rivos mnissler-rivos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

John, thanks for reviewing!

docs/vfio-user.rst Outdated Show resolved Hide resolved
docs/vfio-user.rst Outdated Show resolved Hide resolved
lib/tran.c Show resolved Hide resolved
lib/tran.c Outdated Show resolved Hide resolved
lib/tran.c Outdated Show resolved Hide resolved
lib/tran.c Show resolved Hide resolved
lib/tran.c Outdated Show resolved Hide resolved
lib/tran.c Outdated Show resolved Hide resolved
lib/tran.c Outdated Show resolved Hide resolved
lib/tran.c Outdated Show resolved Hide resolved
@mnissler-rivos
Copy link
Contributor Author

Btw. I think it makes sense to break out 4025f6e into a separate MR so the mechanical s/sock/client/ changes don't clutter this MR up?

@jlevon
Copy link
Collaborator

jlevon commented Aug 23, 2023

Btw. I think it makes sense to break out 4025f6e into a separate MR so the mechanical s/sock/client/ changes don't clutter this MR up?

yep

@tmakatos tmakatos self-requested a review August 28, 2023 16:40
Copy link
Member

@tmakatos tmakatos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • protocol version should be increased
  • protocol changes could go into an individual patch
  • I don't see in the spec how the server-to-client socket becomes known to the client and the server?

Other than that looks good, some minor nits.

docs/vfio-user.rst Outdated Show resolved Hide resolved
docs/vfio-user.rst Outdated Show resolved Hide resolved
lib/tran.c Show resolved Hide resolved
lib/tran.c Show resolved Hide resolved
lib/tran.c Outdated Show resolved Hide resolved
test/py/libvfio_user.py Outdated Show resolved Hide resolved
test/py/libvfio_user.py Outdated Show resolved Hide resolved
test/py/libvfio_user.py Outdated Show resolved Hide resolved
test/py/libvfio_user.py Outdated Show resolved Hide resolved
test/py/test_sgl_read_write.py Outdated Show resolved Hide resolved
@mnissler-rivos
Copy link
Contributor Author

As discussed, I have now broken out various bits and pieces into separate merge requests:

#772 - Introduce client object in python tests
#773 - Replace protocol header flags bit field with mask
#774 - Prepare python test helpers for receiving commands
#775 - Describe the twin-socket feature in the spec

And there's another one that we haven't discussed yet. This is preparation work for the server version capabilities to include the index of the socket file descriptor in the ancillary data array:

#771 - Construct server capabilities using json-c

I think it'll be best if we work through these preparatory MRs first, then I can rebase this one eventually. Or I can alternatively update this one to the final version now - let me know if you have a preference.

I will also go through the comment threads here and resolve what has been addressed already.

lib/tran_sock.c Outdated Show resolved Hide resolved
Copy link
Collaborator

@jlevon jlevon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!!

@jlevon jlevon merged commit 1569a37 into nutanix:master Sep 15, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants