All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jan Beulich <jbeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@citrix.com>,
	Ian Jackson <iwj@xenproject.org>, Julien Grall <julien@xen.org>,
	Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH 1/9] gnttab: defer allocation of maptrack frames table
Date: Mon, 6 Sep 2021 16:43:54 +0200	[thread overview]
Message-ID: <d26de0f2-12b5-4b3c-0956-0c2d2aa977b9@suse.com> (raw)
In-Reply-To: <c3c20938-ba34-965b-0f1e-8d72c8004cc6@citrix.com>

On 06.09.2021 16:05, Andrew Cooper wrote:
> On 26/08/2021 11:09, Jan Beulich wrote:
>> By default all guests are permitted to have up to 1024 maptrack frames,
>> which on 64-bit means an 8k frame table. Yet except for driver domains
>> guests normally don't make use of grant mappings. Defer allocating the
>> table until a map track handle is first requested.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Nack.  This creates new dynamic failures outside the VM's control,
> therefore regressing Xen's usability.

As with the v2 status frames tracking array, the memory demand of
the actual table will typically be quite a bit higher than that of
the frame tracking table. Therefore guests already are at risk of
observing failures from these paths; the increase of that risk from
the change here is typically quite small.

> The maptrack array (and frames for that matter) should be allocated at
> domain creation time, like we do for most other structures in the
> hypervisor.

Structures we allocate at domain creation time are typically ones
which we know will get used (to at least some degree), or where
allocation can't be done later because paths this would be needed on
have no way to indicate respective failure. Neither of this is true
for the maptrack frame table (or the v2 status frame table, for that
matter).

This said, I could see us _switch_ to a policy like the one you
describe, or even allow either behavior depending on some kind of
setting. But then this needs to be done consistently for all forms
of resources (e.g. also the gnttab v2 frames, not just the frame
table).

> Furthermore, seeing as this has come up on multiple community calls, I
> will remind you that it is not just Citrix as a downstream which is
> firmly of this opinion.

No-one but you has voiced such an opinion, so far.

> This entire patch should be replaced with one that...
> 
>> ---
>> I continue to be unconvinced that it is a good idea to allow all DomU-s
>> 1024 maptrack frames by default.
> 
> ... defaults to something smaller for plain domUs, because this improves
> the general case without leaving VMs more liable to crash at runtime.

Just to continue to waste memory? Or to force the admin to know very
precisely how many maptrack entries a guest may need to create, such
that they will neither cause large amounts of unused memory nor risk
the guest running out of handles?

Jan



  reply	other threads:[~2021-09-06 14:44 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-26 10:06 [PATCH 0/9] gnttab: further work from XSA-380 / -382 context Jan Beulich
2021-08-26 10:09 ` [PATCH 1/9] gnttab: defer allocation of maptrack frames table Jan Beulich
2021-09-06 13:13   ` Julien Grall
2021-09-06 13:29     ` Jan Beulich
2021-09-06 13:33       ` Julien Grall
2021-09-06 13:58         ` Jan Beulich
2021-09-06 14:05   ` Andrew Cooper
2021-09-06 14:43     ` Jan Beulich [this message]
2021-08-26 10:11 ` [PATCH 2/9] gnttab: drop a redundant expression from gnttab_release_mappings() Jan Beulich
2021-09-06 13:15   ` Julien Grall
2021-08-26 10:12 ` [PATCH 3/9] gnttab: fold recurring is_iomem_page() Jan Beulich
2021-09-06 13:35   ` Julien Grall
2021-09-06 14:02     ` Jan Beulich
2021-08-26 10:13 ` [PATCH 4/9] gnttab: drop GNTMAP_can_fail Jan Beulich
2021-08-26 11:45   ` Andrew Cooper
2021-08-26 13:03     ` Jan Beulich
2021-08-26 13:13       ` Andrew Cooper
2021-08-26 10:13 ` [PATCH 5/9] gnttab: defer allocation of status frame tracking array Jan Beulich
2021-08-26 10:13 ` [PATCH 6/9] gnttab: check handle early in gnttab_get_status_frames() Jan Beulich
2021-09-06 13:41   ` Julien Grall
2021-08-26 10:14 ` [PATCH 7/9] gnttab: no need to translate handle for gnttab_get_status_frames() Jan Beulich
2022-10-07 18:24   ` Andrew Cooper
2021-08-26 10:14 ` [PATCH 8/9] gnttab: bail from GFN-storing loops early in case of error Jan Beulich
2022-10-07 19:36   ` Andrew Cooper
2022-10-10  9:30     ` Jan Beulich
2021-08-26 10:15 ` [PATCH 9/9] gnttab: don't silently truncate GFNs in compat setup-table handling Jan Beulich
2022-10-07 19:57   ` Andrew Cooper
2022-10-10  8:46     ` Jan Beulich
2022-01-05  7:42 ` Ping: [PATCH 0/9] gnttab: further work from XSA-380 / -382 context Jan Beulich
2022-10-07 13:49 ` Jan Beulich
2022-10-07 18:09   ` Andrew Cooper

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=d26de0f2-12b5-4b3c-0956-0c2d2aa977b9@suse.com \
    --to=jbeulich@suse.com \
    --cc=andrew.cooper3@citrix.com \
    --cc=george.dunlap@citrix.com \
    --cc=iwj@xenproject.org \
    --cc=julien@xen.org \
    --cc=sstabellini@kernel.org \
    --cc=wl@xen.org \
    --cc=xen-devel@lists.xenproject.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.