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

[Jan Beulich <JBeulich@xxxxxxxx>]

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. 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.

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