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

[Andres Lagar-Cavilla <andreslc@xxxxxxxxxxxxxx>]

On Aug 31, 2012, at 10:32 AM, David Vrabel wrote:

> On 27/08/12 17:51, andres@xxxxxxxxxxxxxxxx wrote:
>> From: Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxxxx>
>> 
>> 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?

All operations retry. The reason why I could not make it as elegant as you 
suggest is because grant operations are submitted in batches and their 
status(es?) later checked individually elsewhere. This is the case for netback. 
Note that both blkback and gntdev use a more linear structure with the 
gnttab_map_refs helper, which allows me to hide all the retry gore from those 
drivers into grant table code. Likewise for xenbus ring mapping.

In summary, outside of core grant table code, only the netback driver needs to 
check explicitly for retries, due to its batch-copy-delayed-per-slot-check 
structure.

> 
>> +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?
Good idea (MAX_DELAY == 256). I'd like to get Konrad's feedback before a 
re-spin.

> 
> Would it be sensible to ramp the delay faster?  Perhaps double each
> iteration with a maximum possible delay of e.g., 256 ms.
Generally speaking we've never seen past three retries. I am open to changing 
the algorithm but there is a significant possibility it won't matter at all.

> 
>> +#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.

I want to retain the original context for debugging. Eventually we print 
__func__ if things go wrong.

Thanks, great feedback
Andres

> 
> David


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