Re: [Xen-devel] [PATCH v3 01/10] page-alloc: Clamp get_free_buddy() to online nodes

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 30/07/2019 09:09, Jan Beulich wrote:
> On 29.07.2019 19:26, Andrew Cooper wrote:
>> On 29/07/2019 16:48, Jan Beulich wrote:
>>> On 29.07.2019 14:11, Andrew Cooper wrote:
>>>> +    if ( d )
>>>> +        nodes_and(nodemask, nodemask, d->node_affinity);
>>> Despite my earlier ack: Code further down assumes a non-empty mask,
>>> which is no longer guaranteed afaics.
>> Nothing previous guaranteed that d->node_affinity had any bits set in
>> it, either.
>>
>> That said, in practice it is either ALL, or something derived from the
>> cpu=>node mappings, so I don't think this is a problem in practice.
>>
>>> I think you want to append an
>>> "intersects" check in the if().
>> I think it would be better to assert that callers don't give us complete
>> junk.
>>
>>> With that feel free to promote my
>>> A-b to R-b.
>> How about:
>>
>>      if ( d )
>>      {
>>          if ( nodes_intersect(nodemask, d->node_affinity) )
>>              nodes_and(nodemask, nodemask, d->node_affinity);
>>          else
>>              ASSERT_UNREACHABLE();
>>      }
>>
>> ?
>>
>> This change has passed my normal set of prepush checks (not not that
>> there is anything interesting NUMA-wise in there).
> domain_update_node_affinity() means to guarantee a non-empty mask (by
> way of a similar assertion), when ->auto_node_affinity is set. Otoh
> domain_set_node_affinity() may clear that flag, at which point I can't
> see what would guarantee that the intersection would remain non-empty
> as CPUs get offlined.

I don't see what CPU offlining has to do with anything.  There is no
such thing as taking a node out of the node_online_map, nor should there
be - even if we offline an entire socket's worth of CPUs, the memory
controller is still active and available for use.

The domain always has non-zero vCPUs, which will always result in an
intersection with node_online_map.

What is a problem is XEN_DOMCTL_setnodeaffinity being called with node
mask which is disjoint to node_online_map to begin with.

This problematic behaviour already exists today, and I bet there is a
lot of fun to had with that hypercall.

As a first pass,

diff --git a/xen/common/domain.c b/xen/common/domain.c
index 9aefc2a680..57c84cdc42 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -631,8 +631,9 @@ void domain_update_node_affinity(struct domain *d)
 
 int domain_set_node_affinity(struct domain *d, const nodemask_t *affinity)
 {
-    /* Being affine with no nodes is just wrong */
-    if ( nodes_empty(*affinity) )
+    /* Being affine with no nodes, or disjoint with the system, is
wrong. */
+    if ( nodes_empty(*affinity) ||
+         !nodes_intersects(*affinity, node_online_map) )
         return -EINVAL;
 
     spin_lock(&d->node_affinity_lock);

ought to work, and make it safe to retain the ASSERT() in the heap
allocator.

~Andrew

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