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

RFC: Initial vsock stream support #581

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

gkelly
Copy link

@gkelly gkelly commented Nov 13, 2022

What does this PR do?

Adds initial vsock stream socket support to glommio. This is just to start a discussion of the approach, and is NOT ready to land.

Motivation

I am working on high-throughput vsock applications running inside of hypervisors and am evaluating glommio as a potential IO framework.

This implementation is based heavily off of the tcp_socket implementation.

Additional Notes

This is a draft PR, it is not yet ready to merge. It is missing the following:

  • Decide on how to treat SOCK_DGRAM sockets. I'm open to feedback here, but given how similar a STREAM and DGRAM socket behave under vsock I'm tempted to genericize the pieces across the SOCK_ type.
  • All of the missing documentation the linter's unhappy about.
  • Make the vsock example more complete, it's currently just an echo server to prove it works.
  • Tests.
  • Benchmarks.

@glommer
Copy link
Collaborator

glommer commented Nov 15, 2022

Thanks @gkelly !

I am taking a look at this now

@glommer
Copy link
Collaborator

glommer commented Nov 15, 2022

I don't have a lot of comments, to be honest - aside from the lack of documentations on public functions that I am sure you will fix eventually.

The implementation is quite straightforward, and I think the abstraction is the right one (having a generic socket type would be overkill IMHO, so a new class for VSock makes sense).

The example application is neat and simple. The only thing that I'd like to see more is testing and benchmarks, but I realize this can get a bit non trivial with vsock and its setup.

What are the milestones you have in mind before considering ready to merge?

@glommer
Copy link
Collaborator

glommer commented Nov 15, 2022

Okay, I see now that you did add a bunch of things to the list of milestones - including testing and benchmarks.

You already noticed the docs as well, so the only open comment is on how generic this should be.

I don't have a preference to be honest, on the implementation. In terms of API, I like the direction you are taking with a special case top level class, instead of making the socket type a parameter.

My reason for that is that in practice, almost everything is either a standard tcp or dgram socket. Any approach that parametrizes a socket puts the burden of reasoning on people using the standard socket types.

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.

2 participants