* [PATCH] xen: issue warning message when out of grant maptrack entries
@ 2018-09-18 9:32 Juergen Gross
2018-09-18 17:03 ` Boris Ostrovsky
0 siblings, 1 reply; 6+ messages in thread
From: Juergen Gross @ 2018-09-18 9:32 UTC (permalink / raw)
To: linux-kernel, xen-devel; +Cc: boris.ostrovsky, Juergen Gross
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@suse.com>
---
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;
}
}
--
2.16.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] xen: issue warning message when out of grant maptrack entries
2018-09-18 9:32 [PATCH] xen: issue warning message when out of grant maptrack entries Juergen Gross
@ 2018-09-18 17:03 ` Boris Ostrovsky
2018-09-18 17:17 ` Juergen Gross
0 siblings, 1 reply; 6+ messages in thread
From: Boris Ostrovsky @ 2018-09-18 17:03 UTC (permalink / raw)
To: Juergen Gross, linux-kernel, xen-devel
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@suse.com>
> ---
> 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;
> }
> }
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 ?
-boris
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen: issue warning message when out of grant maptrack entries
2018-09-18 17:03 ` Boris Ostrovsky
@ 2018-09-18 17:17 ` Juergen Gross
2018-09-18 17:25 ` Boris Ostrovsky
2018-09-19 13:29 ` Boris Ostrovsky
0 siblings, 2 replies; 6+ messages in thread
From: Juergen Gross @ 2018-09-18 17:17 UTC (permalink / raw)
To: Boris Ostrovsky, linux-kernel, xen-devel
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@suse.com>
>> ---
>> 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;
>> }
>> }
>
>
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen: issue warning message when out of grant maptrack entries
2018-09-18 17:17 ` Juergen Gross
@ 2018-09-18 17:25 ` Boris Ostrovsky
2018-09-19 13:29 ` Boris Ostrovsky
1 sibling, 0 replies; 6+ messages in thread
From: Boris Ostrovsky @ 2018-09-18 17:25 UTC (permalink / raw)
To: Juergen Gross, linux-kernel, xen-devel
On 9/18/18 1:17 PM, Juergen Gross wrote:
>
> The loop is running until the end, so i == count for the call of kmap_ops().
OK, so I can't read code.
Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen: issue warning message when out of grant maptrack entries
2018-09-18 17:17 ` Juergen Gross
2018-09-18 17:25 ` Boris Ostrovsky
@ 2018-09-19 13:29 ` Boris Ostrovsky
2018-09-19 13:34 ` Juergen Gross
1 sibling, 1 reply; 6+ messages in thread
From: Boris Ostrovsky @ 2018-09-19 13:29 UTC (permalink / raw)
To: Juergen Gross, linux-kernel, xen-devel
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@suse.com>
>>> ---
>>> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen: issue warning message when out of grant maptrack entries
2018-09-19 13:29 ` Boris Ostrovsky
@ 2018-09-19 13:34 ` Juergen Gross
0 siblings, 0 replies; 6+ messages in thread
From: Juergen Gross @ 2018-09-19 13:34 UTC (permalink / raw)
To: Boris Ostrovsky, linux-kernel, xen-devel
On 19/09/18 15:29, Boris Ostrovsky wrote:
> 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@suse.com>
>>>> ---
>>>> 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?
Right. Thanks for catching that!
V2 coming soon...
Juergen
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-09-19 13:34 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-18 9:32 [PATCH] xen: issue warning message when out of grant maptrack entries Juergen Gross
2018-09-18 17:03 ` Boris Ostrovsky
2018-09-18 17:17 ` Juergen Gross
2018-09-18 17:25 ` Boris Ostrovsky
2018-09-19 13:29 ` Boris Ostrovsky
2018-09-19 13:34 ` Juergen Gross
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).