Re: [Xen-devel] [PATCH v3 09/10] xen/nodemask: Sanitise the remainder of the nodemask API

From: Jan Beulich <JBeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: StefanoStabellini <sstabellini@kernel.org>,
	"Wei Liu" <wl@xen.org>,
	"George Dunlap" <george.dunlap@eu.citrix.com>,
	"Julien Grall" <julien.grall@arm.com>,
	Xen-devel <xen-devel@lists.xenproject.org>,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH v3 09/10] xen/nodemask: Sanitise the remainder of the nodemask API
Date: Tue, 30 Jul 2019 11:43:12 +0000	[thread overview]
Message-ID: <bb272ab1-c63c-0d5a-9360-d7a8a22ee52e@suse.com> (raw)
In-Reply-To: <20190729121204.13559-10-andrew.cooper3@citrix.com>

On 29.07.2019 14:12, Andrew Cooper wrote:
> --- a/xen/arch/x86/srat.c
> +++ b/xen/arch/x86/srat.c
> @@ -332,7 +332,7 @@ acpi_numa_memory_affinity_init(const struct acpi_srat_mem_affinity *ma)
>   	if (!(ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE)) {
>   		struct node *nd = &nodes[node];
>   
> -		if (!node_test_and_set(node, memory_nodes_parsed)) {
> +		if (!nodemask_test_and_set(node, &memory_nodes_parsed)) {

Shouldn't this have become __nodemask_test_and_set() already in the
previous patch (with the leading underscores taken care of in
whatever way we decide there)?

> @@ -376,7 +376,7 @@ static int __init nodes_cover_memory(void)
>   
>   		do {
>   			found = 0;
> -			for_each_node_mask(j, memory_nodes_parsed)
> +			for_each_node_mask( j, &memory_nodes_parsed )

Here and elsewhere - if you add the inner blanks, then there also
wants to be a blank ahead of the opening parenthesis.

> @@ -1171,7 +1171,7 @@ static unsigned int node_to_scrub(bool get_node)
>           node = 0;
>   
>       if ( node_need_scrub[node] &&
> -         (!get_node || !node_test_and_set(node, node_scrubbing)) )
> +         (!get_node || !nodemask_test_and_set(node, &node_scrubbing)) )
>           return node;
>   
>       /*
> @@ -1205,10 +1205,10 @@ static unsigned int node_to_scrub(bool get_node)
>                * then we'd need to take this lock every time we come in here.
>                */
>               if ( (dist < shortest || closest == NUMA_NO_NODE) &&
> -                 !node_test_and_set(node, node_scrubbing) )
> +                 !nodemask_test_and_set(node, &node_scrubbing) )
>               {
>                   if ( closest != NUMA_NO_NODE )
> -                    node_clear(closest, node_scrubbing);
> +                    nodemask_clear(closest, &node_scrubbing);
>                   shortest = dist;
>                   closest = node;
>               }
> @@ -1360,7 +1360,7 @@ bool scrub_free_pages(void)
>       spin_unlock(&heap_lock);
>   
>    out_nolock:
> -    node_clear(node, node_scrubbing);
> +    nodemask_clear(node, &node_scrubbing);

Seeing this happen after dropping the heap lock, I think the two
nodemask_test_and_set() that I've left in context above could be
converted to their non-locked counterparts at this occasion (or
again in the previous patch).

>   #if MAX_NUMNODES > 1
>   #define for_each_node_mask(node, mask)			\
> -	for ((node) = first_node(mask);			\
> +	for ((node) = nodemask_first(mask);		\
>   		(node) < MAX_NUMNODES;			\
> -		(node) = next_node((node), (mask)))
> +		(node) = nodemask_next(node, mask))
>   #else /* MAX_NUMNODES == 1 */
>   #define for_each_node_mask(node, mask)			\
> -	if (!nodes_empty(mask))				\
> +	if ( !nodemask_empty(mask) )			\
>   		for ((node) = 0; (node) < 1; (node)++)
>   #endif /* MAX_NUMNODES */

Further up in the file you did also replace tabs and insert spaces.
Would you mind doing this here as well (I notice you've adjusted
the if() already, but left the for()s alone).

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