| Message ID | 20190729121204.13559-9-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/nodemask: API cleanup and fixes | expand |
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
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
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
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);
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(-)