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

gntshr stubs unnecessarily inefficient #2

Closed
hannesm opened this issue Mar 15, 2018 · 1 comment
Closed

gntshr stubs unnecessarily inefficient #2

hannesm opened this issue Mar 15, 2018 · 1 comment

Comments

@hannesm
Copy link
Member

hannesm commented Mar 15, 2018

copied from mirage/mirage-platform#110 by @djs55

The stub does this:

CAMLprim value
stub_gntshr_grant_access(value v_ref, value v_iopage, value v_domid, value v_wri
table)
{
    grant_ref_t ref = Int_val(v_ref);
    void *page = base_page_of(v_iopage);
    gntshr_grant_access(ref, page, Int_val(v_domid), !Bool_val(v_writable));

    return Val_unit;
}

where base_page_of looks up the data pointer of the Caml_ba_array_val. This means that to share a contiguous batch of pages we need to use Io_page.get_n (allocating a large page-aligned bigarray) and then use Io_page.to_pages to chop the buffer into separate pages, via Bigarray.Array1.sub.

It would be better to call Io_page.(to_cstruct (get_n ...)) and then to chop the Cstruct into pages since Cstruct.sub is cheaper than Bigarray.Array1.sub. We would modify the stub to take a Cstruct and compute the real page pointer by adding the buffer.Cstruct.off field.

Our code is using a mixture of Io_page.t and Cstruct.t which is a bit confusing. I think we sometimes use Io_page.t to guarantee alignment but this means we end up doing Bigarray.Array1.sub more often than we should and, if we don't do that and use Cstruct.sub instead, we have hard-to-find bugs where the wrong page is granted because of base_page_of.

djs55 pushed a commit to djs55/mirage-xen that referenced this issue Sep 22, 2019
make printf nicer by not dumping the entire buffer, only up to sz
@mato
Copy link
Contributor

mato commented Oct 16, 2020

No longer applicable now that #23 has been merged and the gnttab code has been re-written; closing.

@mato mato closed this as completed Oct 16, 2020
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

No branches or pull requests

2 participants