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
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
