Skip to content

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