* [Xen-devel] [PATCH v3 00/10] xen/nodemask: API cleanup and fixes
@ 2019-07-29 12:11 Andrew Cooper
2019-07-29 12:11 ` [Xen-devel] [PATCH v3 01/10] page-alloc: Clamp get_free_buddy() to online nodes Andrew Cooper
` (9 more replies)
0 siblings, 10 replies; 33+ messages in thread
From: Andrew Cooper @ 2019-07-29 12:11 UTC (permalink / raw)
To: Xen-devel
Cc: Juergen Gross, Stefano Stabellini, Wei Liu, George Dunlap,
Andrew Cooper, Dario Faggioli, Julien Grall, Jan Beulich,
Volodymyr Babchuk, Roger Pau Monné
This series has expanded substantially from v2.
The bugfix has been moved to patch 1, in case it wants backporting.
Everything else is API cleanup following from Jan's request to do more than in
v2.
Andrew Cooper (10):
page-alloc: Clamp get_free_buddy() to online nodes
xen/bitmap: Drop {bitmap,cpumask,nodes}_shift_{left,right}()
xen/nodemask: Drop any_online_node() and first_unset_node()
xen/mask: Convert {cpu,node}mask_test() to be static inline
xen/cpumask: Introduce a CPUMASK_PR() wrapper for printing
xen/nodemask: Introduce a NODEMASK_PR() wrapper for printing
xen/nodemask: Drop nodes_{setall,clear}() and improve the initialisers
xen/nodemask: Introduce unlocked __nodemask_{set,clear}() helpers
xen/nodemask: Sanitise the remainder of the nodemask API
xen/nodemask: Drop remaining refeces to linux
xen/arch/x86/cpu/mcheck/mce.c | 2 +-
xen/arch/x86/crash.c | 2 +-
xen/arch/x86/dom0_build.c | 12 +-
xen/arch/x86/io_apic.c | 6 +-
xen/arch/x86/irq.c | 5 +-
xen/arch/x86/numa.c | 7 +-
xen/arch/x86/srat.c | 15 +-
xen/arch/x86/sysctl.c | 3 +-
xen/common/bitmap.c | 88 -----------
xen/common/cpupool.c | 7 +-
xen/common/domain.c | 10 +-
xen/common/domctl.c | 4 +-
xen/common/keyhandler.c | 10 +-
xen/common/page_alloc.c | 35 +++--
xen/common/sched_credit.c | 8 +-
xen/common/sched_credit2.c | 12 +-
xen/common/sched_null.c | 7 +-
xen/common/sched_rt.c | 3 +-
xen/common/sysctl.c | 2 +-
xen/include/xen/bitmap.h | 22 ---
xen/include/xen/cpumask.h | 41 ++---
xen/include/xen/nodemask.h | 346 +++++++++++++++++-------------------------
22 files changed, 228 insertions(+), 419 deletions(-)
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* [Xen-devel] [PATCH v3 01/10] page-alloc: Clamp get_free_buddy() to online nodes
2019-07-29 12:11 [Xen-devel] [PATCH v3 00/10] xen/nodemask: API cleanup and fixes Andrew Cooper
@ 2019-07-29 12:11 ` Andrew Cooper
2019-07-29 15:48 ` Jan Beulich
2019-07-29 12:11 ` [Xen-devel] [PATCH v3 02/10] xen/bitmap: Drop {bitmap, cpumask, nodes}_shift_{left, right}() Andrew Cooper
` (8 subsequent siblings)
9 siblings, 1 reply; 33+ messages in thread
From: Andrew Cooper @ 2019-07-29 12:11 UTC (permalink / raw)
To: Xen-devel
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
Julien Grall, Jan Beulich, Roger Pau Monné
d->node_affinity defaults to NODE_MASK_ALL which has bits set outside of
node_online_map. This in turn causes the loop in get_free_buddy() to waste
effort iterating over offline nodes.
Always clamp d->node_affinity to node_online_map when in use.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
v3:
* Rebase to before the nodemask API cleanup, to make it easier to backport.
v2:
* Rebase over the nodemask API change, and implement with a single
nodes_and()
---
xen/common/page_alloc.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 44a72d0b19..efa437c7df 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -811,11 +811,18 @@ static struct page_info *get_free_buddy(unsigned int zone_lo,
const struct domain *d)
{
nodeid_t first, node = MEMF_get_node(memflags), req_node = node;
- nodemask_t nodemask = d ? d->node_affinity : node_online_map;
+ nodemask_t nodemask = node_online_map;
unsigned int j, zone, nodemask_retry = 0;
struct page_info *pg;
bool use_unscrubbed = (memflags & MEMF_no_scrub);
+ /*
+ * d->node_affinity is our preferred allocation set if provided, but it
+ * may have bit set outside of node_online_map. Clamp it.
+ */
+ if ( d )
+ nodes_and(nodemask, nodemask, d->node_affinity);
+
if ( node == NUMA_NO_NODE )
{
if ( d != NULL )
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Xen-devel] [PATCH v3 02/10] xen/bitmap: Drop {bitmap, cpumask, nodes}_shift_{left, right}()
2019-07-29 12:11 [Xen-devel] [PATCH v3 00/10] xen/nodemask: API cleanup and fixes Andrew Cooper
2019-07-29 12:11 ` [Xen-devel] [PATCH v3 01/10] page-alloc: Clamp get_free_buddy() to online nodes Andrew Cooper
@ 2019-07-29 12:11 ` Andrew Cooper
2019-07-29 15:50 ` Jan Beulich
2019-07-29 12:11 ` [Xen-devel] [PATCH v3 03/10] xen/nodemask: Drop any_online_node() and first_unset_node() Andrew Cooper
` (7 subsequent siblings)
9 siblings, 1 reply; 33+ messages in thread
From: Andrew Cooper @ 2019-07-29 12:11 UTC (permalink / raw)
To: Xen-devel
Cc: Stefano Stabellini, Wei Liu, Andrew Cooper, Julien Grall,
Jan Beulich, Volodymyr Babchuk, Roger Pau Monné
These operations have never been used in Xen since their introduction, and it
doesn't seem likely that they will in the future.
Bloat-o-meter reports that they aren't the smalled when compiled, either.
add/remove: 0/2 grow/shrink: 0/0 up/down: 0/-814 (-814)
Function old new delta
__bitmap_shift_left 366 - -366
__bitmap_shift_right 448 - -448
Total: Before=3323730, After=3322916, chg -0.02%
Suggested-by: Jan Beulich <JBeulich@suse.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
v3:
* New
---
xen/common/bitmap.c | 88 ----------------------------------------------
xen/include/xen/bitmap.h | 22 ------------
xen/include/xen/cpumask.h | 15 --------
xen/include/xen/nodemask.h | 19 ----------
4 files changed, 144 deletions(-)
diff --git a/xen/common/bitmap.c b/xen/common/bitmap.c
index 34de387880..fd070bee97 100644
--- a/xen/common/bitmap.c
+++ b/xen/common/bitmap.c
@@ -109,94 +109,6 @@ void __bitmap_complement(unsigned long *dst, const unsigned long *src, int bits)
}
EXPORT_SYMBOL(__bitmap_complement);
-/*
- * __bitmap_shift_right - logical right shift of the bits in a bitmap
- * @dst - destination bitmap
- * @src - source bitmap
- * @nbits - shift by this many bits
- * @bits - bitmap size, in bits
- *
- * Shifting right (dividing) means moving bits in the MS -> LS bit
- * direction. Zeros are fed into the vacated MS positions and the
- * LS bits shifted off the bottom are lost.
- */
-void __bitmap_shift_right(unsigned long *dst,
- const unsigned long *src, int shift, int bits)
-{
- int k, lim = BITS_TO_LONGS(bits), left = bits % BITS_PER_LONG;
- int off = shift/BITS_PER_LONG, rem = shift % BITS_PER_LONG;
- unsigned long mask = (1UL << left) - 1;
- for (k = 0; off + k < lim; ++k) {
- unsigned long upper, lower;
-
- /*
- * If shift is not word aligned, take lower rem bits of
- * word above and make them the top rem bits of result.
- */
- if (!rem || off + k + 1 >= lim)
- upper = 0;
- else {
- upper = src[off + k + 1];
- if (off + k + 1 == lim - 1 && left)
- upper &= mask;
- }
- lower = src[off + k];
- if (left && off + k == lim - 1)
- lower &= mask;
- dst[k] = rem
- ? (upper << (BITS_PER_LONG - rem)) | (lower >> rem)
- : lower;
- if (left && k == lim - 1)
- dst[k] &= mask;
- }
- if (off)
- memset(&dst[lim - off], 0, off*sizeof(unsigned long));
-}
-EXPORT_SYMBOL(__bitmap_shift_right);
-
-
-/*
- * __bitmap_shift_left - logical left shift of the bits in a bitmap
- * @dst - destination bitmap
- * @src - source bitmap
- * @nbits - shift by this many bits
- * @bits - bitmap size, in bits
- *
- * Shifting left (multiplying) means moving bits in the LS -> MS
- * direction. Zeros are fed into the vacated LS bit positions
- * and those MS bits shifted off the top are lost.
- */
-
-void __bitmap_shift_left(unsigned long *dst,
- const unsigned long *src, int shift, int bits)
-{
- int k, lim = BITS_TO_LONGS(bits), left = bits % BITS_PER_LONG;
- int off = shift/BITS_PER_LONG, rem = shift % BITS_PER_LONG;
- for (k = lim - off - 1; k >= 0; --k) {
- unsigned long upper, lower;
-
- /*
- * If shift is not word aligned, take upper rem bits of
- * word below and make them the bottom rem bits of result.
- */
- if (rem && k > 0)
- lower = src[k - 1];
- else
- lower = 0;
- upper = src[k];
- if (left && k == lim - 1)
- upper &= (1UL << left) - 1;
- dst[k + off] = rem ? (lower >> (BITS_PER_LONG - rem))
- | (upper << rem)
- : upper;
- if (left && k + off == lim - 1)
- dst[k + off] &= (1UL << left) - 1;
- }
- if (off)
- memset(dst, 0, off*sizeof(unsigned long));
-}
-EXPORT_SYMBOL(__bitmap_shift_left);
-
void __bitmap_and(unsigned long *dst, const unsigned long *bitmap1,
const unsigned long *bitmap2, int bits)
{
diff --git a/xen/include/xen/bitmap.h b/xen/include/xen/bitmap.h
index 0430c1ce2a..4e1e690af1 100644
--- a/xen/include/xen/bitmap.h
+++ b/xen/include/xen/bitmap.h
@@ -38,8 +38,6 @@
* bitmap_empty(src, nbits) Are all bits zero in *src?
* bitmap_full(src, nbits) Are all bits set in *src?
* bitmap_weight(src, nbits) Hamming Weight: number set bits
- * bitmap_shift_right(dst, src, n, nbits) *dst = *src >> n
- * bitmap_shift_left(dst, src, n, nbits) *dst = *src << n
*/
/*
@@ -74,10 +72,6 @@ extern int __bitmap_equal(const unsigned long *bitmap1,
const unsigned long *bitmap2, int bits);
extern void __bitmap_complement(unsigned long *dst, const unsigned long *src,
int bits);
-extern void __bitmap_shift_right(unsigned long *dst,
- const unsigned long *src, int shift, int bits);
-extern void __bitmap_shift_left(unsigned long *dst,
- const unsigned long *src, int shift, int bits);
extern void __bitmap_and(unsigned long *dst, const unsigned long *bitmap1,
const unsigned long *bitmap2, int bits);
extern void __bitmap_or(unsigned long *dst, const unsigned long *bitmap1,
@@ -233,22 +227,6 @@ static inline int bitmap_weight(const unsigned long *src, int nbits)
return __bitmap_weight(src, nbits);
}
-static inline void bitmap_shift_right(unsigned long *dst,
- const unsigned long *src, int n, int nbits)
-{
- bitmap_switch(nbits,,
- *dst = *src >> n,
- __bitmap_shift_right(dst, src, n, nbits));
-}
-
-static inline void bitmap_shift_left(unsigned long *dst,
- const unsigned long *src, int n, int nbits)
-{
- bitmap_switch(nbits,,
- *dst = (*src << n) & BITMAP_LAST_WORD_MASK(nbits),
- __bitmap_shift_left(dst, src, n, nbits));
-}
-
#undef bitmap_switch
#undef bitmap_bytes
diff --git a/xen/include/xen/cpumask.h b/xen/include/xen/cpumask.h
index 1d73f9f3b6..6be9567e9e 100644
--- a/xen/include/xen/cpumask.h
+++ b/xen/include/xen/cpumask.h
@@ -31,9 +31,6 @@
* int cpumask_full(mask) Is mask full (all bits sets)?
* int cpumask_weight(mask) Hamming weigh - number of set bits
*
- * void cpumask_shift_right(dst, src, n) Shift right
- * void cpumask_shift_left(dst, src, n) Shift left
- *
* int cpumask_first(mask) Number lowest set bit, or NR_CPUS
* int cpumask_next(cpu, mask) Next cpu past 'cpu', or NR_CPUS
* int cpumask_last(mask) Number highest set bit, or NR_CPUS
@@ -213,18 +210,6 @@ static inline void cpumask_copy(cpumask_t *dstp, const cpumask_t *srcp)
bitmap_copy(dstp->bits, srcp->bits, nr_cpumask_bits);
}
-static inline void cpumask_shift_right(cpumask_t *dstp,
- const cpumask_t *srcp, int n)
-{
- bitmap_shift_right(dstp->bits, srcp->bits, n, nr_cpumask_bits);
-}
-
-static inline void cpumask_shift_left(cpumask_t *dstp,
- const cpumask_t *srcp, int n)
-{
- bitmap_shift_left(dstp->bits, srcp->bits, n, nr_cpumask_bits);
-}
-
static inline int cpumask_first(const cpumask_t *srcp)
{
return min_t(int, nr_cpu_ids, find_first_bit(srcp->bits, nr_cpu_ids));
diff --git a/xen/include/xen/nodemask.h b/xen/include/xen/nodemask.h
index e287399352..5eebc2c5ee 100644
--- a/xen/include/xen/nodemask.h
+++ b/xen/include/xen/nodemask.h
@@ -30,9 +30,6 @@
* int nodes_full(mask) Is mask full (all bits sets)?
* int nodes_weight(mask) Hamming weight - number of set bits
*
- * void nodes_shift_right(dst, src, n) Shift right
- * void nodes_shift_left(dst, src, n) Shift left
- *
* int first_node(mask) Number lowest set bit, or MAX_NUMNODES
* int next_node(node, mask) Next node past 'node', or MAX_NUMNODES
* int last_node(mask) Number highest set bit, or MAX_NUMNODES
@@ -189,22 +186,6 @@ static inline int __nodes_weight(const nodemask_t *srcp, int nbits)
return bitmap_weight(srcp->bits, nbits);
}
-#define nodes_shift_right(dst, src, n) \
- __nodes_shift_right(&(dst), &(src), (n), MAX_NUMNODES)
-static inline void __nodes_shift_right(nodemask_t *dstp,
- const nodemask_t *srcp, int n, int nbits)
-{
- bitmap_shift_right(dstp->bits, srcp->bits, n, nbits);
-}
-
-#define nodes_shift_left(dst, src, n) \
- __nodes_shift_left(&(dst), &(src), (n), MAX_NUMNODES)
-static inline void __nodes_shift_left(nodemask_t *dstp,
- const nodemask_t *srcp, int n, int nbits)
-{
- bitmap_shift_left(dstp->bits, srcp->bits, n, nbits);
-}
-
/* FIXME: better would be to fix all architectures to never return
> MAX_NUMNODES, then the silly min_ts could be dropped. */
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Xen-devel] [PATCH v3 03/10] xen/nodemask: Drop any_online_node() and first_unset_node()
2019-07-29 12:11 [Xen-devel] [PATCH v3 00/10] xen/nodemask: API cleanup and fixes Andrew Cooper
2019-07-29 12:11 ` [Xen-devel] [PATCH v3 01/10] page-alloc: Clamp get_free_buddy() to online nodes Andrew Cooper
2019-07-29 12:11 ` [Xen-devel] [PATCH v3 02/10] xen/bitmap: Drop {bitmap, cpumask, nodes}_shift_{left, right}() Andrew Cooper
@ 2019-07-29 12:11 ` Andrew Cooper
2019-07-29 15:51 ` Jan Beulich
2019-07-29 12:11 ` [Xen-devel] [PATCH v3 04/10] xen/mask: Convert {cpu, node}mask_test() to be static inline Andrew Cooper
` (6 subsequent siblings)
9 siblings, 1 reply; 33+ messages in thread
From: Andrew Cooper @ 2019-07-29 12:11 UTC (permalink / raw)
To: Xen-devel
Cc: Stefano Stabellini, Wei Liu, Andrew Cooper, Julien Grall,
Jan Beulich, Volodymyr Babchuk, Roger Pau Monné
These have never been used in Xen, and it is unlikely that they would be
useful in the future.
any_online_cpu() was dropped by c/s 22bdce1c048 "eliminate first_cpu() etc"
but the API comment was left in place. Drop that too.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
v3:
* New
---
xen/include/xen/cpumask.h | 2 --
xen/include/xen/nodemask.h | 20 --------------------
2 files changed, 22 deletions(-)
diff --git a/xen/include/xen/cpumask.h b/xen/include/xen/cpumask.h
index 6be9567e9e..9448f5c6f8 100644
--- a/xen/include/xen/cpumask.h
+++ b/xen/include/xen/cpumask.h
@@ -50,8 +50,6 @@
* int cpu_possible(cpu) Is some cpu possible?
* int cpu_present(cpu) Is some cpu present (can schedule)?
*
- * int any_online_cpu(mask) First online cpu in mask, or NR_CPUS
- *
* for_each_possible_cpu(cpu) for-loop cpu over cpu_possible_map
* for_each_online_cpu(cpu) for-loop cpu over cpu_online_map
* for_each_present_cpu(cpu) for-loop cpu over cpu_present_map
diff --git a/xen/include/xen/nodemask.h b/xen/include/xen/nodemask.h
index 5eebc2c5ee..c28dd3c768 100644
--- a/xen/include/xen/nodemask.h
+++ b/xen/include/xen/nodemask.h
@@ -33,8 +33,6 @@
* int first_node(mask) Number lowest set bit, or MAX_NUMNODES
* int next_node(node, mask) Next node past 'node', or MAX_NUMNODES
* int last_node(mask) Number highest set bit, or MAX_NUMNODES
- * int first_unset_node(mask) First node not set in mask, or
- * MAX_NUMNODES.
* int cycle_node(node, mask) Next node cycling from 'node', or
* MAX_NUMNODES
*
@@ -49,8 +47,6 @@
*
* int node_online(node) Is some node online?
*
- * int any_online_node(mask) First online node in mask
- *
* node_set_online(node) set bit 'node' in node_online_map
* node_set_offline(node) clear bit 'node' in node_online_map
*
@@ -224,13 +220,6 @@ static inline int __last_node(const nodemask_t *srcp, int nbits)
m; \
})
-#define first_unset_node(mask) __first_unset_node(&(mask))
-static inline int __first_unset_node(const nodemask_t *maskp)
-{
- return min_t(int,MAX_NUMNODES,
- find_first_zero_bit(maskp->bits, MAX_NUMNODES));
-}
-
#define cycle_node(n, src) __cycle_node((n), &(src), MAX_NUMNODES)
static inline int __cycle_node(int n, const nodemask_t *maskp, int nbits)
{
@@ -293,15 +282,6 @@ extern nodemask_t node_online_map;
#define node_online(node) ((node) == 0)
#endif
-#define any_online_node(mask) \
-({ \
- int node; \
- for_each_node_mask(node, (mask)) \
- if (node_online(node)) \
- break; \
- node; \
-})
-
#define node_set_online(node) set_bit((node), node_online_map.bits)
#define node_set_offline(node) clear_bit((node), node_online_map.bits)
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Xen-devel] [PATCH v3 04/10] xen/mask: Convert {cpu, node}mask_test() to be static inline
2019-07-29 12:11 [Xen-devel] [PATCH v3 00/10] xen/nodemask: API cleanup and fixes Andrew Cooper
` (2 preceding siblings ...)
2019-07-29 12:11 ` [Xen-devel] [PATCH v3 03/10] xen/nodemask: Drop any_online_node() and first_unset_node() Andrew Cooper
@ 2019-07-29 12:11 ` Andrew Cooper
2019-07-30 8:52 ` Jan Beulich
2019-07-29 12:11 ` [Xen-devel] [PATCH v3 05/10] xen/cpumask: Introduce a CPUMASK_PR() wrapper for printing Andrew Cooper
` (5 subsequent siblings)
9 siblings, 1 reply; 33+ messages in thread
From: Andrew Cooper @ 2019-07-29 12:11 UTC (permalink / raw)
To: Xen-devel
Cc: Stefano Stabellini, Wei Liu, Andrew Cooper, Julien Grall,
Jan Beulich, Volodymyr Babchuk, Roger Pau Monné
The buggy version of GCC isn't supported by Xen, so reimplement the helpers
with type checking, using Xen's latest type expectations.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
v3:
* New
---
xen/arch/x86/srat.c | 2 +-
xen/common/page_alloc.c | 2 +-
xen/include/xen/cpumask.h | 18 +++++-------------
xen/include/xen/nodemask.h | 18 +++++++-----------
4 files changed, 14 insertions(+), 26 deletions(-)
diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c
index 47a4267220..506a56d66b 100644
--- a/xen/arch/x86/srat.c
+++ b/xen/arch/x86/srat.c
@@ -495,7 +495,7 @@ int __init acpi_scan_nodes(u64 start, u64 end)
for (i = 0; i < nr_cpu_ids; i++) {
if (cpu_to_node[i] == NUMA_NO_NODE)
continue;
- if (!node_isset(cpu_to_node[i], processor_nodes_parsed))
+ if (!nodemask_test(cpu_to_node[i], &processor_nodes_parsed))
numa_set_node(i, NUMA_NO_NODE);
}
numa_init_array();
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index efa437c7df..77e649d065 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -878,7 +878,7 @@ static struct page_info *get_free_buddy(unsigned int zone_lo,
return NULL;
/* Pick next node. */
- if ( !node_isset(node, nodemask) )
+ if ( !nodemask_test(node, &nodemask) )
{
/* Very first node may be caller-specified and outside nodemask. */
ASSERT(!nodemask_retry);
diff --git a/xen/include/xen/cpumask.h b/xen/include/xen/cpumask.h
index 9448f5c6f8..478ec66e5b 100644
--- a/xen/include/xen/cpumask.h
+++ b/xen/include/xen/cpumask.h
@@ -14,7 +14,7 @@
* void cpumask_clear_cpu(cpu, mask) turn off bit 'cpu' in mask
* void cpumask_setall(mask) set all bits
* void cpumask_clear(mask) clear all bits
- * int cpumask_test_cpu(cpu, mask) true iff bit 'cpu' set in mask
+ * bool cpumask_test_cpu(cpu, mask) true iff bit 'cpu' set in mask
* int cpumask_test_and_set_cpu(cpu, mask) test and set bit 'cpu' in mask
* int cpumask_test_and_clear_cpu(cpu, mask) test and clear bit 'cpu' in mask
*
@@ -53,15 +53,6 @@
* for_each_possible_cpu(cpu) for-loop cpu over cpu_possible_map
* for_each_online_cpu(cpu) for-loop cpu over cpu_online_map
* for_each_present_cpu(cpu) for-loop cpu over cpu_present_map
- *
- * Subtlety:
- * 1) The 'type-checked' form of cpumask_test_cpu() causes gcc (3.3.2, anyway)
- * to generate slightly worse code. Note for example the additional
- * 40 lines of assembly code compiling the "for each possible cpu"
- * loops buried in the disk_stat_read() macros calls when compiling
- * drivers/block/genhd.c (arch i386, CONFIG_SMP=y). So use a simple
- * one-line #define for cpumask_test_cpu(), instead of wrapping an inline
- * inside a macro, the way we do the other calls.
*/
#include <xen/bitmap.h>
@@ -117,9 +108,10 @@ static inline void cpumask_clear(cpumask_t *dstp)
bitmap_zero(dstp->bits, nr_cpumask_bits);
}
-/* No static inline type checking - see Subtlety (1) above. */
-#define cpumask_test_cpu(cpu, cpumask) \
- test_bit(cpumask_check(cpu), (cpumask)->bits)
+static inline bool cpumask_test_cpu(unsigned int cpu, const cpumask_t *dst)
+{
+ return test_bit(cpumask_check(cpu), dst->bits);
+}
static inline int cpumask_test_and_set_cpu(int cpu, volatile cpumask_t *addr)
{
diff --git a/xen/include/xen/nodemask.h b/xen/include/xen/nodemask.h
index c28dd3c768..7ab8b794c6 100644
--- a/xen/include/xen/nodemask.h
+++ b/xen/include/xen/nodemask.h
@@ -14,7 +14,7 @@
* void node_clear(node, mask) turn off bit 'node' in mask
* void nodes_setall(mask) set all bits
* void nodes_clear(mask) clear all bits
- * int node_isset(node, mask) true iff bit 'node' set in mask
+ * bool nodemask_test(node, mask) true iff bit 'node' set in mask
* int node_test_and_set(node, mask) test and set bit 'node' in mask
*
* void nodes_and(dst, src1, src2) dst = src1 & src2 [intersection]
@@ -45,18 +45,12 @@
*
* int num_online_nodes() Number of online Nodes
*
- * int node_online(node) Is some node online?
+ * bool node_online(node) Is this node online?
*
* node_set_online(node) set bit 'node' in node_online_map
* node_set_offline(node) clear bit 'node' in node_online_map
*
* for_each_online_node(node) for-loop node over node_online_map
- *
- * Subtlety:
- * 1) The 'type-checked' form of node_isset() causes gcc (3.3.2, anyway)
- * to generate slightly worse code. So use a simple one-line #define
- * for node_isset(), instead of wrapping an inline inside a macro, the
- * way we do the other calls.
*/
#include <xen/kernel.h>
@@ -90,8 +84,10 @@ static inline void __nodes_clear(nodemask_t *dstp, int nbits)
bitmap_zero(dstp->bits, nbits);
}
-/* No static inline type checking - see Subtlety (1) above. */
-#define node_isset(node, nodemask) test_bit((node), (nodemask).bits)
+static inline bool nodemask_test(unsigned int node, const nodemask_t *dst)
+{
+ return test_bit(node, dst->bits);
+}
#define node_test_and_set(node, nodemask) \
__node_test_and_set((node), &(nodemask))
@@ -276,7 +272,7 @@ extern nodemask_t node_online_map;
#if MAX_NUMNODES > 1
#define num_online_nodes() nodes_weight(node_online_map)
-#define node_online(node) node_isset((node), node_online_map)
+#define node_online(node) nodemask_test(node, &node_online_map)
#else
#define num_online_nodes() 1
#define node_online(node) ((node) == 0)
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Xen-devel] [PATCH v3 05/10] xen/cpumask: Introduce a CPUMASK_PR() wrapper for printing
2019-07-29 12:11 [Xen-devel] [PATCH v3 00/10] xen/nodemask: API cleanup and fixes Andrew Cooper
` (3 preceding siblings ...)
2019-07-29 12:11 ` [Xen-devel] [PATCH v3 04/10] xen/mask: Convert {cpu, node}mask_test() to be static inline Andrew Cooper
@ 2019-07-29 12:11 ` Andrew Cooper
2019-07-30 8:55 ` Jan Beulich
2019-07-29 12:12 ` [Xen-devel] [PATCH v3 06/10] xen/nodemask: Introduce a NODEMASK_PR() " Andrew Cooper
` (4 subsequent siblings)
9 siblings, 1 reply; 33+ messages in thread
From: Andrew Cooper @ 2019-07-29 12:11 UTC (permalink / raw)
To: Xen-devel
Cc: Juergen Gross, Stefano Stabellini, Wei Liu, George Dunlap,
Andrew Cooper, Dario Faggioli, Julien Grall, Jan Beulich,
Volodymyr Babchuk, Roger Pau Monné
Having to specify 'nr_cpu_id, cpumask_bits(foo)' for all printing operations
is quite repetative. Introduce a wrapper to help.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Dario Faggioli <dfaggioli@suse.com>
CC: Juergen Gross <jgross@suse.com>
v3:
* New
Juergen: If this is too disruptive to your core schedulling series, I can
defer it. Its just API cleanup patching the subsequent patch.
---
xen/arch/x86/cpu/mcheck/mce.c | 2 +-
xen/arch/x86/crash.c | 2 +-
xen/arch/x86/io_apic.c | 6 ++----
xen/arch/x86/irq.c | 5 ++---
xen/arch/x86/sysctl.c | 3 +--
xen/common/cpupool.c | 7 +++----
xen/common/keyhandler.c | 8 ++++----
xen/common/sched_credit.c | 6 +++---
xen/common/sched_credit2.c | 12 ++++++------
xen/common/sched_null.c | 7 +++----
xen/common/sched_rt.c | 3 +--
xen/include/xen/cpumask.h | 6 ++++++
12 files changed, 33 insertions(+), 34 deletions(-)
diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c
index 2a9747ed19..28ad7dd659 100644
--- a/xen/arch/x86/cpu/mcheck/mce.c
+++ b/xen/arch/x86/cpu/mcheck/mce.c
@@ -547,7 +547,7 @@ void mcheck_cmn_handler(const struct cpu_user_regs *regs)
snprintf(ebuf, sizeof(ebuf),
"MCE: Fatal error happened on CPUs %*pb",
- nr_cpu_ids, cpumask_bits(&mce_fatal_cpus));
+ CPUMASK_PR(&mce_fatal_cpus));
mc_panic(ebuf);
}
diff --git a/xen/arch/x86/crash.c b/xen/arch/x86/crash.c
index a9f3e1890c..32132e4cb9 100644
--- a/xen/arch/x86/crash.c
+++ b/xen/arch/x86/crash.c
@@ -160,7 +160,7 @@ static void nmi_shootdown_cpus(void)
printk("Shot down all CPUs\n");
else
printk("Failed to shoot down CPUs {%*pbl}\n",
- nr_cpu_ids, cpumask_bits(&waiting_to_crash));
+ CPUMASK_PR(&waiting_to_crash));
/*
* Try to crash shutdown IOMMU functionality as some old crashdump
diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index f93f711051..5d25862bd8 100644
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -2238,8 +2238,7 @@ int io_apic_set_pci_routing (int ioapic, int pin, int irq, int edge_level, int a
SET_DEST(entry, logical, cpu_mask_to_apicid(mask));
} else {
printk(XENLOG_ERR "IRQ%d: no target CPU (%*pb vs %*pb)\n",
- irq, nr_cpu_ids, cpumask_bits(desc->arch.cpu_mask),
- nr_cpu_ids, cpumask_bits(TARGET_CPUS));
+ irq, CPUMASK_PR(desc->arch.cpu_mask), CPUMASK_PR(TARGET_CPUS));
desc->status |= IRQ_DISABLED;
}
@@ -2437,8 +2436,7 @@ int ioapic_guest_write(unsigned long physbase, unsigned int reg, u32 val)
else
{
gprintk(XENLOG_ERR, "IRQ%d: no target CPU (%*pb vs %*pb)\n",
- irq, nr_cpu_ids, cpumask_bits(desc->arch.cpu_mask),
- nr_cpu_ids, cpumask_bits(TARGET_CPUS));
+ irq, CPUMASK_PR(desc->arch.cpu_mask), CPUMASK_PR(TARGET_CPUS));
desc->status |= IRQ_DISABLED;
rte.mask = 1;
}
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index 668a1f5b36..0ee33464d2 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -2398,8 +2398,7 @@ static void dump_irqs(unsigned char key)
spin_lock_irqsave(&desc->lock, flags);
printk(" IRQ:%4d aff:{%*pbl}/{%*pbl} vec:%02x %-15s status=%03x ",
- irq, nr_cpu_ids, cpumask_bits(desc->affinity),
- nr_cpu_ids, cpumask_bits(desc->arch.cpu_mask),
+ irq, CPUMASK_PR(desc->affinity), CPUMASK_PR(desc->arch.cpu_mask),
desc->arch.vector, desc->handler->typename, desc->status);
if ( ssid )
@@ -2563,7 +2562,7 @@ void fixup_irqs(const cpumask_t *mask, bool verbose)
printk("Cannot set affinity for IRQ%u\n", irq);
else if ( break_affinity )
printk("Broke affinity for IRQ%u, new: %*pb\n",
- irq, nr_cpu_ids, cpumask_bits(affinity));
+ irq, CPUMASK_PR(affinity));
}
/* That doesn't seem sufficient. Give it 1ms. */
diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
index 3f06fecbd8..c50d910a1c 100644
--- a/xen/arch/x86/sysctl.c
+++ b/xen/arch/x86/sysctl.c
@@ -150,8 +150,7 @@ static long smt_up_down_helper(void *data)
if ( !ret )
printk(XENLOG_INFO "SMT %s - online CPUs 0x%*pb\n",
- up ? "enabled" : "disabled",
- nr_cpu_ids, cpumask_bits(&cpu_online_map));
+ up ? "enabled" : "disabled", CPUMASK_PR(&cpu_online_map));
return ret;
}
diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
index 31ac323e40..f90e496eda 100644
--- a/xen/common/cpupool.c
+++ b/xen/common/cpupool.c
@@ -712,18 +712,17 @@ void dump_runq(unsigned char key)
sched_smt_power_savings? "enabled":"disabled");
printk("NOW=%"PRI_stime"\n", now);
- printk("Online Cpus: %*pbl\n", nr_cpu_ids, cpumask_bits(&cpu_online_map));
+ printk("Online Cpus: %*pbl\n", CPUMASK_PR(&cpu_online_map));
if ( !cpumask_empty(&cpupool_free_cpus) )
{
- printk("Free Cpus: %*pbl\n",
- nr_cpu_ids, cpumask_bits(&cpupool_free_cpus));
+ printk("Free Cpus: %*pbl\n", CPUMASK_PR(&cpupool_free_cpus));
schedule_dump(NULL);
}
for_each_cpupool(c)
{
printk("Cpupool %d:\n", (*c)->cpupool_id);
- printk("Cpus: %*pbl\n", nr_cpu_ids, cpumask_bits((*c)->cpu_valid));
+ printk("Cpus: %*pbl\n", CPUMASK_PR((*c)->cpu_valid));
schedule_dump(*c);
}
diff --git a/xen/common/keyhandler.c b/xen/common/keyhandler.c
index 4f4a660b0c..a5e95e2fe9 100644
--- a/xen/common/keyhandler.c
+++ b/xen/common/keyhandler.c
@@ -272,8 +272,8 @@ static void dump_domains(unsigned char key)
printk(" nr_pages=%d xenheap_pages=%d shared_pages=%u paged_pages=%u "
"dirty_cpus={%*pbl} max_pages=%u\n",
d->tot_pages, d->xenheap_pages, atomic_read(&d->shr_pages),
- atomic_read(&d->paged_pages), nr_cpu_ids,
- cpumask_bits(d->dirty_cpumask), d->max_pages);
+ atomic_read(&d->paged_pages), CPUMASK_PR(d->dirty_cpumask),
+ d->max_pages);
printk(" handle=%02x%02x%02x%02x-%02x%02x-%02x%02x-"
"%02x%02x-%02x%02x%02x%02x%02x%02x vm_assist=%08lx\n",
d->handle[ 0], d->handle[ 1], d->handle[ 2], d->handle[ 3],
@@ -312,8 +312,8 @@ static void dump_domains(unsigned char key)
printk("dirty_cpu=%u", v->dirty_cpu);
printk("\n");
printk(" cpu_hard_affinity={%*pbl} cpu_soft_affinity={%*pbl}\n",
- nr_cpu_ids, cpumask_bits(v->cpu_hard_affinity),
- nr_cpu_ids, cpumask_bits(v->cpu_soft_affinity));
+ CPUMASK_PR(v->cpu_hard_affinity),
+ CPUMASK_PR(v->cpu_soft_affinity));
printk(" pause_count=%d pause_flags=%lx\n",
atomic_read(&v->pause_count), v->pause_flags);
arch_dump_vcpu_info(v);
diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
index 3c0d7c7267..81dee5e472 100644
--- a/xen/common/sched_credit.c
+++ b/xen/common/sched_credit.c
@@ -2057,8 +2057,8 @@ csched_dump_pcpu(const struct scheduler *ops, int cpu)
printk("CPU[%02d] nr_run=%d, sort=%d, sibling=%*pb, core=%*pb\n",
cpu, spc->nr_runnable, spc->runq_sort_last,
- nr_cpu_ids, cpumask_bits(per_cpu(cpu_sibling_mask, cpu)),
- nr_cpu_ids, cpumask_bits(per_cpu(cpu_core_mask, cpu)));
+ CPUMASK_PR(per_cpu(cpu_sibling_mask, cpu)),
+ CPUMASK_PR(per_cpu(cpu_core_mask, cpu)));
/* current VCPU (nothing to say if that's the idle vcpu). */
svc = CSCHED_VCPU(curr_on_cpu(cpu));
@@ -2119,7 +2119,7 @@ csched_dump(const struct scheduler *ops)
prv->ticks_per_tslice,
prv->vcpu_migr_delay/ MICROSECS(1));
- printk("idlers: %*pb\n", nr_cpu_ids, cpumask_bits(prv->idlers));
+ printk("idlers: %*pb\n", CPUMASK_PR(prv->idlers));
printk("active vcpus:\n");
loop = 0;
diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index 8e4381d8a7..33fc86ffb2 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -3653,8 +3653,8 @@ dump_pcpu(const struct scheduler *ops, int cpu)
printk("CPU[%02d] runq=%d, sibling=%*pb, core=%*pb\n",
cpu, c2r(cpu),
- nr_cpu_ids, cpumask_bits(per_cpu(cpu_sibling_mask, cpu)),
- nr_cpu_ids, cpumask_bits(per_cpu(cpu_core_mask, cpu)));
+ CPUMASK_PR(per_cpu(cpu_sibling_mask, cpu)),
+ CPUMASK_PR(per_cpu(cpu_core_mask, cpu)));
/* current VCPU (nothing to say if that's the idle vcpu) */
svc = csched2_vcpu(curr_on_cpu(cpu));
@@ -3698,7 +3698,7 @@ csched2_dump(const struct scheduler *ops)
"\taveload = %"PRI_stime" (~%"PRI_stime"%%)\n",
i,
cpumask_weight(&prv->rqd[i].active),
- nr_cpu_ids, cpumask_bits(&prv->rqd[i].active),
+ CPUMASK_PR(&prv->rqd[i].active),
prv->rqd[i].max_weight,
prv->rqd[i].pick_bias,
prv->rqd[i].load,
@@ -3708,9 +3708,9 @@ csched2_dump(const struct scheduler *ops)
printk("\tidlers: %*pb\n"
"\ttickled: %*pb\n"
"\tfully idle cores: %*pb\n",
- nr_cpu_ids, cpumask_bits(&prv->rqd[i].idle),
- nr_cpu_ids, cpumask_bits(&prv->rqd[i].tickled),
- nr_cpu_ids, cpumask_bits(&prv->rqd[i].smt_idle));
+ CPUMASK_PR(&prv->rqd[i].idle),
+ CPUMASK_PR(&prv->rqd[i].tickled),
+ CPUMASK_PR(&prv->rqd[i].smt_idle));
}
printk("Domain info:\n");
diff --git a/xen/common/sched_null.c b/xen/common/sched_null.c
index c47c1b5aae..5aec9f17bd 100644
--- a/xen/common/sched_null.c
+++ b/xen/common/sched_null.c
@@ -782,9 +782,8 @@ static void null_dump_pcpu(const struct scheduler *ops, int cpu)
lock = pcpu_schedule_lock_irqsave(cpu, &flags);
printk("CPU[%02d] sibling=%*pb, core=%*pb",
- cpu,
- nr_cpu_ids, cpumask_bits(per_cpu(cpu_sibling_mask, cpu)),
- nr_cpu_ids, cpumask_bits(per_cpu(cpu_core_mask, cpu)));
+ cpu, CPUMASK_PR(per_cpu(cpu_sibling_mask, cpu)),
+ CPUMASK_PR(per_cpu(cpu_core_mask, cpu)));
if ( per_cpu(npc, cpu).vcpu != NULL )
printk(", vcpu=%pv", per_cpu(npc, cpu).vcpu);
printk("\n");
@@ -810,7 +809,7 @@ static void null_dump(const struct scheduler *ops)
spin_lock_irqsave(&prv->lock, flags);
- printk("\tcpus_free = %*pbl\n", nr_cpu_ids, cpumask_bits(&prv->cpus_free));
+ printk("\tcpus_free = %*pbl\n", CPUMASK_PR(&prv->cpus_free));
printk("Domain info:\n");
loop = 0;
diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
index 0acfc3d702..e0e350bdf3 100644
--- a/xen/common/sched_rt.c
+++ b/xen/common/sched_rt.c
@@ -344,8 +344,7 @@ rt_dump_vcpu(const struct scheduler *ops, const struct rt_vcpu *svc)
has_extratime(svc),
vcpu_on_q(svc),
vcpu_runnable(svc->vcpu),
- svc->flags,
- nr_cpu_ids, cpumask_bits(mask));
+ svc->flags, CPUMASK_PR(mask));
}
static void
diff --git a/xen/include/xen/cpumask.h b/xen/include/xen/cpumask.h
index 478ec66e5b..ca712efe57 100644
--- a/xen/include/xen/cpumask.h
+++ b/xen/include/xen/cpumask.h
@@ -61,6 +61,12 @@
typedef struct cpumask{ DECLARE_BITMAP(bits, NR_CPUS); } cpumask_t;
+/*
+ * printf arguments for a cpumask. Shorthand for using '%*pb[l]' when
+ * printing a cpumask.
+ */
+#define CPUMASK_PR(src) nr_cpu_ids, cpumask_bits(src)
+
extern unsigned int nr_cpu_ids;
#if NR_CPUS > 4 * BITS_PER_LONG
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Xen-devel] [PATCH v3 06/10] xen/nodemask: Introduce a NODEMASK_PR() wrapper for printing
2019-07-29 12:11 [Xen-devel] [PATCH v3 00/10] xen/nodemask: API cleanup and fixes Andrew Cooper
` (4 preceding siblings ...)
2019-07-29 12:11 ` [Xen-devel] [PATCH v3 05/10] xen/cpumask: Introduce a CPUMASK_PR() wrapper for printing Andrew Cooper
@ 2019-07-29 12:12 ` Andrew Cooper
2019-07-30 8:58 ` Jan Beulich
2019-07-29 12:12 ` [Xen-devel] [PATCH v3 07/10] xen/nodemask: Drop nodes_{setall, clear}() and improve the initialisers Andrew Cooper
` (3 subsequent siblings)
9 siblings, 1 reply; 33+ messages in thread
From: Andrew Cooper @ 2019-07-29 12:12 UTC (permalink / raw)
To: Xen-devel
Cc: Stefano Stabellini, Wei Liu, Andrew Cooper, Julien Grall,
Jan Beulich, Volodymyr Babchuk, Roger Pau Monné
Rework nodes_addr() into nodemask_bits() and change the indirection to match
its cpumask_bits() counterpart, and update the caller.
Use NODEMASK_PR() to fix up one opencoded access into nodemask.bits in
dump_domains().
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
v3:
* New
---
xen/common/domctl.c | 4 ++--
xen/common/keyhandler.c | 2 +-
xen/include/xen/nodemask.h | 13 ++++++++++---
3 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index fa260ce5fb..9ed9f57f0d 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -123,14 +123,14 @@ int xenctl_bitmap_to_cpumask(cpumask_var_t *cpumask,
static int nodemask_to_xenctl_bitmap(struct xenctl_bitmap *xenctl_nodemap,
const nodemask_t *nodemask)
{
- return bitmap_to_xenctl_bitmap(xenctl_nodemap, nodes_addr(*nodemask),
+ return bitmap_to_xenctl_bitmap(xenctl_nodemap, nodemask_bits(nodemask),
MAX_NUMNODES);
}
static int xenctl_bitmap_to_nodemask(nodemask_t *nodemask,
const struct xenctl_bitmap *xenctl_nodemap)
{
- return xenctl_bitmap_to_bitmap(nodes_addr(*nodemask), xenctl_nodemap,
+ return xenctl_bitmap_to_bitmap(nodemask_bits(nodemask), xenctl_nodemap,
MAX_NUMNODES);
}
diff --git a/xen/common/keyhandler.c b/xen/common/keyhandler.c
index a5e95e2fe9..57b360ee4b 100644
--- a/xen/common/keyhandler.c
+++ b/xen/common/keyhandler.c
@@ -293,7 +293,7 @@ static void dump_domains(unsigned char key)
dump_pageframe_info(d);
printk("NODE affinity for domain %d: [%*pbl]\n",
- d->domain_id, MAX_NUMNODES, d->node_affinity.bits);
+ d->domain_id, NODEMASK_PR(&d->node_affinity));
printk("VCPU information and callbacks for domain %u:\n",
d->domain_id);
diff --git a/xen/include/xen/nodemask.h b/xen/include/xen/nodemask.h
index 7ab8b794c6..1dd6c7458e 100644
--- a/xen/include/xen/nodemask.h
+++ b/xen/include/xen/nodemask.h
@@ -39,7 +39,7 @@
* nodemask_t nodemask_of_node(node) Return nodemask with bit 'node' set
* NODE_MASK_ALL Initializer - all bits set
* NODE_MASK_NONE Initializer - no bits set
- * unsigned long *nodes_addr(mask) Array of unsigned long's in mask
+ * unsigned long *nodemask_bits(mask) Array of unsigned long's in mask
*
* for_each_node_mask(node, mask) for-loop node over mask
*
@@ -58,6 +58,15 @@
#include <xen/numa.h>
typedef struct { DECLARE_BITMAP(bits, MAX_NUMNODES); } nodemask_t;
+
+/*
+ * printf arguments for a nodemask. Shorthand for using '%*pb[l]' when
+ * printing a nodemask.
+ */
+#define NODEMASK_PR(src) MAX_NUMNODES, nodemask_bits(src)
+
+#define nodemask_bits(src) ((src)->bits)
+
extern nodemask_t _unused_nodemask_arg_;
#define node_set(node, dst) __node_set((node), &(dst))
@@ -250,8 +259,6 @@ static inline int __cycle_node(int n, const nodemask_t *maskp, int nbits)
[0 ... BITS_TO_LONGS(MAX_NUMNODES)-1] = 0UL \
} })
-#define nodes_addr(src) ((src).bits)
-
#if MAX_NUMNODES > 1
#define for_each_node_mask(node, mask) \
for ((node) = first_node(mask); \
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Xen-devel] [PATCH v3 07/10] xen/nodemask: Drop nodes_{setall, clear}() and improve the initialisers
2019-07-29 12:11 [Xen-devel] [PATCH v3 00/10] xen/nodemask: API cleanup and fixes Andrew Cooper
` (5 preceding siblings ...)
2019-07-29 12:12 ` [Xen-devel] [PATCH v3 06/10] xen/nodemask: Introduce a NODEMASK_PR() " Andrew Cooper
@ 2019-07-29 12:12 ` Andrew Cooper
2019-07-30 9:44 ` Jan Beulich
2019-07-29 12:12 ` [Xen-devel] [PATCH v3 08/10] xen/nodemask: Introduce unlocked __nodemask_{set, clear}() helpers Andrew Cooper
` (2 subsequent siblings)
9 siblings, 1 reply; 33+ messages in thread
From: Andrew Cooper @ 2019-07-29 12:12 UTC (permalink / raw)
To: Xen-devel
Cc: Stefano Stabellini, Wei Liu, Andrew Cooper, Julien Grall,
Jan Beulich, Volodymyr Babchuk, Roger Pau Monné
There is no need to use runtime variable-length clearing when MAX_NUMNODES is
known to the compiler. Drop these functions and use the initialisers instead.
numa_initmem_init() opencodes nodemask_of_node(), but its implemention is
still poor, so replace it with NODEMASK_OF() which is calculated at compile
time, and without the use of a locked set_bit() operation.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
v3:
* New
---
xen/arch/x86/dom0_build.c | 2 +-
xen/arch/x86/numa.c | 3 +-
xen/common/domain.c | 4 +--
xen/include/xen/nodemask.h | 85 +++++++++++++++++-----------------------------
4 files changed, 35 insertions(+), 59 deletions(-)
diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
index c69570920c..06500c87c6 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -231,7 +231,7 @@ unsigned int __init dom0_max_vcpus(void)
if ( pv_shim )
{
- nodes_setall(dom0_nodes);
+ dom0_nodes = NODEMASK_ALL;
/*
* When booting in shim mode APs are not started until the guest brings
diff --git a/xen/arch/x86/numa.c b/xen/arch/x86/numa.c
index 7e1f563012..7473f83b7b 100644
--- a/xen/arch/x86/numa.c
+++ b/xen/arch/x86/numa.c
@@ -269,8 +269,7 @@ void __init numa_initmem_init(unsigned long start_pfn, unsigned long end_pfn)
/* setup dummy node covering all memory */
memnode_shift = BITS_PER_LONG - 1;
memnodemap = _memnodemap;
- nodes_clear(node_online_map);
- node_set_online(0);
+ node_online_map = NODEMASK_OF(0);
for ( i = 0; i < nr_cpu_ids; i++ )
numa_set_node(i, 0);
cpumask_copy(&node_to_cpumask[0], cpumask_of(0));
diff --git a/xen/common/domain.c b/xen/common/domain.c
index e8e850796e..11565a64b3 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -384,7 +384,7 @@ struct domain *domain_create(domid_t domid,
INIT_PAGE_LIST_HEAD(&d->xenpage_list);
spin_lock_init(&d->node_affinity_lock);
- d->node_affinity = NODE_MASK_ALL;
+ d->node_affinity = NODEMASK_ALL;
d->auto_node_affinity = 1;
spin_lock_init(&d->shutdown_lock);
@@ -615,7 +615,7 @@ void domain_update_node_affinity(struct domain *d)
dom_affinity = cpumask_empty(dom_cpumask_soft) ?
dom_cpumask : dom_cpumask_soft;
- nodes_clear(d->node_affinity);
+ d->node_affinity = NODEMASK_NONE;
for_each_cpu ( cpu, dom_affinity )
node_set(cpu_to_node(cpu), d->node_affinity);
}
diff --git a/xen/include/xen/nodemask.h b/xen/include/xen/nodemask.h
index 1dd6c7458e..9933fec5c4 100644
--- a/xen/include/xen/nodemask.h
+++ b/xen/include/xen/nodemask.h
@@ -12,8 +12,6 @@
*
* void node_set(node, mask) turn on bit 'node' in mask
* void node_clear(node, mask) turn off bit 'node' in mask
- * void nodes_setall(mask) set all bits
- * void nodes_clear(mask) clear all bits
* bool nodemask_test(node, mask) true iff bit 'node' set in mask
* int node_test_and_set(node, mask) test and set bit 'node' in mask
*
@@ -36,9 +34,9 @@
* int cycle_node(node, mask) Next node cycling from 'node', or
* MAX_NUMNODES
*
- * nodemask_t nodemask_of_node(node) Return nodemask with bit 'node' set
- * NODE_MASK_ALL Initializer - all bits set
- * NODE_MASK_NONE Initializer - no bits set
+ * nodemask_t NODEMASK_OF(node) Initializer - bit 'node' set
+ * NODEMASK_ALL Initializer - all bits set
+ * NODEMASK_NONE Initializer - no bits set
* unsigned long *nodemask_bits(mask) Array of unsigned long's in mask
*
* for_each_node_mask(node, mask) for-loop node over mask
@@ -67,7 +65,34 @@ typedef struct { DECLARE_BITMAP(bits, MAX_NUMNODES); } nodemask_t;
#define nodemask_bits(src) ((src)->bits)
-extern nodemask_t _unused_nodemask_arg_;
+#define NODEMASK_LAST_WORD BITMAP_LAST_WORD_MASK(MAX_NUMNODES)
+
+#define NODEMASK_NONE \
+((nodemask_t) {{ \
+ [0 ... BITS_TO_LONGS(MAX_NUMNODES) - 1] = 0 \
+}})
+
+#if MAX_NUMNODES <= BITS_PER_LONG
+
+#define NODEMASK_ALL ((nodemask_t) {{ NODEMASK_LAST_WORD }})
+#define NODEMASK_OF(node) ((nodemask_t) {{ 1UL << (node) }})
+
+#else /* MAX_NUMNODES > BITS_PER_LONG */
+
+#define NODEMASK_ALL \
+((nodemask_t) {{ \
+ [0 ... BITS_TO_LONGS(MAX_NUMNODES) - 2] = ~0UL, \
+ [BITS_TO_LONGS(MAX_NUMNODES) - 1] = NODEMASK_LAST_WORD \
+}})
+
+#define NODEMASK_OF(node) \
+({ \
+ nodemask_t m = NODES_NONE; \
+ m.bits[(node) / BITS_PER_LONG] = 1UL << ((node) % BITS_PER_LONG); \
+ m; \
+})
+
+#endif /* MAX_NUMNODES */
#define node_set(node, dst) __node_set((node), &(dst))
static inline void __node_set(int node, volatile nodemask_t *dstp)
@@ -81,18 +106,6 @@ static inline void __node_clear(int node, volatile nodemask_t *dstp)
clear_bit(node, dstp->bits);
}
-#define nodes_setall(dst) __nodes_setall(&(dst), MAX_NUMNODES)
-static inline void __nodes_setall(nodemask_t *dstp, int nbits)
-{
- bitmap_fill(dstp->bits, nbits);
-}
-
-#define nodes_clear(dst) __nodes_clear(&(dst), MAX_NUMNODES)
-static inline void __nodes_clear(nodemask_t *dstp, int nbits)
-{
- bitmap_zero(dstp->bits, nbits);
-}
-
static inline bool nodemask_test(unsigned int node, const nodemask_t *dst)
{
return test_bit(node, dst->bits);
@@ -213,18 +226,6 @@ static inline int __last_node(const nodemask_t *srcp, int nbits)
return pnode;
}
-#define nodemask_of_node(node) \
-({ \
- typeof(_unused_nodemask_arg_) m; \
- if (sizeof(m) == sizeof(unsigned long)) { \
- m.bits[0] = 1UL<<(node); \
- } else { \
- nodes_clear(m); \
- node_set((node), m); \
- } \
- m; \
-})
-
#define cycle_node(n, src) __cycle_node((n), &(src), MAX_NUMNODES)
static inline int __cycle_node(int n, const nodemask_t *maskp, int nbits)
{
@@ -235,30 +236,6 @@ static inline int __cycle_node(int n, const nodemask_t *maskp, int nbits)
return nxt;
}
-#define NODE_MASK_LAST_WORD BITMAP_LAST_WORD_MASK(MAX_NUMNODES)
-
-#if MAX_NUMNODES <= BITS_PER_LONG
-
-#define NODE_MASK_ALL \
-((nodemask_t) { { \
- [BITS_TO_LONGS(MAX_NUMNODES)-1] = NODE_MASK_LAST_WORD \
-} })
-
-#else
-
-#define NODE_MASK_ALL \
-((nodemask_t) { { \
- [0 ... BITS_TO_LONGS(MAX_NUMNODES)-2] = ~0UL, \
- [BITS_TO_LONGS(MAX_NUMNODES)-1] = NODE_MASK_LAST_WORD \
-} })
-
-#endif
-
-#define NODE_MASK_NONE \
-((nodemask_t) { { \
- [0 ... BITS_TO_LONGS(MAX_NUMNODES)-1] = 0UL \
-} })
-
#if MAX_NUMNODES > 1
#define for_each_node_mask(node, mask) \
for ((node) = first_node(mask); \
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Xen-devel] [PATCH v3 08/10] xen/nodemask: Introduce unlocked __nodemask_{set, clear}() helpers
2019-07-29 12:11 [Xen-devel] [PATCH v3 00/10] xen/nodemask: API cleanup and fixes Andrew Cooper
` (6 preceding siblings ...)
2019-07-29 12:12 ` [Xen-devel] [PATCH v3 07/10] xen/nodemask: Drop nodes_{setall, clear}() and improve the initialisers Andrew Cooper
@ 2019-07-29 12:12 ` Andrew Cooper
2019-07-30 11:26 ` Jan Beulich
2019-07-29 12:12 ` [Xen-devel] [PATCH v3 09/10] xen/nodemask: Sanitise the remainder of the nodemask API Andrew Cooper
2019-07-29 12:12 ` [Xen-devel] [PATCH v3 10/10] xen/nodemask: Drop remaining refeces to linux Andrew Cooper
9 siblings, 1 reply; 33+ messages in thread
From: Andrew Cooper @ 2019-07-29 12:12 UTC (permalink / raw)
To: Xen-devel
Cc: Stefano Stabellini, Wei Liu, Andrew Cooper, Julien Grall,
Jan Beulich, Volodymyr Babchuk, Roger Pau Monné
As with the cpumask side of things, there are times when we do not need locked
bit operations on a nodemask.
Convert appropriate callers. Three of them operate on init-time data, while
domain_update_node_affinity() already has updates to the nodemask in question
protected by a spinlock.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
v3:
* New
---
xen/arch/x86/dom0_build.c | 2 +-
xen/arch/x86/srat.c | 4 ++--
xen/common/domain.c | 2 +-
xen/include/xen/nodemask.h | 12 ++++++++++++
4 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
index 06500c87c6..c625e64d03 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -246,7 +246,7 @@ unsigned int __init dom0_max_vcpus(void)
for ( i = 0; i < dom0_nr_pxms; ++i )
if ( (node = pxm_to_node(dom0_pxms[i])) != NUMA_NO_NODE )
- node_set(node, dom0_nodes);
+ __nodemask_set(node, &dom0_nodes);
nodes_and(dom0_nodes, dom0_nodes, node_online_map);
if ( nodes_empty(dom0_nodes) )
dom0_nodes = node_online_map;
diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c
index 506a56d66b..5f44ac27f1 100644
--- a/xen/arch/x86/srat.c
+++ b/xen/arch/x86/srat.c
@@ -228,7 +228,7 @@ acpi_numa_x2apic_affinity_init(const struct acpi_srat_x2apic_cpu_affinity *pa)
}
apicid_to_node[pa->apic_id] = node;
- node_set(node, processor_nodes_parsed);
+ __nodemask_set(node, &processor_nodes_parsed);
acpi_numa = 1;
printk(KERN_INFO "SRAT: PXM %u -> APIC %08x -> Node %u\n",
pxm, pa->apic_id, node);
@@ -261,7 +261,7 @@ acpi_numa_processor_affinity_init(const struct acpi_srat_cpu_affinity *pa)
return;
}
apicid_to_node[pa->apic_id] = node;
- node_set(node, processor_nodes_parsed);
+ __nodemask_set(node, &processor_nodes_parsed);
acpi_numa = 1;
printk(KERN_INFO "SRAT: PXM %u -> APIC %02x -> Node %u\n",
pxm, pa->apic_id, node);
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 11565a64b3..5dbc68cbc3 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -617,7 +617,7 @@ void domain_update_node_affinity(struct domain *d)
d->node_affinity = NODEMASK_NONE;
for_each_cpu ( cpu, dom_affinity )
- node_set(cpu_to_node(cpu), d->node_affinity);
+ __nodemask_set(cpu_to_node(cpu), &d->node_affinity);
}
spin_unlock(&d->node_affinity_lock);
diff --git a/xen/include/xen/nodemask.h b/xen/include/xen/nodemask.h
index 9933fec5c4..1605c1bcc5 100644
--- a/xen/include/xen/nodemask.h
+++ b/xen/include/xen/nodemask.h
@@ -11,7 +11,9 @@
* The available nodemask operations are:
*
* void node_set(node, mask) turn on bit 'node' in mask
+ * void __nodemask_set(node, mask) turn on bit 'node' in mask (unlocked)
* void node_clear(node, mask) turn off bit 'node' in mask
+ * void __nodemask_clear(node, mask) turn off bit 'node' in mask (unlocked)
* bool nodemask_test(node, mask) true iff bit 'node' set in mask
* int node_test_and_set(node, mask) test and set bit 'node' in mask
*
@@ -100,12 +102,22 @@ static inline void __node_set(int node, volatile nodemask_t *dstp)
set_bit(node, dstp->bits);
}
+static inline void __nodemask_set(unsigned int node, nodemask_t *dst)
+{
+ __set_bit(node, dst->bits);
+}
+
#define node_clear(node, dst) __node_clear((node), &(dst))
static inline void __node_clear(int node, volatile nodemask_t *dstp)
{
clear_bit(node, dstp->bits);
}
+static inline void __nodemask_clear(unsigned int node, nodemask_t *dst)
+{
+ __clear_bit(node, dst->bits);
+}
+
static inline bool nodemask_test(unsigned int node, const nodemask_t *dst)
{
return test_bit(node, dst->bits);
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Xen-devel] [PATCH v3 09/10] xen/nodemask: Sanitise the remainder of the nodemask API
2019-07-29 12:11 [Xen-devel] [PATCH v3 00/10] xen/nodemask: API cleanup and fixes Andrew Cooper
` (7 preceding siblings ...)
2019-07-29 12:12 ` [Xen-devel] [PATCH v3 08/10] xen/nodemask: Introduce unlocked __nodemask_{set, clear}() helpers Andrew Cooper
@ 2019-07-29 12:12 ` Andrew Cooper
2019-07-30 11:43 ` Jan Beulich
2019-07-29 12:12 ` [Xen-devel] [PATCH v3 10/10] xen/nodemask: Drop remaining refeces to linux Andrew Cooper
9 siblings, 1 reply; 33+ messages in thread
From: Andrew Cooper @ 2019-07-29 12:12 UTC (permalink / raw)
To: Xen-devel
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
Julien Grall, Jan Beulich, Roger Pau Monné
The nodemask API differs from the cpumask API because each wrapper to bitmap
operations is further wrapped by a macro which takes the address of the
nodemask objects.
This results in code which is slightly confusing to read as it doesn't follow
C's calling conventions, and prohibits the use of slightly more complicated
constructs for specifying parameters.
Drop all wrapping macros, rename the nodemask static inline functions to drop
the double underscores, and feed MAX_NUMNODES into appropriate locations.
Furthermore, the naming is inconsistent. As we're changing all callers
anyway, rationalise all the naming to be of the form nodemask_*(), and update
the types to per Xen's latest expectations.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
v3:
* Split various bits out into earlier patches
* Rename the APIs to be consistent.
v2:
* New
---
xen/arch/x86/dom0_build.c | 8 +-
xen/arch/x86/numa.c | 4 +-
xen/arch/x86/srat.c | 9 ++-
xen/common/domain.c | 4 +-
xen/common/page_alloc.c | 26 +++----
xen/common/sched_credit.c | 2 +-
xen/common/sysctl.c | 2 +-
xen/include/xen/nodemask.h | 186 ++++++++++++++++++++-------------------------
8 files changed, 109 insertions(+), 132 deletions(-)
diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
index c625e64d03..cb0f693d76 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -247,10 +247,10 @@ unsigned int __init dom0_max_vcpus(void)
for ( i = 0; i < dom0_nr_pxms; ++i )
if ( (node = pxm_to_node(dom0_pxms[i])) != NUMA_NO_NODE )
__nodemask_set(node, &dom0_nodes);
- nodes_and(dom0_nodes, dom0_nodes, node_online_map);
- if ( nodes_empty(dom0_nodes) )
+ nodemask_and(&dom0_nodes, &dom0_nodes, &node_online_map);
+ if ( nodemask_empty(&dom0_nodes) )
dom0_nodes = node_online_map;
- for_each_node_mask ( node, dom0_nodes )
+ for_each_node_mask ( node, &dom0_nodes )
cpumask_or(&dom0_cpus, &dom0_cpus, &node_to_cpumask(node));
cpumask_and(&dom0_cpus, &dom0_cpus, cpupool0->cpu_valid);
if ( cpumask_empty(&dom0_cpus) )
@@ -344,7 +344,7 @@ unsigned long __init dom0_compute_nr_pages(
if ( !dom0_mem_set && CONFIG_DOM0_MEM[0] )
parse_dom0_mem(CONFIG_DOM0_MEM);
- for_each_node_mask ( node, dom0_nodes )
+ for_each_node_mask ( node, &dom0_nodes )
avail += avail_domheap_pages_region(node, 0, 0) +
initial_images_nrpages(node);
diff --git a/xen/arch/x86/numa.c b/xen/arch/x86/numa.c
index 7473f83b7b..d3db213432 100644
--- a/xen/arch/x86/numa.c
+++ b/xen/arch/x86/numa.c
@@ -186,13 +186,13 @@ void __init numa_init_array(void)
mapping. To avoid this fill in the mapping for all possible
CPUs, as the number of CPUs is not known yet.
We round robin the existing nodes. */
- rr = first_node(node_online_map);
+ rr = nodemask_first(&node_online_map);
for ( i = 0; i < nr_cpu_ids; i++ )
{
if ( cpu_to_node[i] != NUMA_NO_NODE )
continue;
numa_set_node(i, rr);
- rr = cycle_node(rr, node_online_map);
+ rr = nodemask_cycle(rr, &node_online_map);
}
}
diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c
index 5f44ac27f1..9e37e24d70 100644
--- 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)) {
nd->start = start;
nd->end = end;
} else {
@@ -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 )
if (start < nodes[j].end
&& end > nodes[j].start) {
if (start >= nodes[j].start) {
@@ -480,10 +480,11 @@ int __init acpi_scan_nodes(u64 start, u64 end)
return -1;
}
- nodes_or(all_nodes_parsed, memory_nodes_parsed, processor_nodes_parsed);
+ nodemask_or(&all_nodes_parsed, &memory_nodes_parsed,
+ &processor_nodes_parsed);
/* Finally register nodes */
- for_each_node_mask(i, all_nodes_parsed)
+ for_each_node_mask( i, &all_nodes_parsed )
{
u64 size = nodes[i].end - nodes[i].start;
if ( size == 0 )
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 5dbc68cbc3..3228b08b3a 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -630,7 +630,7 @@ 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) )
+ if ( nodemask_empty(affinity) )
return -EINVAL;
spin_lock(&d->node_affinity_lock);
@@ -639,7 +639,7 @@ int domain_set_node_affinity(struct domain *d, const nodemask_t *affinity)
* Being/becoming explicitly affine to all nodes is not particularly
* useful. Let's take it as the `reset node affinity` command.
*/
- if ( nodes_full(*affinity) )
+ if ( nodemask_full(affinity) )
{
d->auto_node_affinity = 1;
goto out;
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 77e649d065..97539ffd69 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -821,12 +821,12 @@ static struct page_info *get_free_buddy(unsigned int zone_lo,
* may have bit set outside of node_online_map. Clamp it.
*/
if ( d )
- nodes_and(nodemask, nodemask, d->node_affinity);
+ nodemask_and(&nodemask, &nodemask, &d->node_affinity);
if ( node == NUMA_NO_NODE )
{
if ( d != NULL )
- node = cycle_node(d->last_alloc_node, nodemask);
+ node = nodemask_cycle(d->last_alloc_node, &nodemask);
if ( node >= MAX_NUMNODES )
node = cpu_to_node(smp_processor_id());
@@ -882,19 +882,19 @@ static struct page_info *get_free_buddy(unsigned int zone_lo,
{
/* Very first node may be caller-specified and outside nodemask. */
ASSERT(!nodemask_retry);
- first = node = first_node(nodemask);
+ first = node = nodemask_first(&nodemask);
if ( node < MAX_NUMNODES )
continue;
}
- else if ( (node = next_node(node, nodemask)) >= MAX_NUMNODES )
- node = first_node(nodemask);
+ else if ( (node = nodemask_next(node, &nodemask)) >= MAX_NUMNODES )
+ node = nodemask_first(&nodemask);
if ( node == first )
{
/* When we have tried all in nodemask, we fall back to others. */
if ( (memflags & MEMF_exact_node) || nodemask_retry++ )
return NULL;
- nodes_andnot(nodemask, node_online_map, nodemask);
- first = node = first_node(nodemask);
+ nodemask_andnot(&nodemask, &node_online_map, &nodemask);
+ first = node = nodemask_first(&nodemask);
if ( node >= MAX_NUMNODES )
return NULL;
}
@@ -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;
/*
@@ -1182,7 +1182,7 @@ static unsigned int node_to_scrub(bool get_node)
for ( ; ; )
{
do {
- node = cycle_node(node, node_online_map);
+ node = nodemask_cycle(node, &node_online_map);
} while ( !cpumask_empty(&node_to_cpumask(node)) &&
(node != local_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);
return node_to_scrub(false) != NUMA_NO_NODE;
}
@@ -2010,7 +2010,7 @@ static void __init scrub_heap_pages(void)
continue;
last_distance = INT_MAX;
- best_node = first_node(node_online_map);
+ best_node = nodemask_first(&node_online_map);
/* Figure out which NODE CPUs are close. */
for_each_online_node ( j )
{
diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
index 81dee5e472..3e569d6970 100644
--- a/xen/common/sched_credit.c
+++ b/xen/common/sched_credit.c
@@ -1810,7 +1810,7 @@ csched_load_balance(struct csched_private *prv, int cpu,
} while( peer_cpu != first_cpu );
next_node:
- peer_node = cycle_node(peer_node, node_online_map);
+ peer_node = nodemask_cycle(peer_node, &node_online_map);
} while( peer_node != node );
}
diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index 765effde8d..c8c6805bb4 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -280,7 +280,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
bool_t do_meminfo = !guest_handle_is_null(ni->meminfo);
bool_t do_distance = !guest_handle_is_null(ni->distance);
- num_nodes = last_node(node_online_map) + 1;
+ num_nodes = nodemask_last(&node_online_map) + 1;
if ( do_meminfo || do_distance )
{
diff --git a/xen/include/xen/nodemask.h b/xen/include/xen/nodemask.h
index 1605c1bcc5..ed918e4a8d 100644
--- a/xen/include/xen/nodemask.h
+++ b/xen/include/xen/nodemask.h
@@ -10,30 +10,30 @@
*
* The available nodemask operations are:
*
- * void node_set(node, mask) turn on bit 'node' in mask
+ * void nodemask_set(node, mask) turn on bit 'node' in mask
* void __nodemask_set(node, mask) turn on bit 'node' in mask (unlocked)
- * void node_clear(node, mask) turn off bit 'node' in mask
+ * void nodemask_clear(node, mask) turn off bit 'node' in mask
* void __nodemask_clear(node, mask) turn off bit 'node' in mask (unlocked)
* bool nodemask_test(node, mask) true iff bit 'node' set in mask
- * int node_test_and_set(node, mask) test and set bit 'node' in mask
+ * bool nodemask_test_and_set(node, mask) test and set bit 'node' in mask
*
- * void nodes_and(dst, src1, src2) dst = src1 & src2 [intersection]
- * void nodes_or(dst, src1, src2) dst = src1 | src2 [union]
- * void nodes_xor(dst, src1, src2) dst = src1 ^ src2
- * void nodes_andnot(dst, src1, src2) dst = src1 & ~src2
- * void nodes_complement(dst, src) dst = ~src
+ * void nodemask_and(dst, src1, src2) dst = src1 & src2 [intersection]
+ * void nodemask_or(dst, src1, src2) dst = src1 | src2 [union]
+ * void nodemask_xor(dst, src1, src2) dst = src1 ^ src2
+ * void nodemask_andnot(dst, src1, src2)dst = src1 & ~src2
+ * void nodemask_complement(dst, src) dst = ~src
*
- * int nodes_equal(mask1, mask2) Does mask1 == mask2?
- * int nodes_intersects(mask1, mask2) Do mask1 and mask2 intersect?
- * int nodes_subset(mask1, mask2) Is mask1 a subset of mask2?
- * int nodes_empty(mask) Is mask empty (no bits sets)?
- * int nodes_full(mask) Is mask full (all bits sets)?
- * int nodes_weight(mask) Hamming weight - number of set bits
+ * bool nodemask_equal(mask1, mask2) Does mask1 == mask2?
+ * bool nodemask_intersects(mask1, mask2) Do mask1 and mask2 intersect?
+ * bool nodemask_subset(mask1, mask2) Is mask1 a subset of mask2?
+ * bool nodemask_empty(mask) Is mask empty (no bits sets)?
+ * bool nodemask_full(mask) Is mask full (all bits sets)?
+ * unsigned int nodemask_weight(mask) Hamming weight - number of set bits
*
- * int first_node(mask) Number lowest set bit, or MAX_NUMNODES
- * int next_node(node, mask) Next node past 'node', or MAX_NUMNODES
- * int last_node(mask) Number highest set bit, or MAX_NUMNODES
- * int cycle_node(node, mask) Next node cycling from 'node', or
+ * node nodemask_first(mask) Number lowest set bit, or MAX_NUMNODES
+ * node nodemask_next(node, mask) Next node past 'node', or MAX_NUMNODES
+ * node nodemask_last(mask) Number highest set bit, or MAX_NUMNODES
+ * node nodemask_cycle(node, mask) Next node cycling from 'node', or
* MAX_NUMNODES
*
* nodemask_t NODEMASK_OF(node) Initializer - bit 'node' set
@@ -43,7 +43,7 @@
*
* for_each_node_mask(node, mask) for-loop node over mask
*
- * int num_online_nodes() Number of online Nodes
+ * unsigned int num_online_nodes() Number of online Nodes
*
* bool node_online(node) Is this node online?
*
@@ -96,10 +96,9 @@ typedef struct { DECLARE_BITMAP(bits, MAX_NUMNODES); } nodemask_t;
#endif /* MAX_NUMNODES */
-#define node_set(node, dst) __node_set((node), &(dst))
-static inline void __node_set(int node, volatile nodemask_t *dstp)
+static inline void nodemask_set(unsigned int node, nodemask_t *dst)
{
- set_bit(node, dstp->bits);
+ set_bit(node, dst->bits);
}
static inline void __nodemask_set(unsigned int node, nodemask_t *dst)
@@ -107,10 +106,9 @@ static inline void __nodemask_set(unsigned int node, nodemask_t *dst)
__set_bit(node, dst->bits);
}
-#define node_clear(node, dst) __node_clear((node), &(dst))
-static inline void __node_clear(int node, volatile nodemask_t *dstp)
+static inline void nodemask_clear(unsigned int node, nodemask_t *dst)
{
- clear_bit(node, dstp->bits);
+ clear_bit(node, dst->bits);
}
static inline void __nodemask_clear(unsigned int node, nodemask_t *dst)
@@ -123,139 +121,117 @@ static inline bool nodemask_test(unsigned int node, const nodemask_t *dst)
return test_bit(node, dst->bits);
}
-#define node_test_and_set(node, nodemask) \
- __node_test_and_set((node), &(nodemask))
-static inline int __node_test_and_set(int node, nodemask_t *addr)
+static inline bool nodemask_test_and_set(unsigned int node, nodemask_t *dst)
{
- return test_and_set_bit(node, addr->bits);
+ return test_and_set_bit(node, dst->bits);
}
-#define nodes_and(dst, src1, src2) \
- __nodes_and(&(dst), &(src1), &(src2), MAX_NUMNODES)
-static inline void __nodes_and(nodemask_t *dstp, const nodemask_t *src1p,
- const nodemask_t *src2p, int nbits)
+static inline void nodemask_and(nodemask_t *dst, const nodemask_t *src1,
+ const nodemask_t *src2)
{
- bitmap_and(dstp->bits, src1p->bits, src2p->bits, nbits);
+ bitmap_and(dst->bits, src1->bits, src2->bits, MAX_NUMNODES);
}
-#define nodes_or(dst, src1, src2) \
- __nodes_or(&(dst), &(src1), &(src2), MAX_NUMNODES)
-static inline void __nodes_or(nodemask_t *dstp, const nodemask_t *src1p,
- const nodemask_t *src2p, int nbits)
+static inline void nodemask_or(nodemask_t *dst, const nodemask_t *src1,
+ const nodemask_t *src2)
{
- bitmap_or(dstp->bits, src1p->bits, src2p->bits, nbits);
+ bitmap_or(dst->bits, src1->bits, src2->bits, MAX_NUMNODES);
}
-#define nodes_xor(dst, src1, src2) \
- __nodes_xor(&(dst), &(src1), &(src2), MAX_NUMNODES)
-static inline void __nodes_xor(nodemask_t *dstp, const nodemask_t *src1p,
- const nodemask_t *src2p, int nbits)
+static inline void nodemask_xor(nodemask_t *dst, const nodemask_t *src1,
+ const nodemask_t *src2)
{
- bitmap_xor(dstp->bits, src1p->bits, src2p->bits, nbits);
+ bitmap_xor(dst->bits, src1->bits, src2->bits, MAX_NUMNODES);
}
-#define nodes_andnot(dst, src1, src2) \
- __nodes_andnot(&(dst), &(src1), &(src2), MAX_NUMNODES)
-static inline void __nodes_andnot(nodemask_t *dstp, const nodemask_t *src1p,
- const nodemask_t *src2p, int nbits)
+static inline void nodemask_andnot(nodemask_t *dst, const nodemask_t *src1,
+ const nodemask_t *src2)
{
- bitmap_andnot(dstp->bits, src1p->bits, src2p->bits, nbits);
+ bitmap_andnot(dst->bits, src1->bits, src2->bits, MAX_NUMNODES);
}
-#define nodes_complement(dst, src) \
- __nodes_complement(&(dst), &(src), MAX_NUMNODES)
-static inline void __nodes_complement(nodemask_t *dstp,
- const nodemask_t *srcp, int nbits)
+static inline void nodemask_complement(nodemask_t *dst, const nodemask_t *src)
{
- bitmap_complement(dstp->bits, srcp->bits, nbits);
+ bitmap_complement(dst->bits, src->bits, MAX_NUMNODES);
}
-#define nodes_equal(src1, src2) \
- __nodes_equal(&(src1), &(src2), MAX_NUMNODES)
-static inline int __nodes_equal(const nodemask_t *src1p,
- const nodemask_t *src2p, int nbits)
+static inline bool nodemask_equal(const nodemask_t *src1,
+ const nodemask_t *src2)
{
- return bitmap_equal(src1p->bits, src2p->bits, nbits);
+ return bitmap_equal(src1->bits, src2->bits, MAX_NUMNODES);
}
-#define nodes_intersects(src1, src2) \
- __nodes_intersects(&(src1), &(src2), MAX_NUMNODES)
-static inline int __nodes_intersects(const nodemask_t *src1p,
- const nodemask_t *src2p, int nbits)
+static inline bool nodemask_intersects(const nodemask_t *src1,
+ const nodemask_t *src2)
{
- return bitmap_intersects(src1p->bits, src2p->bits, nbits);
+ return bitmap_intersects(src1->bits, src2->bits, MAX_NUMNODES);
}
-#define nodes_subset(src1, src2) \
- __nodes_subset(&(src1), &(src2), MAX_NUMNODES)
-static inline int __nodes_subset(const nodemask_t *src1p,
- const nodemask_t *src2p, int nbits)
+static inline bool nodemask_subset(const nodemask_t *src1,
+ const nodemask_t *src2)
{
- return bitmap_subset(src1p->bits, src2p->bits, nbits);
+ return bitmap_subset(src1->bits, src2->bits, MAX_NUMNODES);
}
-#define nodes_empty(src) __nodes_empty(&(src), MAX_NUMNODES)
-static inline int __nodes_empty(const nodemask_t *srcp, int nbits)
+static inline bool nodemask_empty(const nodemask_t *src)
{
- return bitmap_empty(srcp->bits, nbits);
+ return bitmap_empty(src->bits, MAX_NUMNODES);
}
-#define nodes_full(nodemask) __nodes_full(&(nodemask), MAX_NUMNODES)
-static inline int __nodes_full(const nodemask_t *srcp, int nbits)
+static inline bool nodemask_full(const nodemask_t *src)
{
- return bitmap_full(srcp->bits, nbits);
+ return bitmap_full(src->bits, MAX_NUMNODES);
}
-#define nodes_weight(nodemask) __nodes_weight(&(nodemask), MAX_NUMNODES)
-static inline int __nodes_weight(const nodemask_t *srcp, int nbits)
+static inline unsigned int nodemask_weight(const nodemask_t *src)
{
- return bitmap_weight(srcp->bits, nbits);
+ return bitmap_weight(src->bits, MAX_NUMNODES);
}
/* FIXME: better would be to fix all architectures to never return
> MAX_NUMNODES, then the silly min_ts could be dropped. */
-#define first_node(src) __first_node(&(src), MAX_NUMNODES)
-static inline int __first_node(const nodemask_t *srcp, int nbits)
+static inline unsigned int nodemask_first(const nodemask_t *src)
{
- return min_t(int, nbits, find_first_bit(srcp->bits, nbits));
+ return min_t(unsigned int, MAX_NUMNODES,
+ find_first_bit(src->bits, MAX_NUMNODES));
}
-#define next_node(n, src) __next_node((n), &(src), MAX_NUMNODES)
-static inline int __next_node(int n, const nodemask_t *srcp, int nbits)
+static inline unsigned int nodemask_next(unsigned int n, const nodemask_t *src)
{
- return min_t(int, nbits, find_next_bit(srcp->bits, nbits, n+1));
+ return min_t(unsigned int, MAX_NUMNODES,
+ find_next_bit(src->bits, MAX_NUMNODES, n + 1));
}
-#define last_node(src) __last_node(&(src), MAX_NUMNODES)
-static inline int __last_node(const nodemask_t *srcp, int nbits)
+static inline unsigned int nodemask_last(const nodemask_t *src)
{
- int node, pnode = nbits;
- for (node = __first_node(srcp, nbits);
- node < nbits;
- node = __next_node(node, srcp, nbits))
- pnode = node;
- return pnode;
+ unsigned int node, pnode = MAX_NUMNODES;
+
+ for ( node = nodemask_first(src);
+ node < MAX_NUMNODES; node = nodemask_next(node, src) )
+ pnode = node;
+
+ return pnode;
}
-#define cycle_node(n, src) __cycle_node((n), &(src), MAX_NUMNODES)
-static inline int __cycle_node(int n, const nodemask_t *maskp, int nbits)
+static inline unsigned int nodemask_cycle(unsigned int n, const nodemask_t *src)
{
- int nxt = __next_node(n, maskp, nbits);
+ unsigned int nxt = nodemask_next(n, src);
+
+ if ( nxt == MAX_NUMNODES )
+ nxt = nodemask_first(src);
- if (nxt == nbits)
- nxt = __first_node(maskp, nbits);
return nxt;
}
#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 */
@@ -267,16 +243,16 @@ static inline int __cycle_node(int n, const nodemask_t *maskp, int nbits)
extern nodemask_t node_online_map;
#if MAX_NUMNODES > 1
-#define num_online_nodes() nodes_weight(node_online_map)
+#define num_online_nodes() nodemask_weight(&node_online_map)
#define node_online(node) nodemask_test(node, &node_online_map)
#else
-#define num_online_nodes() 1
+#define num_online_nodes() 1U
#define node_online(node) ((node) == 0)
#endif
-#define node_set_online(node) set_bit((node), node_online_map.bits)
-#define node_set_offline(node) clear_bit((node), node_online_map.bits)
+#define node_set_online(node) set_bit(node, node_online_map.bits)
+#define node_set_offline(node) clear_bit(node, node_online_map.bits)
-#define for_each_online_node(node) for_each_node_mask((node), node_online_map)
+#define for_each_online_node(node) for_each_node_mask(node, &node_online_map)
#endif /* __LINUX_NODEMASK_H */
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Xen-devel] [PATCH v3 10/10] xen/nodemask: Drop remaining refeces to linux
2019-07-29 12:11 [Xen-devel] [PATCH v3 00/10] xen/nodemask: API cleanup and fixes Andrew Cooper
` (8 preceding siblings ...)
2019-07-29 12:12 ` [Xen-devel] [PATCH v3 09/10] xen/nodemask: Sanitise the remainder of the nodemask API Andrew Cooper
@ 2019-07-29 12:12 ` Andrew Cooper
2019-07-30 11:44 ` Jan Beulich
9 siblings, 1 reply; 33+ messages in thread
From: Andrew Cooper @ 2019-07-29 12:12 UTC (permalink / raw)
To: Xen-devel
Cc: Stefano Stabellini, Wei Liu, Andrew Cooper, Julien Grall,
Jan Beulich, Volodymyr Babchuk, Roger Pau Monné
This file has now diverged completely from its Linux roots.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
---
xen/include/xen/nodemask.h | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/xen/include/xen/nodemask.h b/xen/include/xen/nodemask.h
index ed918e4a8d..be4016e571 100644
--- a/xen/include/xen/nodemask.h
+++ b/xen/include/xen/nodemask.h
@@ -1,11 +1,11 @@
-#ifndef __LINUX_NODEMASK_H
-#define __LINUX_NODEMASK_H
+#ifndef XEN_NODEMASK_H
+#define XEN_NODEMASK_H
/*
* Nodemasks provide a bitmap suitable for representing the
* set of Node's in a system, one bit position per Node number.
*
- * See detailed comments in the file linux/bitmap.h describing the
+ * See detailed comments in the file xen/bitmap.h describing the
* data type on which these nodemasks are based.
*
* The available nodemask operations are:
@@ -255,4 +255,13 @@ extern nodemask_t node_online_map;
#define for_each_online_node(node) for_each_node_mask(node, &node_online_map)
-#endif /* __LINUX_NODEMASK_H */
+#endif /* XEN_NODEMASK_H */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [Xen-devel] [PATCH v3 01/10] page-alloc: Clamp get_free_buddy() to online nodes
2019-07-29 12:11 ` [Xen-devel] [PATCH v3 01/10] page-alloc: Clamp get_free_buddy() to online nodes Andrew Cooper
@ 2019-07-29 15:48 ` Jan Beulich
2019-07-29 17:26 ` Andrew Cooper
0 siblings, 1 reply; 33+ messages in thread
From: Jan Beulich @ 2019-07-29 15:48 UTC (permalink / raw)
To: Andrew Cooper
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Julien Grall,
Xen-devel, Roger Pau Monné
On 29.07.2019 14:11, Andrew Cooper wrote:
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -811,11 +811,18 @@ static struct page_info *get_free_buddy(unsigned int zone_lo,
> const struct domain *d)
> {
> nodeid_t first, node = MEMF_get_node(memflags), req_node = node;
> - nodemask_t nodemask = d ? d->node_affinity : node_online_map;
> + nodemask_t nodemask = node_online_map;
> unsigned int j, zone, nodemask_retry = 0;
> struct page_info *pg;
> bool use_unscrubbed = (memflags & MEMF_no_scrub);
>
> + /*
> + * d->node_affinity is our preferred allocation set if provided, but it
> + * may have bit set outside of node_online_map. Clamp it.
Would you mind adding the apparently missing "s" to "bit"?
> + */
> + 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. I think you want to append an
"intersects" check in the if(). With that feel free to promote my
A-b to R-b.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Xen-devel] [PATCH v3 02/10] xen/bitmap: Drop {bitmap, cpumask, nodes}_shift_{left, right}()
2019-07-29 12:11 ` [Xen-devel] [PATCH v3 02/10] xen/bitmap: Drop {bitmap, cpumask, nodes}_shift_{left, right}() Andrew Cooper
@ 2019-07-29 15:50 ` Jan Beulich
0 siblings, 0 replies; 33+ messages in thread
From: Jan Beulich @ 2019-07-29 15:50 UTC (permalink / raw)
To: Andrew Cooper
Cc: StefanoStabellini, Wei Liu, Julien Grall, Xen-devel,
Volodymyr Babchuk, Roger Pau Monné
On 29.07.2019 14:11, Andrew Cooper wrote:
> These operations have never been used in Xen since their introduction, and it
> doesn't seem likely that they will in the future.
>
> Bloat-o-meter reports that they aren't the smalled when compiled, either.
"smallest"?
> add/remove: 0/2 grow/shrink: 0/0 up/down: 0/-814 (-814)
> Function old new delta
> __bitmap_shift_left 366 - -366
> __bitmap_shift_right 448 - -448
> Total: Before=3323730, After=3322916, chg -0.02%
>
> Suggested-by: Jan Beulich <JBeulich@suse.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Thanks for taking the time,
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Xen-devel] [PATCH v3 03/10] xen/nodemask: Drop any_online_node() and first_unset_node()
2019-07-29 12:11 ` [Xen-devel] [PATCH v3 03/10] xen/nodemask: Drop any_online_node() and first_unset_node() Andrew Cooper
@ 2019-07-29 15:51 ` Jan Beulich
0 siblings, 0 replies; 33+ messages in thread
From: Jan Beulich @ 2019-07-29 15:51 UTC (permalink / raw)
To: Andrew Cooper
Cc: Stefano Stabellini, Wei Liu, Julien Grall, Xen-devel,
Volodymyr Babchuk, Roger Pau Monné
On 29.07.2019 14:11, Andrew Cooper wrote:
> These have never been used in Xen, and it is unlikely that they would be
> useful in the future.
>
> any_online_cpu() was dropped by c/s 22bdce1c048 "eliminate first_cpu() etc"
> but the API comment was left in place. Drop that too.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Xen-devel] [PATCH v3 01/10] page-alloc: Clamp get_free_buddy() to online nodes
2019-07-29 15:48 ` Jan Beulich
@ 2019-07-29 17:26 ` Andrew Cooper
2019-07-30 8:09 ` Jan Beulich
0 siblings, 1 reply; 33+ messages in thread
From: Andrew Cooper @ 2019-07-29 17:26 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Julien Grall,
Xen-devel, Roger Pau Monné
On 29/07/2019 16:48, Jan Beulich wrote:
> On 29.07.2019 14:11, Andrew Cooper wrote:
>> --- a/xen/common/page_alloc.c
>> +++ b/xen/common/page_alloc.c
>> @@ -811,11 +811,18 @@ static struct page_info *get_free_buddy(unsigned int zone_lo,
>> const struct domain *d)
>> {
>> nodeid_t first, node = MEMF_get_node(memflags), req_node = node;
>> - nodemask_t nodemask = d ? d->node_affinity : node_online_map;
>> + nodemask_t nodemask = node_online_map;
>> unsigned int j, zone, nodemask_retry = 0;
>> struct page_info *pg;
>> bool use_unscrubbed = (memflags & MEMF_no_scrub);
>>
>> + /*
>> + * d->node_affinity is our preferred allocation set if provided, but it
>> + * may have bit set outside of node_online_map. Clamp it.
> Would you mind adding the apparently missing "s" to "bit"?
Oops yes.
>
>> + */
>> + 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).
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Xen-devel] [PATCH v3 01/10] page-alloc: Clamp get_free_buddy() to online nodes
2019-07-29 17:26 ` Andrew Cooper
@ 2019-07-30 8:09 ` Jan Beulich
2019-07-30 17:32 ` Andrew Cooper
0 siblings, 1 reply; 33+ messages in thread
From: Jan Beulich @ 2019-07-30 8:09 UTC (permalink / raw)
To: Andrew Cooper
Cc: StefanoStabellini, Wei Liu, George Dunlap, Julien Grall,
Xen-devel, Roger Pau Monné
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 understand, btw, why the function calls
domain_update_node_affinity() after clearing the flag.) Therefore I
don't think an assertion like you suggest would be legitimate. For
this there would be a prereq of domain_update_node_affinity() clearing
the flag when it finds the intersection has become empty. And even
then there would be a window in time where the intersection might be
actively empty, so some synchronization would be needed in addition.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Xen-devel] [PATCH v3 04/10] xen/mask: Convert {cpu, node}mask_test() to be static inline
2019-07-29 12:11 ` [Xen-devel] [PATCH v3 04/10] xen/mask: Convert {cpu, node}mask_test() to be static inline Andrew Cooper
@ 2019-07-30 8:52 ` Jan Beulich
2019-07-30 9:03 ` Andrew Cooper
0 siblings, 1 reply; 33+ messages in thread
From: Jan Beulich @ 2019-07-30 8:52 UTC (permalink / raw)
To: Andrew Cooper
Cc: StefanoStabellini, Wei Liu, Julien Grall, Xen-devel,
Volodymyr Babchuk, Roger Pau Monné
On 29.07.2019 14:11, Andrew Cooper wrote:
> The buggy version of GCC isn't supported by Xen, so reimplement the helpers
> with type checking, using Xen's latest type expectations.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
with one remark:
> @@ -117,9 +108,10 @@ static inline void cpumask_clear(cpumask_t *dstp)
> bitmap_zero(dstp->bits, nr_cpumask_bits);
> }
>
> -/* No static inline type checking - see Subtlety (1) above. */
> -#define cpumask_test_cpu(cpu, cpumask) \
> - test_bit(cpumask_check(cpu), (cpumask)->bits)
> +static inline bool cpumask_test_cpu(unsigned int cpu, const cpumask_t *dst)
In particular in combination with const I don't think "dst" is a
particularly good name. "src", "msk", or "mask" would all seem
better to me.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Xen-devel] [PATCH v3 05/10] xen/cpumask: Introduce a CPUMASK_PR() wrapper for printing
2019-07-29 12:11 ` [Xen-devel] [PATCH v3 05/10] xen/cpumask: Introduce a CPUMASK_PR() wrapper for printing Andrew Cooper
@ 2019-07-30 8:55 ` Jan Beulich
0 siblings, 0 replies; 33+ messages in thread
From: Jan Beulich @ 2019-07-30 8:55 UTC (permalink / raw)
To: Andrew Cooper
Cc: Juergen Gross, StefanoStabellini, Wei Liu, George Dunlap,
Dario Faggioli, Julien Grall, Xen-devel, Volodymyr Babchuk,
Roger Pau Monné
On 29.07.2019 14:11, Andrew Cooper wrote:
> Having to specify 'nr_cpu_id, cpumask_bits(foo)' for all printing operations
> is quite repetative. Introduce a wrapper to help.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Xen-devel] [PATCH v3 06/10] xen/nodemask: Introduce a NODEMASK_PR() wrapper for printing
2019-07-29 12:12 ` [Xen-devel] [PATCH v3 06/10] xen/nodemask: Introduce a NODEMASK_PR() " Andrew Cooper
@ 2019-07-30 8:58 ` Jan Beulich
2019-07-30 9:09 ` Andrew Cooper
0 siblings, 1 reply; 33+ messages in thread
From: Jan Beulich @ 2019-07-30 8:58 UTC (permalink / raw)
To: Andrew Cooper
Cc: StefanoStabellini, Wei Liu, Julien Grall, Xen-devel,
Volodymyr Babchuk, Roger Pau Monné
On 29.07.2019 14:12, Andrew Cooper wrote:
> Rework nodes_addr() into nodemask_bits() and change the indirection to match
> its cpumask_bits() counterpart, and update the caller.
>
> Use NODEMASK_PR() to fix up one opencoded access into nodemask.bits in
> dump_domains().
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
albeit I think that ...
> @@ -58,6 +58,15 @@
> #include <xen/numa.h>
>
> typedef struct { DECLARE_BITMAP(bits, MAX_NUMNODES); } nodemask_t;
> +
> +/*
> + * printf arguments for a nodemask. Shorthand for using '%*pb[l]' when
> + * printing a nodemask.
> + */
> +#define NODEMASK_PR(src) MAX_NUMNODES, nodemask_bits(src)
> +
> +#define nodemask_bits(src) ((src)->bits)
... it would be nice if nodemask_bits() was defined before its first
use, no matter that such ordering is irrelevant for macros.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Xen-devel] [PATCH v3 04/10] xen/mask: Convert {cpu, node}mask_test() to be static inline
2019-07-30 8:52 ` Jan Beulich
@ 2019-07-30 9:03 ` Andrew Cooper
0 siblings, 0 replies; 33+ messages in thread
From: Andrew Cooper @ 2019-07-30 9:03 UTC (permalink / raw)
To: Jan Beulich
Cc: StefanoStabellini, Wei Liu, Julien Grall, Xen-devel,
Volodymyr Babchuk, Roger Pau Monné
On 30/07/2019 09:52, Jan Beulich wrote:
> On 29.07.2019 14:11, Andrew Cooper wrote:
>> The buggy version of GCC isn't supported by Xen, so reimplement the helpers
>> with type checking, using Xen's latest type expectations.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>
> with one remark:
>
>> @@ -117,9 +108,10 @@ static inline void cpumask_clear(cpumask_t *dstp)
>> bitmap_zero(dstp->bits, nr_cpumask_bits);
>> }
>>
>> -/* No static inline type checking - see Subtlety (1) above. */
>> -#define cpumask_test_cpu(cpu, cpumask) \
>> - test_bit(cpumask_check(cpu), (cpumask)->bits)
>> +static inline bool cpumask_test_cpu(unsigned int cpu, const cpumask_t *dst)
> In particular in combination with const I don't think "dst" is a
> particularly good name. "src", "msk", or "mask" would all seem
> better to me.
Oops - I had several passes trying to get naming consistent, and this
one clearly slipped. I intended src here.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Xen-devel] [PATCH v3 06/10] xen/nodemask: Introduce a NODEMASK_PR() wrapper for printing
2019-07-30 8:58 ` Jan Beulich
@ 2019-07-30 9:09 ` Andrew Cooper
0 siblings, 0 replies; 33+ messages in thread
From: Andrew Cooper @ 2019-07-30 9:09 UTC (permalink / raw)
To: Jan Beulich
Cc: StefanoStabellini, Wei Liu, Julien Grall, Xen-devel,
Volodymyr Babchuk, Roger Pau Monné
On 30/07/2019 09:58, Jan Beulich wrote:
> On 29.07.2019 14:12, Andrew Cooper wrote:
>> Rework nodes_addr() into nodemask_bits() and change the indirection to match
>> its cpumask_bits() counterpart, and update the caller.
>>
>> Use NODEMASK_PR() to fix up one opencoded access into nodemask.bits in
>> dump_domains().
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> albeit I think that ...
>
>> @@ -58,6 +58,15 @@
>> #include <xen/numa.h>
>>
>> typedef struct { DECLARE_BITMAP(bits, MAX_NUMNODES); } nodemask_t;
>> +
>> +/*
>> + * printf arguments for a nodemask. Shorthand for using '%*pb[l]' when
>> + * printing a nodemask.
>> + */
>> +#define NODEMASK_PR(src) MAX_NUMNODES, nodemask_bits(src)
>> +
>> +#define nodemask_bits(src) ((src)->bits)
> ... it would be nice if nodemask_bits() was defined before its first
> use, no matter that such ordering is irrelevant for macros.
In this case, I decided that having NODEMASK_PR() closer to the
nodemask_t is more important, overall.
In practice, they are adjacent and remain so at the end of the series.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Xen-devel] [PATCH v3 07/10] xen/nodemask: Drop nodes_{setall, clear}() and improve the initialisers
2019-07-29 12:12 ` [Xen-devel] [PATCH v3 07/10] xen/nodemask: Drop nodes_{setall, clear}() and improve the initialisers Andrew Cooper
@ 2019-07-30 9:44 ` Jan Beulich
2019-07-31 12:49 ` Andrew Cooper
0 siblings, 1 reply; 33+ messages in thread
From: Jan Beulich @ 2019-07-30 9:44 UTC (permalink / raw)
To: Andrew Cooper
Cc: StefanoStabellini, Wei Liu, Julien Grall, Xen-devel,
Volodymyr Babchuk, Roger Pau Monné
On 29.07.2019 14:12, Andrew Cooper wrote:
> There is no need to use runtime variable-length clearing when MAX_NUMNODES is
> known to the compiler. Drop these functions and use the initialisers instead.
The only slight concern I have with this is that it further locks
down the maximum remaining to be a compile time constant. But this
is not an objection, just a remark.
> @@ -67,7 +65,34 @@ typedef struct { DECLARE_BITMAP(bits, MAX_NUMNODES); } nodemask_t;
>
> #define nodemask_bits(src) ((src)->bits)
>
> -extern nodemask_t _unused_nodemask_arg_;
> +#define NODEMASK_LAST_WORD BITMAP_LAST_WORD_MASK(MAX_NUMNODES)
> +
> +#define NODEMASK_NONE \
> +((nodemask_t) {{ \
> + [0 ... BITS_TO_LONGS(MAX_NUMNODES) - 1] = 0 \
> +}})
> +
> +#if MAX_NUMNODES <= BITS_PER_LONG
> +
> +#define NODEMASK_ALL ((nodemask_t) {{ NODEMASK_LAST_WORD }})
> +#define NODEMASK_OF(node) ((nodemask_t) {{ 1UL << (node) }})
> +
> +#else /* MAX_NUMNODES > BITS_PER_LONG */
> +
> +#define NODEMASK_ALL \
> +((nodemask_t) {{ \
> + [0 ... BITS_TO_LONGS(MAX_NUMNODES) - 2] = ~0UL, \
> + [BITS_TO_LONGS(MAX_NUMNODES) - 1] = NODEMASK_LAST_WORD \
> +}})
> +
> +#define NODEMASK_OF(node) \
> +({ \
> + nodemask_t m = NODES_NONE; \
> + m.bits[(node) / BITS_PER_LONG] = 1UL << ((node) % BITS_PER_LONG); \
I think you will want to avoid the double evaluation of "node"
here. With this taken care of
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Xen-devel] [PATCH v3 08/10] xen/nodemask: Introduce unlocked __nodemask_{set, clear}() helpers
2019-07-29 12:12 ` [Xen-devel] [PATCH v3 08/10] xen/nodemask: Introduce unlocked __nodemask_{set, clear}() helpers Andrew Cooper
@ 2019-07-30 11:26 ` Jan Beulich
2019-07-30 17:48 ` Andrew Cooper
0 siblings, 1 reply; 33+ messages in thread
From: Jan Beulich @ 2019-07-30 11:26 UTC (permalink / raw)
To: Andrew Cooper
Cc: StefanoStabellini, Wei Liu, Julien Grall, Xen-devel,
Volodymyr Babchuk, Roger Pau Monné
On 29.07.2019 14:12, Andrew Cooper wrote:
> --- a/xen/include/xen/nodemask.h
> +++ b/xen/include/xen/nodemask.h
> @@ -11,7 +11,9 @@
> * The available nodemask operations are:
> *
> * void node_set(node, mask) turn on bit 'node' in mask
> + * void __nodemask_set(node, mask) turn on bit 'node' in mask (unlocked)
> * void node_clear(node, mask) turn off bit 'node' in mask
> + * void __nodemask_clear(node, mask) turn off bit 'node' in mask (unlocked)
To be honest I'm unhappy to see you introduce new name space
violations. I realize you want to have the node mask interfaces
match the CPU mask one as closely as possible, but I think we
should diverge here (and eventually make the CPU mask ones
follow whatever route we go here). As to naming, since these
are static inlines, a single leading underscore may be an
option (albeit I'd prefer to avoid this). Another option would
be to have double infix underscores (nodemask__set()). Yet
another option would be to express the non-atomicity in a
verbal way rather than by the number of underscores used. I'm
afraid I don't have a good naming suggestion in that case,
though.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Xen-devel] [PATCH v3 09/10] xen/nodemask: Sanitise the remainder of the nodemask API
2019-07-29 12:12 ` [Xen-devel] [PATCH v3 09/10] xen/nodemask: Sanitise the remainder of the nodemask API Andrew Cooper
@ 2019-07-30 11:43 ` Jan Beulich
0 siblings, 0 replies; 33+ messages in thread
From: Jan Beulich @ 2019-07-30 11:43 UTC (permalink / raw)
To: Andrew Cooper
Cc: StefanoStabellini, Wei Liu, George Dunlap, Julien Grall,
Xen-devel, Roger Pau Monné
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Xen-devel] [PATCH v3 10/10] xen/nodemask: Drop remaining refeces to linux
2019-07-29 12:12 ` [Xen-devel] [PATCH v3 10/10] xen/nodemask: Drop remaining refeces to linux Andrew Cooper
@ 2019-07-30 11:44 ` Jan Beulich
0 siblings, 0 replies; 33+ messages in thread
From: Jan Beulich @ 2019-07-30 11:44 UTC (permalink / raw)
To: Andrew Cooper
Cc: Stefano Stabellini, Wei Liu, Julien Grall, Xen-devel,
Volodymyr Babchuk, Roger Pau Monné
On 29.07.2019 14:12, Andrew Cooper wrote:
> This file has now diverged completely from its Linux roots.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
I guess the title wants to say "references".
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Xen-devel] [PATCH v3 01/10] page-alloc: Clamp get_free_buddy() to online nodes
2019-07-30 8:09 ` Jan Beulich
@ 2019-07-30 17:32 ` Andrew Cooper
2019-07-31 8:22 ` Jan Beulich
0 siblings, 1 reply; 33+ messages in thread
From: Andrew Cooper @ 2019-07-30 17:32 UTC (permalink / raw)
To: Jan Beulich
Cc: StefanoStabellini, Wei Liu, George Dunlap, Julien Grall,
Xen-devel, Roger Pau Monné
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
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [Xen-devel] [PATCH v3 08/10] xen/nodemask: Introduce unlocked __nodemask_{set, clear}() helpers
2019-07-30 11:26 ` Jan Beulich
@ 2019-07-30 17:48 ` Andrew Cooper
2019-07-31 8:41 ` Jan Beulich
0 siblings, 1 reply; 33+ messages in thread
From: Andrew Cooper @ 2019-07-30 17:48 UTC (permalink / raw)
To: Jan Beulich
Cc: StefanoStabellini, Wei Liu, Julien Grall, Xen-devel,
Volodymyr Babchuk, Roger Pau Monné
On 30/07/2019 12:26, Jan Beulich wrote:
> On 29.07.2019 14:12, Andrew Cooper wrote:
>> --- a/xen/include/xen/nodemask.h
>> +++ b/xen/include/xen/nodemask.h
>> @@ -11,7 +11,9 @@
>> * The available nodemask operations are:
>> *
>> * void node_set(node, mask) turn on bit 'node' in mask
>> + * void __nodemask_set(node, mask) turn on bit 'node' in mask (unlocked)
>> * void node_clear(node, mask) turn off bit 'node' in mask
>> + * void __nodemask_clear(node, mask) turn off bit 'node' in mask (unlocked)
> To be honest I'm unhappy to see you introduce new name space
> violations. I realize you want to have the node mask interfaces
> match the CPU mask one as closely as possible, but I think we
> should diverge here (and eventually make the CPU mask ones
> follow whatever route we go here). As to naming, since these
> are static inlines, a single leading underscore may be an
> option (albeit I'd prefer to avoid this). Another option would
> be to have double infix underscores (nodemask__set()). Yet
> another option would be to express the non-atomicity in a
> verbal way rather than by the number of underscores used. I'm
> afraid I don't have a good naming suggestion in that case,
> though.
I'm really not as concerned with namespace violations as you seem to
be. We are not a library having to cooperate with arbitrary others - we
are a freestanding build with the compiler being the only other source
of symbols in the namespace. Our chances of colliding are very low to
begin with, and fixing conflicts if they arise is trivial.
As you note, there is nothing obvious as an alternative. The double
infix is a no-go as far as I'm concerned - it's far too subtle in the code.
Ergo, consistency with cpumask and the bitops is the winning factor here.
At some point in the future if an alternative is chosen, it will want
changing consistently throughout the tree.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Xen-devel] [PATCH v3 01/10] page-alloc: Clamp get_free_buddy() to online nodes
2019-07-30 17:32 ` Andrew Cooper
@ 2019-07-31 8:22 ` Jan Beulich
2019-07-31 9:01 ` Andrew Cooper
0 siblings, 1 reply; 33+ messages in thread
From: Jan Beulich @ 2019-07-31 8:22 UTC (permalink / raw)
To: Andrew Cooper
Cc: StefanoStabellini, Wei Liu, George Dunlap, Julien Grall,
Xen-devel, Roger Pau Monné
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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Xen-devel] [PATCH v3 08/10] xen/nodemask: Introduce unlocked __nodemask_{set, clear}() helpers
2019-07-30 17:48 ` Andrew Cooper
@ 2019-07-31 8:41 ` Jan Beulich
0 siblings, 0 replies; 33+ messages in thread
From: Jan Beulich @ 2019-07-31 8:41 UTC (permalink / raw)
To: Andrew Cooper
Cc: StefanoStabellini, Wei Liu, Julien Grall, Xen-devel,
Volodymyr Babchuk, Roger Pau Monné
On 30.07.2019 19:48, Andrew Cooper wrote:
> On 30/07/2019 12:26, Jan Beulich wrote:
>> On 29.07.2019 14:12, Andrew Cooper wrote:
>>> --- a/xen/include/xen/nodemask.h
>>> +++ b/xen/include/xen/nodemask.h
>>> @@ -11,7 +11,9 @@
>>> * The available nodemask operations are:
>>> *
>>> * void node_set(node, mask) turn on bit 'node' in mask
>>> + * void __nodemask_set(node, mask) turn on bit 'node' in mask (unlocked)
>>> * void node_clear(node, mask) turn off bit 'node' in mask
>>> + * void __nodemask_clear(node, mask) turn off bit 'node' in mask (unlocked)
>> To be honest I'm unhappy to see you introduce new name space
>> violations. I realize you want to have the node mask interfaces
>> match the CPU mask one as closely as possible, but I think we
>> should diverge here (and eventually make the CPU mask ones
>> follow whatever route we go here). As to naming, since these
>> are static inlines, a single leading underscore may be an
>> option (albeit I'd prefer to avoid this). Another option would
>> be to have double infix underscores (nodemask__set()). Yet
>> another option would be to express the non-atomicity in a
>> verbal way rather than by the number of underscores used. I'm
>> afraid I don't have a good naming suggestion in that case,
>> though.
>
> I'm really not as concerned with namespace violations as you seem to
> be. We are not a library having to cooperate with arbitrary others - we
> are a freestanding build with the compiler being the only other source
> of symbols in the namespace. Our chances of colliding are very low to
> begin with, and fixing conflicts if they arise is trivial.
Trivial or not, such conflicts are problematic for branches in
security only maintenance mode. While we _can_ backport such
patches there, we really _shouldn't_.
> As you note, there is nothing obvious as an alternative. The double
> infix is a no-go as far as I'm concerned - it's far too subtle in the code.
Well, the single-underscore-prefix variant is at least better
than the double one, even if still not ideal. I'd prefer if
you switched, but I don't mean to stand in the way of this
going in, so
Acked-by: Jan Beulich <jbeulich@suse.com>
even in its current shape.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Xen-devel] [PATCH v3 01/10] page-alloc: Clamp get_free_buddy() to online nodes
2019-07-31 8:22 ` Jan Beulich
@ 2019-07-31 9:01 ` Andrew Cooper
0 siblings, 0 replies; 33+ messages in thread
From: Andrew Cooper @ 2019-07-31 9:01 UTC (permalink / raw)
To: Jan Beulich
Cc: StefanoStabellini, Wei Liu, George Dunlap, Julien Grall,
Xen-devel, Roger Pau Monné
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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Xen-devel] [PATCH v3 07/10] xen/nodemask: Drop nodes_{setall, clear}() and improve the initialisers
2019-07-30 9:44 ` Jan Beulich
@ 2019-07-31 12:49 ` Andrew Cooper
2019-07-31 13:12 ` Jan Beulich
0 siblings, 1 reply; 33+ messages in thread
From: Andrew Cooper @ 2019-07-31 12:49 UTC (permalink / raw)
To: Jan Beulich
Cc: StefanoStabellini, Wei Liu, Julien Grall, Xen-devel,
Volodymyr Babchuk, Roger Pau Monné
[-- Attachment #1.1: Type: text/plain, Size: 5702 bytes --]
On 30/07/2019 10:44, Jan Beulich wrote:
> On 29.07.2019 14:12, Andrew Cooper wrote:
>> There is no need to use runtime variable-length clearing when MAX_NUMNODES is
>> known to the compiler. Drop these functions and use the initialisers instead.
> The only slight concern I have with this is that it further locks
> down the maximum remaining to be a compile time constant. But this
> is not an objection, just a remark.
The maximum number of nodes I'm aware of at all is 10, and we currently default to 64.
I don't think it is likely that we'll get to a point where a runtime nodesize is a realistic consideration that we would want to take.
>
>> @@ -67,7 +65,34 @@ typedef struct { DECLARE_BITMAP(bits, MAX_NUMNODES); } nodemask_t;
>>
>> #define nodemask_bits(src) ((src)->bits)
>>
>> -extern nodemask_t _unused_nodemask_arg_;
>> +#define NODEMASK_LAST_WORD BITMAP_LAST_WORD_MASK(MAX_NUMNODES)
>> +
>> +#define NODEMASK_NONE \
>> +((nodemask_t) {{ \
>> + [0 ... BITS_TO_LONGS(MAX_NUMNODES) - 1] = 0 \
>> +}})
>> +
>> +#if MAX_NUMNODES <= BITS_PER_LONG
>> +
>> +#define NODEMASK_ALL ((nodemask_t) {{ NODEMASK_LAST_WORD }})
>> +#define NODEMASK_OF(node) ((nodemask_t) {{ 1UL << (node) }})
>> +
>> +#else /* MAX_NUMNODES > BITS_PER_LONG */
>> +
>> +#define NODEMASK_ALL \
>> +((nodemask_t) {{ \
>> + [0 ... BITS_TO_LONGS(MAX_NUMNODES) - 2] = ~0UL, \
>> + [BITS_TO_LONGS(MAX_NUMNODES) - 1] = NODEMASK_LAST_WORD \
>> +}})
>> +
>> +#define NODEMASK_OF(node) \
>> +({ \
>> + nodemask_t m = NODES_NONE; \
>> + m.bits[(node) / BITS_PER_LONG] = 1UL << ((node) % BITS_PER_LONG); \
> I think you will want to avoid the double evaluation of "node"
> here. With this taken care of
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
I'm afraid this is a bit more complicated after I spotted another opencoding of NODEMASK_OF().
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index 00b64c3322..24af9bc471 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -46,7 +46,7 @@ struct cpuinfo_arm cpu_data[NR_CPUS];
register_t __cpu_logical_map[NR_CPUS] = { [0 ... NR_CPUS-1] = MPIDR_INVALID };
/* Fake one node for now. See also include/asm-arm/numa.h */
-nodemask_t __read_mostly node_online_map = { { [0] = 1UL } };
+nodemask_t __read_mostly node_online_map = NODEMASK_OF(0);
/* Xen stack for bringing up the first CPU. */
static unsigned char __initdata cpu0_boot_stack[STACK_SIZE]
diff --git a/xen/arch/x86/numa.c b/xen/arch/x86/numa.c
index 7473f83b7b..9a55c013e5 100644
--- a/xen/arch/x86/numa.c
+++ b/xen/arch/x86/numa.c
@@ -47,7 +47,7 @@ nodeid_t apicid_to_node[MAX_LOCAL_APIC] = {
};
cpumask_t node_to_cpumask[MAX_NUMNODES] __read_mostly;
-nodemask_t __read_mostly node_online_map = { { [0] = 1UL } };
+nodemask_t __read_mostly node_online_map = NODEMASK_OF(0);
bool numa_off;
s8 acpi_numa = 0;
diff --git a/xen/include/xen/nodemask.h b/xen/include/xen/nodemask.h
index 9933fec5c4..c474dca3f0 100644
--- a/xen/include/xen/nodemask.h
+++ b/xen/include/xen/nodemask.h
@@ -86,11 +86,9 @@ typedef struct { DECLARE_BITMAP(bits, MAX_NUMNODES); } nodemask_t;
}})
#define NODEMASK_OF(node) \
-({ \
- nodemask_t m = NODES_NONE; \
- m.bits[(node) / BITS_PER_LONG] = 1UL << ((node) % BITS_PER_LONG); \
- m; \
-})
+((nodemask_t) {{ \
+ [(node) / BITS_PER_LONG] = 1UL << ((node) % BITS_PER_LONG) \
+}})
#endif /* MAX_NUMNODES */
and to be used as a static initialiser, NODEMASK_OF() needs to be an ICE and can't use ({}) .
I don't see a way to avoid expanding node twice, but given that its wrapper is in ALL_CAPS and obviously a macro.
Furthermore, experimenting with a deliberate attempt to provoke this, I got
numa.c: In function ‘numa_initmem_init’:
/local/xen.git/xen/include/xen/nodemask.h:90:10: error: nonconstant array index in initializer
[(node) / BITS_PER_LONG] = 1UL << ((node) % BITS_PER_LONG) \
^
numa.c:274:23: note: in expansion of macro ‘NODEMASK_OF’
node_online_map = NODEMASK_OF(foo++);
^~~~~~~~~~~
/local/xen.git/xen/include/xen/nodemask.h:90:10: note: (near initialization for ‘(anonymous).bits’)
[(node) / BITS_PER_LONG] = 1UL << ((node) % BITS_PER_LONG) \
^
numa.c:274:23: note: in expansion of macro ‘NODEMASK_OF’
node_online_map = NODEMASK_OF(foo++);
^~~~~~~~~~~
from GCC 6.3, which I think covers everything we need, and will prevent side effects from double expansion in practice.
~Andrew
[-- Attachment #1.2: Type: text/html, Size: 7497 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Xen-devel] [PATCH v3 07/10] xen/nodemask: Drop nodes_{setall, clear}() and improve the initialisers
2019-07-31 12:49 ` Andrew Cooper
@ 2019-07-31 13:12 ` Jan Beulich
2019-07-31 13:32 ` Andrew Cooper
0 siblings, 1 reply; 33+ messages in thread
From: Jan Beulich @ 2019-07-31 13:12 UTC (permalink / raw)
To: Andrew Cooper
Cc: StefanoStabellini, Wei Liu, Julien Grall, Xen-devel,
Volodymyr Babchuk, Roger Pau Monné
On 31.07.2019 14:49, Andrew Cooper wrote:
> I don't see a way to avoid expanding node twice, but given that its wrapper is in ALL_CAPS and obviously a macro.
>
> Furthermore, experimenting with a deliberate attempt to provoke this, I got
>
> numa.c: In function ‘numa_initmem_init’:
>
> /local/xen.git/xen/include/xen/nodemask.h:90:10: error: nonconstant array index in initializer
>
> [(node) / BITS_PER_LONG] = 1UL << ((node) % BITS_PER_LONG) \
>
> ^
>
> numa.c:274:23: note: in expansion of macro ‘NODEMASK_OF’
>
> node_online_map = NODEMASK_OF(foo++);
>
> ^~~~~~~~~~~
>
> /local/xen.git/xen/include/xen/nodemask.h:90:10: note: (near initialization for ‘(anonymous).bits’)
>
> [(node) / BITS_PER_LONG] = 1UL << ((node) % BITS_PER_LONG) \
>
> ^
>
> numa.c:274:23: note: in expansion of macro ‘NODEMASK_OF’
>
> node_online_map = NODEMASK_OF(foo++);
>
> ^~~~~~~~~~~
>
> from GCC 6.3, which I think covers everything we need, and will prevent side effects from double expansion in practice.
Ah, right. With this my R-b applies to the change as is (with the
additional adjustments folded in that you've pointed out).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Xen-devel] [PATCH v3 07/10] xen/nodemask: Drop nodes_{setall, clear}() and improve the initialisers
2019-07-31 13:12 ` Jan Beulich
@ 2019-07-31 13:32 ` Andrew Cooper
0 siblings, 0 replies; 33+ messages in thread
From: Andrew Cooper @ 2019-07-31 13:32 UTC (permalink / raw)
To: Jan Beulich
Cc: StefanoStabellini, Wei Liu, Julien Grall, Xen-devel,
Volodymyr Babchuk, Roger Pau Monné
On 31/07/2019 14:12, Jan Beulich wrote:
> On 31.07.2019 14:49, Andrew Cooper wrote:
>> I don't see a way to avoid expanding node twice, but given that its wrapper is in ALL_CAPS and obviously a macro.
>>
>> Furthermore, experimenting with a deliberate attempt to provoke this, I got
>>
>> numa.c: In function ‘numa_initmem_init’:
>>
>> /local/xen.git/xen/include/xen/nodemask.h:90:10: error: nonconstant array index in initializer
>>
>> [(node) / BITS_PER_LONG] = 1UL << ((node) % BITS_PER_LONG) \
>>
>> ^
>>
>> numa.c:274:23: note: in expansion of macro ‘NODEMASK_OF’
>>
>> node_online_map = NODEMASK_OF(foo++);
>>
>> ^~~~~~~~~~~
>>
>> /local/xen.git/xen/include/xen/nodemask.h:90:10: note: (near initialization for ‘(anonymous).bits’)
>>
>> [(node) / BITS_PER_LONG] = 1UL << ((node) % BITS_PER_LONG) \
>>
>> ^
>>
>> numa.c:274:23: note: in expansion of macro ‘NODEMASK_OF’
>>
>> node_online_map = NODEMASK_OF(foo++);
>>
>> ^~~~~~~~~~~
>>
>> from GCC 6.3, which I think covers everything we need, and will prevent side effects from double expansion in practice.
> Ah, right. With this my R-b applies to the change as is (with the
> additional adjustments folded in that you've pointed out).
I've actually tweaked it a fraction more to not bifurcate NODEMASK_OF()
in an ifdef, which means we'll get diagnostics like that out of the
compiler even when I haven't hacked NODES_SHIFT to 7 locally.
However, it now touches ARM code so I'll post a full v4 series with this
and later issues in the series addressed.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
end of thread, other threads:[~2019-07-31 13:33 UTC | newest]
Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-29 12:11 [Xen-devel] [PATCH v3 00/10] xen/nodemask: API cleanup and fixes Andrew Cooper
2019-07-29 12:11 ` [Xen-devel] [PATCH v3 01/10] page-alloc: Clamp get_free_buddy() to online nodes Andrew Cooper
2019-07-29 15:48 ` Jan Beulich
2019-07-29 17:26 ` Andrew Cooper
2019-07-30 8:09 ` Jan Beulich
2019-07-30 17:32 ` Andrew Cooper
2019-07-31 8:22 ` Jan Beulich
2019-07-31 9:01 ` Andrew Cooper
2019-07-29 12:11 ` [Xen-devel] [PATCH v3 02/10] xen/bitmap: Drop {bitmap, cpumask, nodes}_shift_{left, right}() Andrew Cooper
2019-07-29 15:50 ` Jan Beulich
2019-07-29 12:11 ` [Xen-devel] [PATCH v3 03/10] xen/nodemask: Drop any_online_node() and first_unset_node() Andrew Cooper
2019-07-29 15:51 ` Jan Beulich
2019-07-29 12:11 ` [Xen-devel] [PATCH v3 04/10] xen/mask: Convert {cpu, node}mask_test() to be static inline Andrew Cooper
2019-07-30 8:52 ` Jan Beulich
2019-07-30 9:03 ` Andrew Cooper
2019-07-29 12:11 ` [Xen-devel] [PATCH v3 05/10] xen/cpumask: Introduce a CPUMASK_PR() wrapper for printing Andrew Cooper
2019-07-30 8:55 ` Jan Beulich
2019-07-29 12:12 ` [Xen-devel] [PATCH v3 06/10] xen/nodemask: Introduce a NODEMASK_PR() " Andrew Cooper
2019-07-30 8:58 ` Jan Beulich
2019-07-30 9:09 ` Andrew Cooper
2019-07-29 12:12 ` [Xen-devel] [PATCH v3 07/10] xen/nodemask: Drop nodes_{setall, clear}() and improve the initialisers Andrew Cooper
2019-07-30 9:44 ` Jan Beulich
2019-07-31 12:49 ` Andrew Cooper
2019-07-31 13:12 ` Jan Beulich
2019-07-31 13:32 ` Andrew Cooper
2019-07-29 12:12 ` [Xen-devel] [PATCH v3 08/10] xen/nodemask: Introduce unlocked __nodemask_{set, clear}() helpers Andrew Cooper
2019-07-30 11:26 ` Jan Beulich
2019-07-30 17:48 ` Andrew Cooper
2019-07-31 8:41 ` Jan Beulich
2019-07-29 12:12 ` [Xen-devel] [PATCH v3 09/10] xen/nodemask: Sanitise the remainder of the nodemask API Andrew Cooper
2019-07-30 11:43 ` Jan Beulich
2019-07-29 12:12 ` [Xen-devel] [PATCH v3 10/10] xen/nodemask: Drop remaining refeces to linux Andrew Cooper
2019-07-30 11:44 ` Jan Beulich
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).