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

Breaking API: Remove cstruct #25

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

palainp
Copy link
Member

@palainp palainp commented Oct 1, 2024

Dear devs,
Continuing the Cstruct-free work, this is a first proposal for mirage-net.
As it's a breaking change, please let me know what is your preferred API shape :)
This PR is conservative in respect with the current allocations situation, but this can be changed to allow upper layers to allocate buffers, and let write/listen read from/write to them.
Best.

src/mirage_net.ml Outdated Show resolved Hide resolved
src/mirage_net.mli Outdated Show resolved Hide resolved
@hannesm
Copy link
Member

hannesm commented Oct 1, 2024

Dear @palainp, thanks for this PR. I have two minor suggestions, but totally agree that this is the right path forward.

Co-authored-by: Hannes Mehnert <[email protected]>
@hannesm
Copy link
Member

hannesm commented Oct 1, 2024

sorry, I had two commits that were part of the last release (4.0.0) on my laptop, which weren't pushed... so I just pushed them, shouldn't affect this PR much... but implementations of this interface may be affected.

@palainp
Copy link
Member Author

palainp commented Oct 1, 2024

Yes, thanks! I think I'm now up to date, and I'll need to adapt a bit mirage-net-solo5 :)

@hannesm
Copy link
Member

hannesm commented Oct 1, 2024

I'd be keen to wait here until next week and then merge and release this as the next major version. This way we can avoid having too many pinned repositories required. Maybe a heads up mail to the mirageos-devel mailing list would be worth for such a large change!?

val write: t -> size:int -> (Cstruct.t -> int) -> (unit, error) result Lwt.t
val listen: t -> header_size:int -> (Cstruct.t -> unit Lwt.t) -> (unit, error) result Lwt.t
val write: t -> size:int -> (bytes -> int) -> (unit, error) result Lwt.t
val listen: t -> header_size:int -> (string -> unit Lwt.t) -> (unit, error) result Lwt.t
Copy link
Member Author

Choose a reason for hiding this comment

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

Or maybe we can try to break even more the API:

  val write: t -> off:int -> len:int -> string -> (unit, error) result Lwt.t
  val listen: t -> off:int -> len:int -> bytes -> (unit, error) result Lwt.t

I'm not sure about the names, for example with the write implementations, the callback result is compared to the size argument, and now we have to trust it. Similarly for listen where header_size is now an offset to write the result to, and we have to trust it. As @hannesm pointed out, this boundary checking problem may be now more something about trusting the lower part will work correctly and users will never change that?

Copy link
Member Author

@palainp palainp Oct 1, 2024

Choose a reason for hiding this comment

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

And with that change, the allocation need to be done in the upper layers. 👻

Copy link
Member

Choose a reason for hiding this comment

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

The issue/reason for write having a callback where you can modify the data is that the network interface should be able to decide which buffer you can write to. As example, the mirage-net-xen implementation uses page-aligned memory, and for it to not need to copy/blit any data from the above layers, it passes the buffer to the client.

Now, that may be wishful thinking, since bytes isn't the kind of memory mirage-net-xen can use (it needs non-moving memory since that memory region is passed to the other xen domain, and only when the other says "ok, can be freed" the memory can be reclaimed). [that's at least how I understand it] -- this also means that we need to copy in any case.

Copy link
Member

Choose a reason for hiding this comment

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

I think we'll need to play around with the different APIs and figure what is convenient to use, and what has a reasonable performance. I suspect that if every layer allocates their own data (so, ethernet allocates 14 bytes, ip another 20, ...) and on the write the mirage-net implementation needs to allocate a big buffer and blit everything together, this may suffer from performance issues.

On the other hand, it is worth to try, esp. if we're using string/bytes and not cstruct/bigarray (where allocation is expensive).

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