Re: [Xen-devel] [PATCH XEN v6 25/32] tools/libs/gnttab: Extensive updates to API documentation.

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Ian Campbell writes ("[PATCH XEN v6 25/32] tools/libs/gnttab: Extensive updates 
to API documentation."):
> In particular around error handling, behaviour on fork and the unmap
> notification mechanism.
> 
> Behaviour of xengnttab_map_*grant_refs and xengntshr_share_pages on
> partial failure has been confirmed/inferred (by inspection) on Linux
> and Mini-os (the only two known implementations. Likewise the
> behaviour of the notification mechanism has been confirmed/inferred
> (by inspection) of the Linux implementation (currently the only
> implementation) and libvchan (primary known user).
> 
> These updates are not folded into "tools: Refactor
> /dev/xen/gnt{dev,shr} wrappers into libxengnttab." to try and reduce
> the amount of non-movement changes in that patch.
> 
> While I'm not convinced by javadoc/doxygen cause the existing comments
> which appear to use that syntax to have the appropriate /** marker.
> 
> Also fix a typo in a code comment.
> 
> Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> Cc: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
> ---
> Daniel, you input on the description of the unmap notification stuff
> would be much appreciated.
> 
> v6: Rewrapped a comment.
>     Incorported much review from Ian on the API, retitled (was:
>     "tools/libs/gnttab: Review and update doc comments.") and dropped
>     acks
> ---
>  tools/libs/gnttab/include/xengnttab.h | 166 
> ++++++++++++++++++++++++++++------
>  tools/libs/gnttab/linux.c             |  12 ++-
>  2 files changed, 147 insertions(+), 31 deletions(-)
> 
> diff --git a/tools/libs/gnttab/include/xengnttab.h 
> b/tools/libs/gnttab/include/xengnttab.h
> index c810c07..318b978 100644
> --- a/tools/libs/gnttab/include/xengnttab.h
> +++ b/tools/libs/gnttab/include/xengnttab.h
> @@ -31,28 +31,111 @@
>  typedef struct xentoollog_logger xentoollog_logger;
>  
>  /*
> + * PRODUCING AND CONSUMING GRANT REFERENCES
> + * ========================================
> + *
> + * The xengnttab library contains two distinct interfaces, each with
> + * their own distinct handle type and entry points. The represent the
> + * two sides of the grant table interface, producer (gntshr) and
> + * consumer (gnttab).
> + *
> + * The xengnttab_* interfaces take a xengnttab_handle and provide
> + * mechanisms for consuming (i.e. mapping or copying to/from) grant
> + * references provided by a peer.
> + *
> + * The xengntshr_* interfaces take a xengntshr_handle and provide a
> + * mechanism to produce grantable memory and grant references to that
> + * memory, which can be handed to some peer.
> + *
> + * UNMAP NOTIFICATION
> + * ==================
> + *
> + * The xengnt{tab,shr}_*_nofity interfaces implement a best effort
                            ^^^^^^

I would say `cooperative' rather than `best effort' (the latter
implying it may fail due to kernel or hypervisor trouble, rather than
malice).

> + * interface which is intended to allow the underlying kernel
> + * interfaces to attempt a graceful teardown of the peer upon failure
> + * (i.e. crash or exit) of the process on their end.

I would write `... to attempt to notify the peer to perform graceful
teardown ...' rather than `to attempt graceful teardown of the peer'.

> + * Depending on the mechanisms which have been registered an
> + * application may receive a shutdown notification as:

I would continue to use `the peer' to avoid swapping the point of
view.  So `Depending ... the peer may receive ...'

> + * Upon receiving notification of their peer's shutdown an application
> + * is expected to teardown any resources (and in particular any grant
> + * mappings) in a timely manner.

`Upon receiving notification the peer is expected ...'

>  /*
> - * Note:
> - * After fork a child process must not use any opened xc gnttab
> - * handle inherited from their parent. They must open a new handle if
> - * they want to interact with xc.
> + * Returns a handle onto the grant table driver.  Logs errors.
> + *
> + * Note: After fork a child process must not use any opened gnttab
> + * handle inherited from their parent, more access any grant mapped
                                          ^^^^
> + * areas associated with that handle.

You mean `nor'.

> + * The child must open a new handle if they want to interact with
> + * gnttab.
>   *
> - * Return an fd onto the grant table driver.  Logs errors.
> + * A child may safely call xengnttab_close() on a xengnttab_handle
> + * inherited from a parent and this will safely reclaim any virtual
> + * address space used by mappings associated with that handle.

Is this actually true ?  Normally close() on an fd does not munmap
anything mmapped from the fd.

> + * On failure sets errno and returns NULL.
> + *
> + * If notify_offset or notify_port a requested and cannot be set up an
                                      ^
`are'

>  /*
> - * Return an fd onto the grant sharing driver.  Logs errors.
> + * Returns a handle onto the grant sharing driver.  Logs errors.
> + *
> + * The child must open a new handle if they want to interact with
> + * gntshr.

You probably want to mention `fork' in this sentence, or move it after
the Note:.

> +/**
>   * Creates and shares pages with another domain.
>   *

I think you mean `Allocates' not `Creates'.  (Throughout.)  Userland
people are used to thinking of memory as being allocated.

It might be worth mentioning that the API does not permit the sharing
of memory which is allocated in some other way (eg, malloc, mmap).

> diff --git a/tools/libs/gnttab/linux.c b/tools/libs/gnttab/linux.c
> index a76f17f..7ff20ba 100644
> --- a/tools/libs/gnttab/linux.c
> +++ b/tools/libs/gnttab/linux.c
> @@ -132,12 +132,14 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
>      if (addr == MAP_FAILED && errno == EAGAIN)
>      {
>          /*
> -         * The grant hypercall can return EAGAIN if the granted page is
> -         * swapped out. Since the paging daemon may be in the same domain, 
> the
> -         * hypercall cannot block without causing a deadlock.
> +         * The grant hypercall can return EAGAIN if the granted page
> +         * is swapped out. Since the paging daemon may be in the same
> +         * domain, the hypercall cannot block without causing a
> +         * deadlock.
>           *
> -         * Because there are no notificaitons when the page is swapped in, 
> wait
> -         * a bit before retrying, and hope that the page will arrive 
> eventually.
> +         * Because there are no notifications when the page is swapped
> +         * in, wait a bit before retrying, and hope that the page will
> +         * arrive eventually.

WTF.  But not a blocker.


Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel