With a single global count for the claims it's easy to substract
domain_tot_pages() from the claim so the number given in the hypercall
is the real reservation of the domain. This is the current behaviour.
However, a later patch introduces exact-node claims and those interact
very poorly with such a scheme. Since accounting domain_tot_pages() in
one case but not the other seems strictly worse than not accounting them
at all (which is at least consistent), this patch stops substracting
tot_pages from the claim and instead checks that claimed memory +
allocated memory don't exceed max_mem.
Arguably it's also clearer for the caller to align the amount of claimed
memory with that of the requested claim.
Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
xen/common/page_alloc.c | 19 ++++++-------------
1 file changed, 6 insertions(+), 13 deletions(-)
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index bd4538c28d82..49c3258169db 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -523,7 +523,7 @@ out:
int domain_set_outstanding_pages(struct domain *d, unsigned long pages)
{
int ret = -ENOMEM;
- unsigned long claim, avail_pages;
+ unsigned long avail_pages;
/*
* take the domain's page_alloc_lock, else all d->tot_page adjustments
@@ -549,28 +549,21 @@ int domain_set_outstanding_pages(struct domain *d, unsigned long pages)
goto out;
}
- /* disallow a claim not exceeding domain_tot_pages() or above max_pages */
- if ( (pages <= domain_tot_pages(d)) || (pages > d->max_pages) )
+ /* Don't claim past max_pages */
+ if ( (domain_tot_pages(d) + pages) > d->max_pages )
{
ret = -EINVAL;
goto out;
}
/* how much memory is available? */
- avail_pages = total_avail_pages;
+ avail_pages = total_avail_pages - outstanding_claims;
- avail_pages -= outstanding_claims;
-
- /*
- * Note, if domain has already allocated memory before making a claim
- * then the claim must take domain_tot_pages() into account
- */
- claim = pages - domain_tot_pages(d);
- if ( claim > avail_pages )
+ if ( pages > avail_pages )
goto out;
/* yay, claim fits in available memory, stake the claim, success! */
- d->outstanding_pages = claim;
+ d->outstanding_pages = pages;
outstanding_claims += d->outstanding_pages;
ret = 0;
--
2.48.1On 14.03.2025 18:24, Alejandro Vallejo wrote:
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -523,7 +523,7 @@ out:
> int domain_set_outstanding_pages(struct domain *d, unsigned long pages)
> {
> int ret = -ENOMEM;
> - unsigned long claim, avail_pages;
> + unsigned long avail_pages;
>
> /*
> * take the domain's page_alloc_lock, else all d->tot_page adjustments
> @@ -549,28 +549,21 @@ int domain_set_outstanding_pages(struct domain *d, unsigned long pages)
> goto out;
> }
>
> - /* disallow a claim not exceeding domain_tot_pages() or above max_pages */
> - if ( (pages <= domain_tot_pages(d)) || (pages > d->max_pages) )
> + /* Don't claim past max_pages */
> + if ( (domain_tot_pages(d) + pages) > d->max_pages )
In addition to what Roger has said, how can such a behavioral change come without
any caller-side adjustment?
Jan
On Fri, Mar 14, 2025 at 05:24:53PM +0000, Alejandro Vallejo wrote: > With a single global count for the claims it's easy to substract > domain_tot_pages() from the claim so the number given in the hypercall > is the real reservation of the domain. This is the current behaviour. > > However, a later patch introduces exact-node claims and those interact > very poorly with such a scheme. Since accounting domain_tot_pages() in > one case but not the other seems strictly worse than not accounting them > at all (which is at least consistent), this patch stops substracting > tot_pages from the claim and instead checks that claimed memory + > allocated memory don't exceed max_mem. Hm, while I don't have any specific interest in keeping the current behavior, XENMEM_claim_pages is part of the stable ABI (it's not a domctl), and hence should be stable. Note also the comment above the definition of XENMEM_claim_pages how it states the specific behavior that you are trying to change (and which should have been adjusted as part of this change). I have no idea why this was made a xenmem rather than a domctl hypercall, but if you want to change the semantics I think the only option is introducing a new hypercall. Thanks, Roger.
On 05.06.2025 18:42, Roger Pau Monné wrote: > On Fri, Mar 14, 2025 at 05:24:53PM +0000, Alejandro Vallejo wrote: >> With a single global count for the claims it's easy to substract >> domain_tot_pages() from the claim so the number given in the hypercall >> is the real reservation of the domain. This is the current behaviour. >> >> However, a later patch introduces exact-node claims and those interact >> very poorly with such a scheme. Since accounting domain_tot_pages() in >> one case but not the other seems strictly worse than not accounting them >> at all (which is at least consistent), this patch stops substracting >> tot_pages from the claim and instead checks that claimed memory + >> allocated memory don't exceed max_mem. > > Hm, while I don't have any specific interest in keeping the current > behavior, XENMEM_claim_pages is part of the stable ABI (it's not a > domctl), and hence should be stable. Is that true? It sits inside a #if defined(__XEN__) || defined(__XEN_TOOLS__) which generally de-marks unstable (tools-only) interfaces. > Note also the comment above the > definition of XENMEM_claim_pages how it states the specific behavior > that you are trying to change (and which should have been adjusted as > part of this change). This is the more important part, imo. > I have no idea why this was made a xenmem rather than a domctl > hypercall, but if you want to change the semantics I think the only > option is introducing a new hypercall. It _is_ a memory operation, and it re-uses one of the interface structs there. (Yes, none of these would technically have prevented it from being a domctl.) Jan
On Tue Jun 10, 2025 at 2:23 PM CEST, Jan Beulich wrote: > On 05.06.2025 18:42, Roger Pau Monné wrote: >> On Fri, Mar 14, 2025 at 05:24:53PM +0000, Alejandro Vallejo wrote: >>> With a single global count for the claims it's easy to substract >>> domain_tot_pages() from the claim so the number given in the hypercall >>> is the real reservation of the domain. This is the current behaviour. >>> >>> However, a later patch introduces exact-node claims and those interact >>> very poorly with such a scheme. Since accounting domain_tot_pages() in >>> one case but not the other seems strictly worse than not accounting them >>> at all (which is at least consistent), this patch stops substracting >>> tot_pages from the claim and instead checks that claimed memory + >>> allocated memory don't exceed max_mem. >> >> Hm, while I don't have any specific interest in keeping the current >> behavior, XENMEM_claim_pages is part of the stable ABI (it's not a >> domctl), and hence should be stable. > > Is that true? It sits inside a > > #if defined(__XEN__) || defined(__XEN_TOOLS__) > > which generally de-marks unstable (tools-only) interfaces. > >> Note also the comment above the >> definition of XENMEM_claim_pages how it states the specific behavior >> that you are trying to change (and which should have been adjusted as >> part of this change). > > This is the more important part, imo. Ugh. I missed that. Regardless, the current scheme is highly counterintuitive and works only because toolstack colaborates in removing the claim after dom creation. > >> I have no idea why this was made a xenmem rather than a domctl >> hypercall, but if you want to change the semantics I think the only >> option is introducing a new hypercall. > > It _is_ a memory operation, and it re-uses one of the interface structs > there. (Yes, none of these would technically have prevented it from > being a domctl.) > > Jan As Jan mentions, my understanding was that __XEN_TOOLS__ effectively meant unstable. It is quite hard to keep the previous semantics for one hypercall while introducing a new one that behaves slightly differently. Cheers, Alejandro
On Tue, Jun 10, 2025 at 02:23:26PM +0200, Jan Beulich wrote: > On 05.06.2025 18:42, Roger Pau Monné wrote: > > On Fri, Mar 14, 2025 at 05:24:53PM +0000, Alejandro Vallejo wrote: > >> With a single global count for the claims it's easy to substract > >> domain_tot_pages() from the claim so the number given in the hypercall > >> is the real reservation of the domain. This is the current behaviour. > >> > >> However, a later patch introduces exact-node claims and those interact > >> very poorly with such a scheme. Since accounting domain_tot_pages() in > >> one case but not the other seems strictly worse than not accounting them > >> at all (which is at least consistent), this patch stops substracting > >> tot_pages from the claim and instead checks that claimed memory + > >> allocated memory don't exceed max_mem. > > > > Hm, while I don't have any specific interest in keeping the current > > behavior, XENMEM_claim_pages is part of the stable ABI (it's not a > > domctl), and hence should be stable. > > Is that true? It sits inside a > > #if defined(__XEN__) || defined(__XEN_TOOLS__) > > which generally de-marks unstable (tools-only) interfaces. Ops, my bad, I didn't realize it was inside such region. > > Note also the comment above the > > definition of XENMEM_claim_pages how it states the specific behavior > > that you are trying to change (and which should have been adjusted as > > part of this change). > > This is the more important part, imo. I see. Well, it's in a kind of a weird position then, because there's no equivalent of XEN_DOMCTL_INTERFACE_VERSION that we could use to signal callers of the changed interface, like we do for domctl. Thanks, Roger.
© 2016 - 2025 Red Hat, Inc.