Re: [Xen-devel] [PATCH] xen: issue warning message when out of grant maptrack entries

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 9/18/18 1:17 PM, Juergen Gross wrote:
> On 18/09/18 19:03, Boris Ostrovsky wrote:
>> On 9/18/18 5:32 AM, Juergen Gross wrote:
>>> When a driver domain (e.g. dom0) is running out of maptrack entries it
>>> can't map any more foreign domain pages. Instead of silently stalling
>>> the affected domUs issue a rate limited warning in this case in order
>>> to make it easier to detect that situation.
>>>
>>> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
>>> ---
>>>  drivers/xen/grant-table.c | 25 +++++++++++++++++++------
>>>  1 file changed, 19 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
>>> index 7bafa703a992..09f6ff8c1957 100644
>>> --- a/drivers/xen/grant-table.c
>>> +++ b/drivers/xen/grant-table.c
>>> @@ -1040,18 +1040,31 @@ int gnttab_map_refs(struct gnttab_map_grant_ref 
>>> *map_ops,
>>>             return ret;
>>>  
>>>     for (i = 0; i < count; i++) {
>>> -           /* Retry eagain maps */
>>> -           if (map_ops[i].status == GNTST_eagain)
>>> -                   gnttab_retry_eagain_gop(GNTTABOP_map_grant_ref, map_ops 
>>> + i,
>>> -                                           &map_ops[i].status, __func__);
>>> -
>>> -           if (map_ops[i].status == GNTST_okay) {
>>> +           switch (map_ops[i].status) {
>>> +           case GNTST_okay:
>>> +           {
>>>                     struct xen_page_foreign *foreign;
>>>  
>>>                     SetPageForeign(pages[i]);
>>>                     foreign = xen_page_foreign(pages[i]);
>>>                     foreign->domid = map_ops[i].dom;
>>>                     foreign->gref = map_ops[i].ref;
>>> +                   break;
>>> +           }
>>> +
>>> +           case GNTST_no_device_space:
>>> +                   pr_warn_ratelimited("maptrack limit reached, can't map 
>>> all guest pages\n");
>>> +                   break;
>>> +
>>> +           case GNTST_eagain:
>>> +                   /* Retry eagain maps */
>>> +                   gnttab_retry_eagain_gop(GNTTABOP_map_grant_ref,
>>> +                                           map_ops + i,
>>> +                                           &map_ops[i].status, __func__);
>>> +                   break;
>>> +
>>> +           default:
>>> +                   break;
>>>             }
>>>     }


After having a second look at this (and at the risk of embarrassing
myself again with this patch) --- aren't we supposed to test status
after gnttab_retry_eagain_gop() call, and then actually set foreign
properties?

-boris


>>
>> Should we pass 'i' instead of count to set_foreign_p2m_mapping() below?
>> The loop there will skip entries that are in error, but does it make
>> sense to do the hypercall for kmap_ops with count>i ?
> The loop is running until the end, so i == count for the call of kmap_ops().
>
>
> Juergen


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel