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.

Sign up for GitHub

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

Breaking API: Remove cstruct #25

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
97b8306
remove Cstruct.t
palainp Sep 29, 2024
115c526
the data that listen gives to the callback ir read-only
palainp Oct 1, 2024
bb0f321
use bytes instead of Bytes.t
palainp Oct 1, 2024
8cfd794
Merge branch 'mirage:main' into remove-cstruct
palainp Oct 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion mirage-net.opam
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ depends: [
"dune" {>= "1.0"}
"fmt"
"macaddr" {>= "4.0.0"}
"cstruct" {>= "4.0.0"}
"lwt" {>= "4.0.0"}
]
synopsis: "Network signatures for MirageOS"
Expand Down
2 changes: 1 addition & 1 deletion src/dune
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
(library
(name mirage_net)
(public_name mirage-net)
(libraries fmt cstruct macaddr lwt))
(libraries fmt macaddr lwt))
4 changes: 2 additions & 2 deletions src/mirage_net.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@ module type S = sig
val pp_error: error Fmt.t
type t
val disconnect : t -> unit Lwt.t
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 •
edited
Loading

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).

val mac: t -> Macaddr.t
val mtu: t -> int
val get_stats_counters: t -> stats
Expand Down
4 changes: 2 additions & 2 deletions src/mirage_net.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,15 +57,15 @@ module type S = sig
(** Disconnect from the network device. While this might take some time to
complete, it can never result in an error. *)

val write: t -> size:int -> (Cstruct.t -> int) -> (unit, error) result Lwt.t
val write: t -> size:int -> (bytes -> int) -> (unit, error) result Lwt.t
(** [write net ~size fill] allocates a buffer of length [size], where [size]
must not exceed the interface maximum packet size ({!mtu} plus Ethernet
header). The allocated buffer is zeroed and passed to the [fill] function
which returns the payload length, which may not exceed the length of the
buffer. When [fill] returns, a sub buffer is put on the wire: the allocated
buffer from index 0 to the returned length. *)

val listen: t -> header_size:int -> (Cstruct.t -> unit Lwt.t) -> (unit, error) result Lwt.t
val listen: t -> header_size:int -> (string -> unit Lwt.t) -> (unit, error) result Lwt.t
(** [listen ~header_size net fn] waits for a [packet] with size at most
[header_size + mtu] on the network device. When a [packet] is received, an
asynchronous task is created in which [fn packet] is called. The ownership
Expand Down