Re: [MirageOS-devel] wireshark capture of failed download from mirage-www on ARM

[Dave Scott <Dave.Scott@xxxxxxxxxx>]

Hi,

Sorry for the delay replying!

On 31 Jul 2014, at 11:44, Thomas Leonard <talex5@xxxxxxxxx> wrote:

> On 26 July 2014 10:27, David Scott <scott.dj@xxxxxxxxx> wrote:
>> 
>> 
>> On Saturday, July 26, 2014, Anil Madhavapeddy <anil@xxxxxxxxxx> wrote:
>>> 
>>> On 25 Jul 2014, at 04:54, Thomas Leonard <talex5@xxxxxxxxx> wrote:
>>>> 
>>>> Here's a slightly better work-around:
>>>> 
>>>> 
>>>> https://github.com/talex5/mirage-tcpip/commit/9f7e0a628ebd1cece7d0fb979273a96be78233ad
>>>> 
>>>> It changes the TCP packet splitting code so that the second part of
>>>> the page is copied to a new IO page. This avoids sending the same
>>>> physical page to Linux twice (which is what doesn't work).
>>>> 
>>>> With this, the previous hack can be reverted.
>>>> 
>>>> In my testing, this increased the streaming TCP download speed from 56
>>>> KB/s to 3.9 MB/s on the CubieTruck.
>>> 
>>> This is certainly a fix that works, but I'm loath to insert an unnecessary
>>> copy in the TCP stack just because of a limitation in Xen/ARM (that will
>>> go
>>> away in Xen 4.5, I expect).
>>> 
>>> Could we keep track of the active grants in the Gnttab libary, and force a
>>> page duplication at that point instead?  That would fix it for any other
>>> Cstruct consumers as well, and be a more logical place to insert Xen
>>> version detection in the future to remove the extra copy if not needed.
>> 
>> 
>> If the problem is granting the same page twice, would it be enough to
>> maintain a map of page address to (grant index * refount), and then
>> "grant_access" could return the existing grant id and bump the refcount? I
>> think we just need to share the grant, rather than duplicate the page. (Is
>> that right?)
> 
> How would that fit in with the current API? It looks like you first
> allocate a gntref (an int) and then use that to share the page:
> 
>  val get : unit -> gntref Lwt.t
> 
>  val grant_access : domid:int -> writable:bool -> gntref -> Io_page.t -> unit

Yeah looking at it I think that interface is a bit too ‘raw’. Unfortunately 
‘type gntref = int’ in the interface so clients are free to do things like
```
         ignore(RX.Proto_64.write ~id ~gref:(Int32.of_int gref) slot)
```
I had been hoping to exploit the observation that (I think all) users call 
‘grant_access’ before serialising the grant reference into the ring metadata 
slot. My evil/cunning plan had been to allocate a fresh grant in ‘get()’ as 
normal, but then swizzle it inside ‘grant_access’ if it turns out that a grant 
has already been allocated in that page. However I would need to change the 
implementation of ‘type gntref’ to make this work :-(

Anyway, looking at the current users in mirage-{block,net}-xen, I can see 2 
users of the low-level API:

  1. granting access to the metadata ring on startup e.g.
```
let allocate_ring ~domid =
  let page = Io_page.get 1 in
  let x = Io_page.to_cstruct page in
  lwt gnt = Gnt.Gntshr.get () in
  for i = 0 to Cstruct.len x - 1 do
    Cstruct.set_uint8 x i 0
  done;
  Gnt.Gntshr.grant_access ~domid ~writable:true gnt page;
  return (gnt, x)
```
  2. granting access to pages for I/O e.g.
```
let refill_requests nf =
  let num = Ring.Rpc.Front.get_free_requests nf.rx_fring in
  if num > 0 then
    lwt grefs = Gnt.Gntshr.get_n num in
    let pages = Io_page.pages num in
    List.iter
      (fun (gref, page) ->
         let id = gref mod (1 lsl 16) in
         Gnt.Gntshr.grant_access ~domid:nf.backend_id ~writable:true gref page;
         Hashtbl.add nf.rx_map id (gref, page);
         let slot_id = Ring.Rpc.Front.next_req_id nf.rx_fring in
         let slot = Ring.Rpc.Front.slot nf.rx_fring slot_id in
         ignore(RX.Proto_64.write ~id ~gref:(Int32.of_int gref) slot)
      ) (List.combine grefs pages);
    if Ring.Rpc.Front.push_requests_and_check_notify nf.rx_fring
    then notify nf ();
    return ()
  else return ()
```

We’ve already got a ‘with_grants’ function but this doesn’t work for these 
cases because they both want to ‘leak’ the grants (they are cleaned up 
elsewhere). Perhaps we should make an API (‘return_grants’ ‘alloc_grants’?) 
which combines ‘get_n’ and ‘grant_access’?

Do you know if the bug is likely to be fixed in the backend in a reasonable 
timeframe? Or is it worth changing the API soon so we could make a workaround 
in the frontend?

Cheers,
Dave


_______________________________________________
MirageOS-devel mailing list
MirageOS-devel@xxxxxxxxxxxxxxxxxxxx
http://lists.xenproject.org/cgi-bin/mailman/listinfo/mirageos-devel