[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • From: Jan Beulich <JBeulich@xxxxxxxx>
  • Date: Wed, 31 Jul 2019 08:22:23 +0000
  • Accept-language: en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1;spf=pass smtp.mailfrom=suse.com;dmarc=pass action=none header.from=suse.com;dkim=pass header.d=suse.com;arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=tF+W9Kgg86ZexYEBx+wWz7RSsGIi1esGC9+N3MylO84=; b=IwPpUFfKjk12x2GMeuJRl8cf693T0x6547rVLj87sWi27KqFBlCoBEICu1oeDd9UTw4DYpm/XJTNQwTOUS1YUJv9ncSRKhjOvvRAuw1TxawMKLPc99aRZZefMYn6/5fnfmWLNNLcErQiZQF8brZIhEZfVJ9+vj4JBt09c4+LHA0EtQOnJeSJfpoILoe9NLWq1EpZ1r0Lfi2Oxq1z2iONEw/Z9yPv1mvgUrVRN53bk0aLxOMneaj/viNl+DfQIisv/xE/6/PlcWQlyZ+PLDWUQgHMjd1FOVvb15hwJoFd3jLQnQHs8Zeh53vdA9yJ3cS2RbvuRmV/Gx3W2430dcJwyQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=GS4hep1rMVI3AKZiPUE1K1+7uNgX8rX6NR/as2DOniZAOVEJJ7KRhBD8JLWqET7wrcvGBM6PyUK3/TOUTIACDmsfGJ4tKVd5/oL1zPyINwx4u76fTDi1dstxI0v3mE40/aGw8q4WK/srXS+CCH3pTNsjHjip4I3usQ5CHhOylXSpno/50tTYnTqy2Ac2AE2sAfBrHmu+/QhO5qB2bE5YE+TgdNsqtbWF5sO/V5yqdIVYTNrvFEd49Nl2wYjjv2CeH+E6vuSmvLgzZZ/RPLrW6uSRKrasDMl45nZy3WzpK++mjY3YE4kBsXG4g4b7lvPMnSWD8DbnY089zaWH3kyMZw==
  • Authentication-results: spf=none (sender IP is ) smtp.mailfrom=JBeulich@xxxxxxxx;
  • Cc: StefanoStabellini <sstabellini@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxxxxx>, Julien Grall <julien.grall@xxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Delivery-date: Wed, 31 Jul 2019 08:38:09 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Thread-index: AQHVRgcXbA5oo5qfM0G3fbP8NjBdsqbhvgyAgAAbkuCAAPZygIAAnfGXgAD4FQA=
  • Thread-topic: [Xen-devel] [PATCH v3 01/10] page-alloc: Clamp get_free_buddy() to online nodes

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.