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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 31/07/2019 09:22, Jan Beulich wrote:
> On 30.07.2019 19:32, Andrew Cooper wrote:
>> 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.
> Oh, right - I forgot that we (almost) never clear bits from
> node_online_map. There's one use of node_set_offline() in
> memory_add() - I wonder whether we shouldn't ditch
> node_set_offline() to make more visible that we don't mean
> to ever clear bits there.
>
>> 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;
> Right, and then you don't need the nodes_empty() part anymore.

Good point.

> With this change folded in (or as a prereq one to allow backporting) you
> can add my R-b with the adjustment further up in place.

I think I'll fold it together into a single change.  Its directly
relevant to the subject.

~Andrew

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