Re: [PATCH] Xen backend support for paged out grant targets.

[David Vrabel <david.vrabel@citrix.com>]

On 27/08/12 17:51, andres@lagarcavilla.org wrote:
> From: Andres Lagar-Cavilla <andres@lagarcavilla.org>
> 
> Since Xen-4.2, hvm domains may have portions of their memory paged out. When a
> foreign domain (such as dom0) attempts to map these frames, the map will
> initially fail. The hypervisor returns a suitable errno, and kicks an
> asynchronous page-in operation carried out by a helper. The foreign domain is
> expected to retry the mapping operation until it eventually succeeds. The
> foreign domain is not put to sleep because itself could be the one running the
> pager assist (typical scenario for dom0).
> 
> This patch adds support for this mechanism for backend drivers using grant
> mapping and copying operations. Specifically, this covers the blkback and
> gntdev drivers (which map foregin grants), and the netback driver (which copies
> foreign grants).
> 
> * Add GNTST_eagain, already exposed by Xen, to the grant interface.
> * Add a retry method for grants that fail with GNTST_eagain (i.e. because the
>   target foregin frame is paged out).
> * Insert hooks with appropriate macro decorators in the aforementioned drivers.

I think you should implement wrappers around HYPERVISOR_grant_table_op()
have have the wrapper do the retries instead of every backend having to
check for EAGAIN and issue the retries itself. Similar to the
gnttab_map_grant_no_eagain() function you've already added.

Why do some operations not retry anyway?

> +void
> +gnttab_retry_eagain_gop(unsigned int cmd, void *gop, int16_t *status,
> +						const char *func)
> +{
> +	u8 delay = 1;
> +
> +	do {
> +		BUG_ON(HYPERVISOR_grant_table_op(cmd, gop, 1));
> +		if (*status == GNTST_eagain)
> +			msleep(delay++);
> +	} while ((*status == GNTST_eagain) && delay);

Terminating the loop when delay wraps is a bit subtle.  Why not make
delay unsigned and check delay <= MAX_DELAY?

Would it be sensible to ramp the delay faster?  Perhaps double each
iteration with a maximum possible delay of e.g., 256 ms.

> +#define gnttab_map_grant_no_eagain(_gop)                                    \
> +do {                                                                        \
> +    if ( HYPERVISOR_grant_table_op(GNTTABOP_map_grant_ref, (_gop), 1))      \
> +        BUG();                                                              \
> +    if ((_gop)->status == GNTST_eagain)                                     \
> +        gnttab_retry_eagain_map((_gop));                                    \
> +} while(0)

Inline functions, please.

David