* [PATCH] introduce and used relaxed cpumask operations
@ 2015-01-19 15:58 Jan Beulich
2015-01-21 12:21 ` Andrew Cooper
2015-01-21 14:28 ` George Dunlap
0 siblings, 2 replies; 9+ messages in thread
From: Jan Beulich @ 2015-01-19 15:58 UTC (permalink / raw)
To: xen-devel
Cc: Keir Fraser, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Ian Campbell
[-- Attachment #1: Type: text/plain, Size: 12105 bytes --]
Using atomic (LOCKed on x86) bitops for certain of the operations on
cpumask_t is overkill when the variables aren't concurrently accessible
(e.g. local function variables, or due to explicit locking). Introduce
alternatives using non-atomic bitops and use them where appropriate.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -157,7 +157,7 @@ static void evt_do_broadcast(cpumask_t *
{
unsigned int cpu = smp_processor_id();
- if ( cpumask_test_and_clear_cpu(cpu, mask) )
+ if ( __cpumask_tst_clr_cpu(cpu, mask) )
raise_softirq(TIMER_SOFTIRQ);
cpuidle_wakeup_mwait(mask);
@@ -196,7 +196,7 @@ again:
continue;
if ( deadline <= now )
- cpumask_set_cpu(cpu, &mask);
+ __cpumask_set_cpu(cpu, &mask);
else if ( deadline < next_event )
next_event = deadline;
}
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -1450,7 +1450,7 @@ void desc_guest_eoi(struct irq_desc *des
cpumask_copy(&cpu_eoi_map, action->cpu_eoi_map);
- if ( cpumask_test_and_clear_cpu(smp_processor_id(), &cpu_eoi_map) )
+ if ( __cpumask_tst_clr_cpu(smp_processor_id(), &cpu_eoi_map) )
{
__set_eoi_ready(desc);
spin_unlock(&desc->lock);
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -3212,7 +3212,7 @@ long do_mmuext_op(
for_each_online_cpu(cpu)
if ( !cpumask_intersects(&mask,
per_cpu(cpu_sibling_mask, cpu)) )
- cpumask_set_cpu(cpu, &mask);
+ __cpumask_set_cpu(cpu, &mask);
flush_mask(&mask, FLUSH_CACHE);
}
else
--- a/xen/arch/x86/platform_hypercall.c
+++ b/xen/arch/x86/platform_hypercall.c
@@ -451,7 +451,7 @@ ret_t do_platform_op(XEN_GUEST_HANDLE_PA
if ( !idletime )
{
- cpumask_clear_cpu(cpu, cpumap);
+ __cpumask_clr_cpu(cpu, cpumap);
continue;
}
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -178,7 +178,7 @@ static void smp_send_timer_broadcast_ipi
if ( cpumask_test_cpu(cpu, &mask) )
{
- cpumask_clear_cpu(cpu, &mask);
+ __cpumask_clr_cpu(cpu, &mask);
raise_softirq(TIMER_SOFTIRQ);
}
--- a/xen/common/core_parking.c
+++ b/xen/common/core_parking.c
@@ -75,11 +75,10 @@ static unsigned int core_parking_perform
if ( core_weight < core_tmp )
{
core_weight = core_tmp;
- cpumask_clear(&core_candidate_map);
- cpumask_set_cpu(cpu, &core_candidate_map);
+ cpumask_copy(&core_candidate_map, cpumask_of(cpu));
}
else if ( core_weight == core_tmp )
- cpumask_set_cpu(cpu, &core_candidate_map);
+ __cpumask_set_cpu(cpu, &core_candidate_map);
}
for_each_cpu(cpu, &core_candidate_map)
@@ -88,11 +87,10 @@ static unsigned int core_parking_perform
if ( sibling_weight < sibling_tmp )
{
sibling_weight = sibling_tmp;
- cpumask_clear(&sibling_candidate_map);
- cpumask_set_cpu(cpu, &sibling_candidate_map);
+ cpumask_copy(&sibling_candidate_map, cpumask_of(cpu));
}
else if ( sibling_weight == sibling_tmp )
- cpumask_set_cpu(cpu, &sibling_candidate_map);
+ __cpumask_set_cpu(cpu, &sibling_candidate_map);
}
cpu = cpumask_first(&sibling_candidate_map);
@@ -135,11 +133,10 @@ static unsigned int core_parking_power(u
if ( core_weight > core_tmp )
{
core_weight = core_tmp;
- cpumask_clear(&core_candidate_map);
- cpumask_set_cpu(cpu, &core_candidate_map);
+ cpumask_copy(&core_candidate_map, cpumask_of(cpu));
}
else if ( core_weight == core_tmp )
- cpumask_set_cpu(cpu, &core_candidate_map);
+ __cpumask_set_cpu(cpu, &core_candidate_map);
}
for_each_cpu(cpu, &core_candidate_map)
@@ -148,11 +145,10 @@ static unsigned int core_parking_power(u
if ( sibling_weight > sibling_tmp )
{
sibling_weight = sibling_tmp;
- cpumask_clear(&sibling_candidate_map);
- cpumask_set_cpu(cpu, &sibling_candidate_map);
+ cpumask_copy(&sibling_candidate_map, cpumask_of(cpu));
}
else if ( sibling_weight == sibling_tmp )
- cpumask_set_cpu(cpu, &sibling_candidate_map);
+ __cpumask_set_cpu(cpu, &sibling_candidate_map);
}
cpu = cpumask_first(&sibling_candidate_map);
--- a/xen/common/cpu.c
+++ b/xen/common/cpu.c
@@ -192,7 +192,7 @@ int disable_nonboot_cpus(void)
break;
}
- cpumask_set_cpu(cpu, &frozen_cpus);
+ __cpumask_set_cpu(cpu, &frozen_cpus);
}
BUG_ON(!error && (num_online_cpus() != 1));
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -1337,7 +1337,7 @@ static int __init find_non_smt(unsigned
if ( cpumask_intersects(dest, per_cpu(cpu_sibling_mask, i)) )
continue;
cpu = cpumask_first(per_cpu(cpu_sibling_mask, i));
- cpumask_set_cpu(cpu, dest);
+ __cpumask_set_cpu(cpu, dest);
}
return cpumask_weight(dest);
}
@@ -1449,7 +1449,7 @@ void __init scrub_heap_pages(void)
cpus = find_non_smt(best_node, &node_cpus);
if ( cpus == 0 )
{
- cpumask_set_cpu(smp_processor_id(), &node_cpus);
+ __cpumask_set_cpu(smp_processor_id(), &node_cpus);
cpus = 1;
}
/* We already have the node information from round #0. */
--- a/xen/common/sched_credit.c
+++ b/xen/common/sched_credit.c
@@ -372,7 +372,7 @@ __runq_tickle(unsigned int cpu, struct c
{
if ( cur->pri != CSCHED_PRI_IDLE )
SCHED_STAT_CRANK(tickle_idlers_none);
- cpumask_set_cpu(cpu, &mask);
+ __cpumask_set_cpu(cpu, &mask);
}
else if ( !idlers_empty )
{
@@ -422,7 +422,7 @@ __runq_tickle(unsigned int cpu, struct c
SCHED_VCPU_STAT_CRANK(cur, migrate_r);
SCHED_STAT_CRANK(migrate_kicked_away);
set_bit(_VPF_migrating, &cur->vcpu->pause_flags);
- cpumask_set_cpu(cpu, &mask);
+ __cpumask_set_cpu(cpu, &mask);
}
else if ( !new_idlers_empty )
{
@@ -432,7 +432,7 @@ __runq_tickle(unsigned int cpu, struct c
{
this_cpu(last_tickle_cpu) =
cpumask_cycle(this_cpu(last_tickle_cpu), &idle_mask);
- cpumask_set_cpu(this_cpu(last_tickle_cpu), &mask);
+ __cpumask_set_cpu(this_cpu(last_tickle_cpu), &mask);
}
else
cpumask_or(&mask, &mask, &idle_mask);
@@ -675,7 +675,7 @@ _csched_cpu_pick(const struct scheduler
*/
cpumask_and(&idlers, &cpu_online_map, CSCHED_PRIV(ops)->idlers);
if ( vc->processor == cpu && IS_RUNQ_IDLE(cpu) )
- cpumask_set_cpu(cpu, &idlers);
+ __cpumask_set_cpu(cpu, &idlers);
cpumask_and(&cpus, &cpus, &idlers);
/*
@@ -692,7 +692,7 @@ _csched_cpu_pick(const struct scheduler
*/
if ( !cpumask_test_cpu(cpu, &cpus) && !cpumask_empty(&cpus) )
cpu = cpumask_cycle(cpu, &cpus);
- cpumask_clear_cpu(cpu, &cpus);
+ __cpumask_clr_cpu(cpu, &cpus);
while ( !cpumask_empty(&cpus) )
{
@@ -1536,7 +1536,7 @@ csched_load_balance(struct csched_privat
/* Find out what the !idle are in this node */
cpumask_andnot(&workers, online, prv->idlers);
cpumask_and(&workers, &workers, &node_to_cpumask(peer_node));
- cpumask_clear_cpu(cpu, &workers);
+ __cpumask_clr_cpu(cpu, &workers);
peer_cpu = cpumask_first(&workers);
if ( peer_cpu >= nr_cpu_ids )
--- a/xen/common/sched_rt.c
+++ b/xen/common/sched_rt.c
@@ -663,7 +663,7 @@ burn_budget(const struct scheduler *ops,
* lock is grabbed before calling this function
*/
static struct rt_vcpu *
-__runq_pick(const struct scheduler *ops, cpumask_t *mask)
+__runq_pick(const struct scheduler *ops, const cpumask_t *mask)
{
struct list_head *runq = rt_runq(ops);
struct list_head *iter;
@@ -780,10 +780,7 @@ rt_schedule(const struct scheduler *ops,
}
else
{
- cpumask_t cur_cpu;
- cpumask_clear(&cur_cpu);
- cpumask_set_cpu(cpu, &cur_cpu);
- snext = __runq_pick(ops, &cur_cpu);
+ snext = __runq_pick(ops, cpumask_of(cpu));
if ( snext == NULL )
snext = rt_vcpu(idle_vcpu[cpu]);
--- a/xen/common/softirq.c
+++ b/xen/common/softirq.c
@@ -88,7 +88,7 @@ void cpumask_raise_softirq(const cpumask
if ( !test_and_set_bit(nr, &softirq_pending(cpu)) &&
cpu != this_cpu &&
!arch_skip_send_event_check(cpu) )
- cpumask_set_cpu(cpu, raise_mask);
+ __cpumask_set_cpu(cpu, raise_mask);
if ( raise_mask == &send_mask )
smp_send_event_check_mask(raise_mask);
@@ -106,7 +106,7 @@ void cpu_raise_softirq(unsigned int cpu,
if ( !per_cpu(batching, this_cpu) || in_irq() )
smp_send_event_check_cpu(cpu);
else
- set_bit(nr, &per_cpu(batch_mask, this_cpu));
+ __cpumask_set_cpu(nr, &per_cpu(batch_mask, this_cpu));
}
void cpu_raise_softirq_batch_begin(void)
@@ -122,7 +122,7 @@ void cpu_raise_softirq_batch_finish(void
ASSERT(per_cpu(batching, this_cpu));
for_each_cpu ( cpu, mask )
if ( !softirq_pending(cpu) )
- cpumask_clear_cpu(cpu, mask);
+ __cpumask_clr_cpu(cpu, mask);
smp_send_event_check_mask(mask);
cpumask_clear(mask);
--per_cpu(batching, this_cpu);
--- a/xen/include/xen/cpumask.h
+++ b/xen/include/xen/cpumask.h
@@ -103,11 +103,21 @@ static inline void cpumask_set_cpu(int c
set_bit(cpumask_check(cpu), dstp->bits);
}
+static inline void __cpumask_set_cpu(int cpu, cpumask_t *dstp)
+{
+ __set_bit(cpumask_check(cpu), dstp->bits);
+}
+
static inline void cpumask_clear_cpu(int cpu, volatile cpumask_t *dstp)
{
clear_bit(cpumask_check(cpu), dstp->bits);
}
+static inline void __cpumask_clr_cpu(int cpu, cpumask_t *dstp)
+{
+ __clear_bit(cpumask_check(cpu), dstp->bits);
+}
+
static inline void cpumask_setall(cpumask_t *dstp)
{
bitmap_fill(dstp->bits, nr_cpumask_bits);
@@ -122,16 +132,26 @@ static inline void cpumask_clear(cpumask
#define cpumask_test_cpu(cpu, cpumask) \
test_bit(cpumask_check(cpu), (cpumask)->bits)
-static inline int cpumask_test_and_set_cpu(int cpu, cpumask_t *addr)
+static inline int cpumask_test_and_set_cpu(int cpu, volatile cpumask_t *addr)
{
return test_and_set_bit(cpumask_check(cpu), addr->bits);
}
-static inline int cpumask_test_and_clear_cpu(int cpu, cpumask_t *addr)
+static inline int __cpumask_tst_set_cpu(int cpu, cpumask_t *addr)
+{
+ return __test_and_set_bit(cpumask_check(cpu), addr->bits);
+}
+
+static inline int cpumask_test_and_clear_cpu(int cpu, volatile cpumask_t *addr)
{
return test_and_clear_bit(cpumask_check(cpu), addr->bits);
}
+static inline int __cpumask_tst_clr_cpu(int cpu, cpumask_t *addr)
+{
+ return __test_and_clear_bit(cpumask_check(cpu), addr->bits);
+}
+
static inline void cpumask_and(cpumask_t *dstp, const cpumask_t *src1p,
const cpumask_t *src2p)
{
[-- Attachment #2: cpumask-relax.patch --]
[-- Type: text/plain, Size: 12150 bytes --]
introduce and used relaxed cpumask operations
Using atomic (LOCKed on x86) bitops for certain of the operations on
cpumask_t is overkill when the variables aren't concurrently accessible
(e.g. local function variables, or due to explicit locking). Introduce
alternatives using non-atomic bitops and use them where appropriate.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -157,7 +157,7 @@ static void evt_do_broadcast(cpumask_t *
{
unsigned int cpu = smp_processor_id();
- if ( cpumask_test_and_clear_cpu(cpu, mask) )
+ if ( __cpumask_tst_clr_cpu(cpu, mask) )
raise_softirq(TIMER_SOFTIRQ);
cpuidle_wakeup_mwait(mask);
@@ -196,7 +196,7 @@ again:
continue;
if ( deadline <= now )
- cpumask_set_cpu(cpu, &mask);
+ __cpumask_set_cpu(cpu, &mask);
else if ( deadline < next_event )
next_event = deadline;
}
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -1450,7 +1450,7 @@ void desc_guest_eoi(struct irq_desc *des
cpumask_copy(&cpu_eoi_map, action->cpu_eoi_map);
- if ( cpumask_test_and_clear_cpu(smp_processor_id(), &cpu_eoi_map) )
+ if ( __cpumask_tst_clr_cpu(smp_processor_id(), &cpu_eoi_map) )
{
__set_eoi_ready(desc);
spin_unlock(&desc->lock);
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -3212,7 +3212,7 @@ long do_mmuext_op(
for_each_online_cpu(cpu)
if ( !cpumask_intersects(&mask,
per_cpu(cpu_sibling_mask, cpu)) )
- cpumask_set_cpu(cpu, &mask);
+ __cpumask_set_cpu(cpu, &mask);
flush_mask(&mask, FLUSH_CACHE);
}
else
--- a/xen/arch/x86/platform_hypercall.c
+++ b/xen/arch/x86/platform_hypercall.c
@@ -451,7 +451,7 @@ ret_t do_platform_op(XEN_GUEST_HANDLE_PA
if ( !idletime )
{
- cpumask_clear_cpu(cpu, cpumap);
+ __cpumask_clr_cpu(cpu, cpumap);
continue;
}
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -178,7 +178,7 @@ static void smp_send_timer_broadcast_ipi
if ( cpumask_test_cpu(cpu, &mask) )
{
- cpumask_clear_cpu(cpu, &mask);
+ __cpumask_clr_cpu(cpu, &mask);
raise_softirq(TIMER_SOFTIRQ);
}
--- a/xen/common/core_parking.c
+++ b/xen/common/core_parking.c
@@ -75,11 +75,10 @@ static unsigned int core_parking_perform
if ( core_weight < core_tmp )
{
core_weight = core_tmp;
- cpumask_clear(&core_candidate_map);
- cpumask_set_cpu(cpu, &core_candidate_map);
+ cpumask_copy(&core_candidate_map, cpumask_of(cpu));
}
else if ( core_weight == core_tmp )
- cpumask_set_cpu(cpu, &core_candidate_map);
+ __cpumask_set_cpu(cpu, &core_candidate_map);
}
for_each_cpu(cpu, &core_candidate_map)
@@ -88,11 +87,10 @@ static unsigned int core_parking_perform
if ( sibling_weight < sibling_tmp )
{
sibling_weight = sibling_tmp;
- cpumask_clear(&sibling_candidate_map);
- cpumask_set_cpu(cpu, &sibling_candidate_map);
+ cpumask_copy(&sibling_candidate_map, cpumask_of(cpu));
}
else if ( sibling_weight == sibling_tmp )
- cpumask_set_cpu(cpu, &sibling_candidate_map);
+ __cpumask_set_cpu(cpu, &sibling_candidate_map);
}
cpu = cpumask_first(&sibling_candidate_map);
@@ -135,11 +133,10 @@ static unsigned int core_parking_power(u
if ( core_weight > core_tmp )
{
core_weight = core_tmp;
- cpumask_clear(&core_candidate_map);
- cpumask_set_cpu(cpu, &core_candidate_map);
+ cpumask_copy(&core_candidate_map, cpumask_of(cpu));
}
else if ( core_weight == core_tmp )
- cpumask_set_cpu(cpu, &core_candidate_map);
+ __cpumask_set_cpu(cpu, &core_candidate_map);
}
for_each_cpu(cpu, &core_candidate_map)
@@ -148,11 +145,10 @@ static unsigned int core_parking_power(u
if ( sibling_weight > sibling_tmp )
{
sibling_weight = sibling_tmp;
- cpumask_clear(&sibling_candidate_map);
- cpumask_set_cpu(cpu, &sibling_candidate_map);
+ cpumask_copy(&sibling_candidate_map, cpumask_of(cpu));
}
else if ( sibling_weight == sibling_tmp )
- cpumask_set_cpu(cpu, &sibling_candidate_map);
+ __cpumask_set_cpu(cpu, &sibling_candidate_map);
}
cpu = cpumask_first(&sibling_candidate_map);
--- a/xen/common/cpu.c
+++ b/xen/common/cpu.c
@@ -192,7 +192,7 @@ int disable_nonboot_cpus(void)
break;
}
- cpumask_set_cpu(cpu, &frozen_cpus);
+ __cpumask_set_cpu(cpu, &frozen_cpus);
}
BUG_ON(!error && (num_online_cpus() != 1));
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -1337,7 +1337,7 @@ static int __init find_non_smt(unsigned
if ( cpumask_intersects(dest, per_cpu(cpu_sibling_mask, i)) )
continue;
cpu = cpumask_first(per_cpu(cpu_sibling_mask, i));
- cpumask_set_cpu(cpu, dest);
+ __cpumask_set_cpu(cpu, dest);
}
return cpumask_weight(dest);
}
@@ -1449,7 +1449,7 @@ void __init scrub_heap_pages(void)
cpus = find_non_smt(best_node, &node_cpus);
if ( cpus == 0 )
{
- cpumask_set_cpu(smp_processor_id(), &node_cpus);
+ __cpumask_set_cpu(smp_processor_id(), &node_cpus);
cpus = 1;
}
/* We already have the node information from round #0. */
--- a/xen/common/sched_credit.c
+++ b/xen/common/sched_credit.c
@@ -372,7 +372,7 @@ __runq_tickle(unsigned int cpu, struct c
{
if ( cur->pri != CSCHED_PRI_IDLE )
SCHED_STAT_CRANK(tickle_idlers_none);
- cpumask_set_cpu(cpu, &mask);
+ __cpumask_set_cpu(cpu, &mask);
}
else if ( !idlers_empty )
{
@@ -422,7 +422,7 @@ __runq_tickle(unsigned int cpu, struct c
SCHED_VCPU_STAT_CRANK(cur, migrate_r);
SCHED_STAT_CRANK(migrate_kicked_away);
set_bit(_VPF_migrating, &cur->vcpu->pause_flags);
- cpumask_set_cpu(cpu, &mask);
+ __cpumask_set_cpu(cpu, &mask);
}
else if ( !new_idlers_empty )
{
@@ -432,7 +432,7 @@ __runq_tickle(unsigned int cpu, struct c
{
this_cpu(last_tickle_cpu) =
cpumask_cycle(this_cpu(last_tickle_cpu), &idle_mask);
- cpumask_set_cpu(this_cpu(last_tickle_cpu), &mask);
+ __cpumask_set_cpu(this_cpu(last_tickle_cpu), &mask);
}
else
cpumask_or(&mask, &mask, &idle_mask);
@@ -675,7 +675,7 @@ _csched_cpu_pick(const struct scheduler
*/
cpumask_and(&idlers, &cpu_online_map, CSCHED_PRIV(ops)->idlers);
if ( vc->processor == cpu && IS_RUNQ_IDLE(cpu) )
- cpumask_set_cpu(cpu, &idlers);
+ __cpumask_set_cpu(cpu, &idlers);
cpumask_and(&cpus, &cpus, &idlers);
/*
@@ -692,7 +692,7 @@ _csched_cpu_pick(const struct scheduler
*/
if ( !cpumask_test_cpu(cpu, &cpus) && !cpumask_empty(&cpus) )
cpu = cpumask_cycle(cpu, &cpus);
- cpumask_clear_cpu(cpu, &cpus);
+ __cpumask_clr_cpu(cpu, &cpus);
while ( !cpumask_empty(&cpus) )
{
@@ -1536,7 +1536,7 @@ csched_load_balance(struct csched_privat
/* Find out what the !idle are in this node */
cpumask_andnot(&workers, online, prv->idlers);
cpumask_and(&workers, &workers, &node_to_cpumask(peer_node));
- cpumask_clear_cpu(cpu, &workers);
+ __cpumask_clr_cpu(cpu, &workers);
peer_cpu = cpumask_first(&workers);
if ( peer_cpu >= nr_cpu_ids )
--- a/xen/common/sched_rt.c
+++ b/xen/common/sched_rt.c
@@ -663,7 +663,7 @@ burn_budget(const struct scheduler *ops,
* lock is grabbed before calling this function
*/
static struct rt_vcpu *
-__runq_pick(const struct scheduler *ops, cpumask_t *mask)
+__runq_pick(const struct scheduler *ops, const cpumask_t *mask)
{
struct list_head *runq = rt_runq(ops);
struct list_head *iter;
@@ -780,10 +780,7 @@ rt_schedule(const struct scheduler *ops,
}
else
{
- cpumask_t cur_cpu;
- cpumask_clear(&cur_cpu);
- cpumask_set_cpu(cpu, &cur_cpu);
- snext = __runq_pick(ops, &cur_cpu);
+ snext = __runq_pick(ops, cpumask_of(cpu));
if ( snext == NULL )
snext = rt_vcpu(idle_vcpu[cpu]);
--- a/xen/common/softirq.c
+++ b/xen/common/softirq.c
@@ -88,7 +88,7 @@ void cpumask_raise_softirq(const cpumask
if ( !test_and_set_bit(nr, &softirq_pending(cpu)) &&
cpu != this_cpu &&
!arch_skip_send_event_check(cpu) )
- cpumask_set_cpu(cpu, raise_mask);
+ __cpumask_set_cpu(cpu, raise_mask);
if ( raise_mask == &send_mask )
smp_send_event_check_mask(raise_mask);
@@ -106,7 +106,7 @@ void cpu_raise_softirq(unsigned int cpu,
if ( !per_cpu(batching, this_cpu) || in_irq() )
smp_send_event_check_cpu(cpu);
else
- set_bit(nr, &per_cpu(batch_mask, this_cpu));
+ __cpumask_set_cpu(nr, &per_cpu(batch_mask, this_cpu));
}
void cpu_raise_softirq_batch_begin(void)
@@ -122,7 +122,7 @@ void cpu_raise_softirq_batch_finish(void
ASSERT(per_cpu(batching, this_cpu));
for_each_cpu ( cpu, mask )
if ( !softirq_pending(cpu) )
- cpumask_clear_cpu(cpu, mask);
+ __cpumask_clr_cpu(cpu, mask);
smp_send_event_check_mask(mask);
cpumask_clear(mask);
--per_cpu(batching, this_cpu);
--- a/xen/include/xen/cpumask.h
+++ b/xen/include/xen/cpumask.h
@@ -103,11 +103,21 @@ static inline void cpumask_set_cpu(int c
set_bit(cpumask_check(cpu), dstp->bits);
}
+static inline void __cpumask_set_cpu(int cpu, cpumask_t *dstp)
+{
+ __set_bit(cpumask_check(cpu), dstp->bits);
+}
+
static inline void cpumask_clear_cpu(int cpu, volatile cpumask_t *dstp)
{
clear_bit(cpumask_check(cpu), dstp->bits);
}
+static inline void __cpumask_clr_cpu(int cpu, cpumask_t *dstp)
+{
+ __clear_bit(cpumask_check(cpu), dstp->bits);
+}
+
static inline void cpumask_setall(cpumask_t *dstp)
{
bitmap_fill(dstp->bits, nr_cpumask_bits);
@@ -122,16 +132,26 @@ static inline void cpumask_clear(cpumask
#define cpumask_test_cpu(cpu, cpumask) \
test_bit(cpumask_check(cpu), (cpumask)->bits)
-static inline int cpumask_test_and_set_cpu(int cpu, cpumask_t *addr)
+static inline int cpumask_test_and_set_cpu(int cpu, volatile cpumask_t *addr)
{
return test_and_set_bit(cpumask_check(cpu), addr->bits);
}
-static inline int cpumask_test_and_clear_cpu(int cpu, cpumask_t *addr)
+static inline int __cpumask_tst_set_cpu(int cpu, cpumask_t *addr)
+{
+ return __test_and_set_bit(cpumask_check(cpu), addr->bits);
+}
+
+static inline int cpumask_test_and_clear_cpu(int cpu, volatile cpumask_t *addr)
{
return test_and_clear_bit(cpumask_check(cpu), addr->bits);
}
+static inline int __cpumask_tst_clr_cpu(int cpu, cpumask_t *addr)
+{
+ return __test_and_clear_bit(cpumask_check(cpu), addr->bits);
+}
+
static inline void cpumask_and(cpumask_t *dstp, const cpumask_t *src1p,
const cpumask_t *src2p)
{
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] introduce and used relaxed cpumask operations
2015-01-19 15:58 [PATCH] introduce and used relaxed cpumask operations Jan Beulich
@ 2015-01-21 12:21 ` Andrew Cooper
2015-01-21 14:10 ` Jan Beulich
2015-01-21 14:28 ` George Dunlap
1 sibling, 1 reply; 9+ messages in thread
From: Andrew Cooper @ 2015-01-21 12:21 UTC (permalink / raw)
To: Jan Beulich, xen-devel
Cc: George Dunlap, Ian Jackson, Keir Fraser, Ian Campbell, Tim Deegan
[-- Attachment #1.1: Type: text/plain, Size: 5132 bytes --]
On 19/01/15 15:58, Jan Beulich wrote:
> --- a/xen/common/core_parking.c
> +++ b/xen/common/core_parking.c
> @@ -75,11 +75,10 @@ static unsigned int core_parking_perform
> if ( core_weight < core_tmp )
> {
> core_weight = core_tmp;
> - cpumask_clear(&core_candidate_map);
> - cpumask_set_cpu(cpu, &core_candidate_map);
> + cpumask_copy(&core_candidate_map, cpumask_of(cpu));
It is probably worth mentioning changes like this in the commit message,
as they are slightly more than just a simple removal of the lock prefix.
> }
> else if ( core_weight == core_tmp )
> - cpumask_set_cpu(cpu, &core_candidate_map);
> + __cpumask_set_cpu(cpu, &core_candidate_map);
> }
>
> for_each_cpu(cpu, &core_candidate_map)
> --- a/xen/common/sched_rt.c
> +++ b/xen/common/sched_rt.c
> @@ -663,7 +663,7 @@ burn_budget(const struct scheduler *ops,
> * lock is grabbed before calling this function
> */
> static struct rt_vcpu *
> -__runq_pick(const struct scheduler *ops, cpumask_t *mask)
> +__runq_pick(const struct scheduler *ops, const cpumask_t *mask)
> {
> struct list_head *runq = rt_runq(ops);
> struct list_head *iter;
> @@ -780,10 +780,7 @@ rt_schedule(const struct scheduler *ops,
> }
> else
> {
> - cpumask_t cur_cpu;
> - cpumask_clear(&cur_cpu);
> - cpumask_set_cpu(cpu, &cur_cpu);
> - snext = __runq_pick(ops, &cur_cpu);
> + snext = __runq_pick(ops, cpumask_of(cpu));
> if ( snext == NULL )
> snext = rt_vcpu(idle_vcpu[cpu]);
>
> --- a/xen/common/softirq.c
> +++ b/xen/common/softirq.c
> @@ -88,7 +88,7 @@ void cpumask_raise_softirq(const cpumask
> if ( !test_and_set_bit(nr, &softirq_pending(cpu)) &&
> cpu != this_cpu &&
> !arch_skip_send_event_check(cpu) )
> - cpumask_set_cpu(cpu, raise_mask);
> + __cpumask_set_cpu(cpu, raise_mask);
>
> if ( raise_mask == &send_mask )
> smp_send_event_check_mask(raise_mask);
> @@ -106,7 +106,7 @@ void cpu_raise_softirq(unsigned int cpu,
> if ( !per_cpu(batching, this_cpu) || in_irq() )
> smp_send_event_check_cpu(cpu);
> else
> - set_bit(nr, &per_cpu(batch_mask, this_cpu));
> + __cpumask_set_cpu(nr, &per_cpu(batch_mask, this_cpu));
> }
>
> void cpu_raise_softirq_batch_begin(void)
> @@ -122,7 +122,7 @@ void cpu_raise_softirq_batch_finish(void
> ASSERT(per_cpu(batching, this_cpu));
> for_each_cpu ( cpu, mask )
> if ( !softirq_pending(cpu) )
> - cpumask_clear_cpu(cpu, mask);
> + __cpumask_clr_cpu(cpu, mask);
> smp_send_event_check_mask(mask);
> cpumask_clear(mask);
> --per_cpu(batching, this_cpu);
> --- a/xen/include/xen/cpumask.h
> +++ b/xen/include/xen/cpumask.h
> @@ -103,11 +103,21 @@ static inline void cpumask_set_cpu(int c
> set_bit(cpumask_check(cpu), dstp->bits);
> }
>
> +static inline void __cpumask_set_cpu(int cpu, cpumask_t *dstp)
> +{
> + __set_bit(cpumask_check(cpu), dstp->bits);
> +}
> +
> static inline void cpumask_clear_cpu(int cpu, volatile cpumask_t *dstp)
> {
> clear_bit(cpumask_check(cpu), dstp->bits);
> }
>
> +static inline void __cpumask_clr_cpu(int cpu, cpumask_t *dstp)
> +{
> + __clear_bit(cpumask_check(cpu), dstp->bits);
> +}
> +
While I can appreciate the want for a shorter function name, I feel that
consistency with its locked alternative is more important.
> static inline void cpumask_setall(cpumask_t *dstp)
> {
> bitmap_fill(dstp->bits, nr_cpumask_bits);
> @@ -122,16 +132,26 @@ static inline void cpumask_clear(cpumask
> #define cpumask_test_cpu(cpu, cpumask) \
> test_bit(cpumask_check(cpu), (cpumask)->bits)
>
> -static inline int cpumask_test_and_set_cpu(int cpu, cpumask_t *addr)
> +static inline int cpumask_test_and_set_cpu(int cpu, volatile cpumask_t *addr)
> {
> return test_and_set_bit(cpumask_check(cpu), addr->bits);
> }
>
> -static inline int cpumask_test_and_clear_cpu(int cpu, cpumask_t *addr)
> +static inline int __cpumask_tst_set_cpu(int cpu, cpumask_t *addr)
> +{
> + return __test_and_set_bit(cpumask_check(cpu), addr->bits);
> +}
> +
> +static inline int cpumask_test_and_clear_cpu(int cpu, volatile cpumask_t *addr)
This introduction of volatile also need mentioning in the commit
message, but I would agree that it should be here.
~Andrew
> {
> return test_and_clear_bit(cpumask_check(cpu), addr->bits);
> }
>
> +static inline int __cpumask_tst_clr_cpu(int cpu, cpumask_t *addr)
> +{
> + return __test_and_clear_bit(cpumask_check(cpu), addr->bits);
> +}
> +
> static inline void cpumask_and(cpumask_t *dstp, const cpumask_t *src1p,
> const cpumask_t *src2p)
> {
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
[-- Attachment #1.2: Type: text/html, Size: 5965 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] introduce and used relaxed cpumask operations
2015-01-21 12:21 ` Andrew Cooper
@ 2015-01-21 14:10 ` Jan Beulich
2015-01-22 15:29 ` Tim Deegan
0 siblings, 1 reply; 9+ messages in thread
From: Jan Beulich @ 2015-01-21 14:10 UTC (permalink / raw)
To: Andrew Cooper
Cc: Keir Fraser, George Dunlap, Ian Jackson, Tim Deegan, IanCampbell,
xen-devel
>>> On 21.01.15 at 13:21, <andrew.cooper3@citrix.com> wrote:
> On 19/01/15 15:58, Jan Beulich wrote:
>> --- a/xen/common/core_parking.c
>> +++ b/xen/common/core_parking.c
>> @@ -75,11 +75,10 @@ static unsigned int core_parking_perform
>> if ( core_weight < core_tmp )
>> {
>> core_weight = core_tmp;
>> - cpumask_clear(&core_candidate_map);
>> - cpumask_set_cpu(cpu, &core_candidate_map);
>> + cpumask_copy(&core_candidate_map, cpumask_of(cpu));
>
> It is probably worth mentioning changes like this in the commit message,
> as they are slightly more than just a simple removal of the lock prefix.
Added.
>> +static inline void __cpumask_clr_cpu(int cpu, cpumask_t *dstp)
>> +{
>> + __clear_bit(cpumask_check(cpu), dstp->bits);
>> +}
>> +
>
> While I can appreciate the want for a shorter function name, I feel that
> consistency with its locked alternative is more important.
I sort of expected a comment to that effect, but decided to use the
shorter names nevertheless. Let's see what others, namely the REST
maintainers, say.
>> -static inline int cpumask_test_and_set_cpu(int cpu, cpumask_t *addr)
>> +static inline int cpumask_test_and_set_cpu(int cpu, volatile cpumask_t *addr)
>> {
>> return test_and_set_bit(cpumask_check(cpu), addr->bits);
>> }
>>
>> -static inline int cpumask_test_and_clear_cpu(int cpu, cpumask_t *addr)
>> +static inline int __cpumask_tst_set_cpu(int cpu, cpumask_t *addr)
>> +{
>> + return __test_and_set_bit(cpumask_check(cpu), addr->bits);
>> +}
>> +
>> +static inline int cpumask_test_and_clear_cpu(int cpu, volatile cpumask_t *addr)
>
> This introduction of volatile also need mentioning in the commit
> message, but I would agree that it should be here.
Done too.
Jan
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] introduce and used relaxed cpumask operations
2015-01-19 15:58 [PATCH] introduce and used relaxed cpumask operations Jan Beulich
2015-01-21 12:21 ` Andrew Cooper
@ 2015-01-21 14:28 ` George Dunlap
2015-01-21 14:35 ` Jan Beulich
1 sibling, 1 reply; 9+ messages in thread
From: George Dunlap @ 2015-01-21 14:28 UTC (permalink / raw)
To: Jan Beulich, xen-devel
Cc: Ian Campbell, Andrew Cooper, Keir Fraser, Ian Jackson, Tim Deegan
On 01/19/2015 03:58 PM, Jan Beulich wrote:
> Using atomic (LOCKed on x86) bitops for certain of the operations on
> cpumask_t is overkill when the variables aren't concurrently accessible
> (e.g. local function variables, or due to explicit locking). Introduce
> alternatives using non-atomic bitops and use them where appropriate.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
I'm wondering if it might be sensible to have more informative names for
these, that would make it easier for coders / reviewers to see what
aspect makes the cpumask suitable for the relaxed access; for instance,
"local_cpumask_set_cpu()" for local variables, and
"locked_cpumask_set_cpu()" for cpumasks which we know are locked. (Or
perhaps cpumask_set_cpu_local and cpumask_set_cpu_locked.)
At some point some clever person might even be able to write some
Coverity rules that check to make sure cpumask_set_cpu_local only
accesses local variables, &c.
> --- a/xen/common/sched_credit.c
> +++ b/xen/common/sched_credit.c
> @@ -372,7 +372,7 @@ __runq_tickle(unsigned int cpu, struct c
> {
> if ( cur->pri != CSCHED_PRI_IDLE )
> SCHED_STAT_CRANK(tickle_idlers_none);
> - cpumask_set_cpu(cpu, &mask);
> + __cpumask_set_cpu(cpu, &mask);
> }
> else if ( !idlers_empty )
> {
> @@ -422,7 +422,7 @@ __runq_tickle(unsigned int cpu, struct c
> SCHED_VCPU_STAT_CRANK(cur, migrate_r);
> SCHED_STAT_CRANK(migrate_kicked_away);
> set_bit(_VPF_migrating, &cur->vcpu->pause_flags);
> - cpumask_set_cpu(cpu, &mask);
> + __cpumask_set_cpu(cpu, &mask);
> }
> else if ( !new_idlers_empty )
> {
> @@ -432,7 +432,7 @@ __runq_tickle(unsigned int cpu, struct c
> {
> this_cpu(last_tickle_cpu) =
> cpumask_cycle(this_cpu(last_tickle_cpu), &idle_mask);
> - cpumask_set_cpu(this_cpu(last_tickle_cpu), &mask);
> + __cpumask_set_cpu(this_cpu(last_tickle_cpu), &mask);
> }
> else
> cpumask_or(&mask, &mask, &idle_mask);
> @@ -675,7 +675,7 @@ _csched_cpu_pick(const struct scheduler
> */
> cpumask_and(&idlers, &cpu_online_map, CSCHED_PRIV(ops)->idlers);
> if ( vc->processor == cpu && IS_RUNQ_IDLE(cpu) )
> - cpumask_set_cpu(cpu, &idlers);
> + __cpumask_set_cpu(cpu, &idlers);
> cpumask_and(&cpus, &cpus, &idlers);
>
> /*
> @@ -692,7 +692,7 @@ _csched_cpu_pick(const struct scheduler
> */
> if ( !cpumask_test_cpu(cpu, &cpus) && !cpumask_empty(&cpus) )
> cpu = cpumask_cycle(cpu, &cpus);
> - cpumask_clear_cpu(cpu, &cpus);
> + __cpumask_clr_cpu(cpu, &cpus);
>
> while ( !cpumask_empty(&cpus) )
> {
> @@ -1536,7 +1536,7 @@ csched_load_balance(struct csched_privat
> /* Find out what the !idle are in this node */
> cpumask_andnot(&workers, online, prv->idlers);
> cpumask_and(&workers, &workers, &node_to_cpumask(peer_node));
> - cpumask_clear_cpu(cpu, &workers);
> + __cpumask_clr_cpu(cpu, &workers);
>
> peer_cpu = cpumask_first(&workers);
> if ( peer_cpu >= nr_cpu_ids )
All these look good, but...
> --- a/xen/common/sched_rt.c
> +++ b/xen/common/sched_rt.c
> @@ -663,7 +663,7 @@ burn_budget(const struct scheduler *ops,
> * lock is grabbed before calling this function
> */
> static struct rt_vcpu *
> -__runq_pick(const struct scheduler *ops, cpumask_t *mask)
> +__runq_pick(const struct scheduler *ops, const cpumask_t *mask)
> {
> struct list_head *runq = rt_runq(ops);
> struct list_head *iter;
> @@ -780,10 +780,7 @@ rt_schedule(const struct scheduler *ops,
> }
> else
> {
> - cpumask_t cur_cpu;
> - cpumask_clear(&cur_cpu);
> - cpumask_set_cpu(cpu, &cur_cpu);
> - snext = __runq_pick(ops, &cur_cpu);
> + snext = __runq_pick(ops, cpumask_of(cpu));
> if ( snext == NULL )
> snext = rt_vcpu(idle_vcpu[cpu]);
>
This bit really needs explicit mention in the changelog.
-George
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] introduce and used relaxed cpumask operations
2015-01-21 14:28 ` George Dunlap
@ 2015-01-21 14:35 ` Jan Beulich
2015-01-21 14:42 ` George Dunlap
0 siblings, 1 reply; 9+ messages in thread
From: Jan Beulich @ 2015-01-21 14:35 UTC (permalink / raw)
To: George Dunlap
Cc: KeirFraser, Andrew Cooper, Tim Deegan, Ian Campbell, xen-devel,
Ian Jackson
>>> On 21.01.15 at 15:28, <george.dunlap@eu.citrix.com> wrote:
> On 01/19/2015 03:58 PM, Jan Beulich wrote:
>> Using atomic (LOCKed on x86) bitops for certain of the operations on
>> cpumask_t is overkill when the variables aren't concurrently accessible
>> (e.g. local function variables, or due to explicit locking). Introduce
>> alternatives using non-atomic bitops and use them where appropriate.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> I'm wondering if it might be sensible to have more informative names for
> these, that would make it easier for coders / reviewers to see what
> aspect makes the cpumask suitable for the relaxed access; for instance,
> "local_cpumask_set_cpu()" for local variables, and
> "locked_cpumask_set_cpu()" for cpumasks which we know are locked. (Or
> perhaps cpumask_set_cpu_local and cpumask_set_cpu_locked.)
Makes a lot of sense, except that it means even more typing.
>> @@ -780,10 +780,7 @@ rt_schedule(const struct scheduler *ops,
>> }
>> else
>> {
>> - cpumask_t cur_cpu;
>> - cpumask_clear(&cur_cpu);
>> - cpumask_set_cpu(cpu, &cur_cpu);
>> - snext = __runq_pick(ops, &cur_cpu);
>> + snext = __runq_pick(ops, cpumask_of(cpu));
>> if ( snext == NULL )
>> snext = rt_vcpu(idle_vcpu[cpu]);
>>
>
> This bit really needs explicit mention in the changelog.
Already done in response to Andrew's similar request.
Jan
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] introduce and used relaxed cpumask operations
2015-01-21 14:35 ` Jan Beulich
@ 2015-01-21 14:42 ` George Dunlap
2015-01-21 15:06 ` Jan Beulich
0 siblings, 1 reply; 9+ messages in thread
From: George Dunlap @ 2015-01-21 14:42 UTC (permalink / raw)
To: Jan Beulich
Cc: KeirFraser, Andrew Cooper, Tim Deegan, Ian Campbell, xen-devel,
Ian Jackson
On 01/21/2015 02:35 PM, Jan Beulich wrote:
>>>> On 21.01.15 at 15:28, <george.dunlap@eu.citrix.com> wrote:
>> On 01/19/2015 03:58 PM, Jan Beulich wrote:
>>> Using atomic (LOCKed on x86) bitops for certain of the operations on
>>> cpumask_t is overkill when the variables aren't concurrently accessible
>>> (e.g. local function variables, or due to explicit locking). Introduce
>>> alternatives using non-atomic bitops and use them where appropriate.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> I'm wondering if it might be sensible to have more informative names for
>> these, that would make it easier for coders / reviewers to see what
>> aspect makes the cpumask suitable for the relaxed access; for instance,
>> "local_cpumask_set_cpu()" for local variables, and
>> "locked_cpumask_set_cpu()" for cpumasks which we know are locked. (Or
>> perhaps cpumask_set_cpu_local and cpumask_set_cpu_locked.)
>
> Makes a lot of sense, except that it means even more typing.
Well perhaps we could save some typing by renaming it "_cmsksc()"
instead? :-)
The most expensive and annoying part of development is finding bugs in
code; the second is understanding code that was written a long time ago.
Investing a bit of extra typing being able to avoid these errors and
making things easier makes sense to me.
But, it's just a suggestion, and I'm not the one writing the patch, so I
wouldn't hold it up based on that.
>
>>> @@ -780,10 +780,7 @@ rt_schedule(const struct scheduler *ops,
>>> }
>>> else
>>> {
>>> - cpumask_t cur_cpu;
>>> - cpumask_clear(&cur_cpu);
>>> - cpumask_set_cpu(cpu, &cur_cpu);
>>> - snext = __runq_pick(ops, &cur_cpu);
>>> + snext = __runq_pick(ops, cpumask_of(cpu));
>>> if ( snext == NULL )
>>> snext = rt_vcpu(idle_vcpu[cpu]);
>>>
>>
>> This bit really needs explicit mention in the changelog.
>
> Already done in response to Andrew's similar request.
Ah, sorry -- I saw that but for some reason thought he was talking about
a different hunk.
As I said, I think having a more informative name would be better, but
I'll leave that up to you to decide.
With the changelog update, scheduler parts are:
Acked-by: George Dunlap <george.dunlap@eu.citrix.com>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] introduce and used relaxed cpumask operations
2015-01-21 14:42 ` George Dunlap
@ 2015-01-21 15:06 ` Jan Beulich
2015-01-21 15:10 ` George Dunlap
0 siblings, 1 reply; 9+ messages in thread
From: Jan Beulich @ 2015-01-21 15:06 UTC (permalink / raw)
To: George Dunlap
Cc: KeirFraser, Andrew Cooper, TimDeegan, Ian Campbell, xen-devel,
Ian Jackson
>>> On 21.01.15 at 15:42, <george.dunlap@eu.citrix.com> wrote:
> On 01/21/2015 02:35 PM, Jan Beulich wrote:
>>>>> On 21.01.15 at 15:28, <george.dunlap@eu.citrix.com> wrote:
>>> On 01/19/2015 03:58 PM, Jan Beulich wrote:
>>>> @@ -780,10 +780,7 @@ rt_schedule(const struct scheduler *ops,
>>>> }
>>>> else
>>>> {
>>>> - cpumask_t cur_cpu;
>>>> - cpumask_clear(&cur_cpu);
>>>> - cpumask_set_cpu(cpu, &cur_cpu);
>>>> - snext = __runq_pick(ops, &cur_cpu);
>>>> + snext = __runq_pick(ops, cpumask_of(cpu));
>>>> if ( snext == NULL )
>>>> snext = rt_vcpu(idle_vcpu[cpu]);
>>>>
>>>
>>> This bit really needs explicit mention in the changelog.
>>
>> Already done in response to Andrew's similar request.
>
> Ah, sorry -- I saw that but for some reason thought he was talking about
> a different hunk.
It was indeed, be the wording I added
Note that this
- adds a volatile qualifier to cpumask_test_and_{clear,set}_cpu()
(should have been there from the beginning, like is the case for
cpumask_{clear,set}_cpu())
- replaces several cpumask_clear()+cpumask_set_cpu(, n) pairs by the
simpler cpumask_copy(, cpumask_of(n)) (or just cpumask_of(n) if we
can do without copying)
isn't really specific to where these changes get done (as it's a
common pattern).
Jan
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] introduce and used relaxed cpumask operations
2015-01-21 15:06 ` Jan Beulich
@ 2015-01-21 15:10 ` George Dunlap
0 siblings, 0 replies; 9+ messages in thread
From: George Dunlap @ 2015-01-21 15:10 UTC (permalink / raw)
To: Jan Beulich
Cc: KeirFraser, Andrew Cooper, TimDeegan, Ian Campbell, xen-devel,
Ian Jackson
On 01/21/2015 03:06 PM, Jan Beulich wrote:
>>>> On 21.01.15 at 15:42, <george.dunlap@eu.citrix.com> wrote:
>> On 01/21/2015 02:35 PM, Jan Beulich wrote:
>>>>>> On 21.01.15 at 15:28, <george.dunlap@eu.citrix.com> wrote:
>>>> On 01/19/2015 03:58 PM, Jan Beulich wrote:
>>>>> @@ -780,10 +780,7 @@ rt_schedule(const struct scheduler *ops,
>>>>> }
>>>>> else
>>>>> {
>>>>> - cpumask_t cur_cpu;
>>>>> - cpumask_clear(&cur_cpu);
>>>>> - cpumask_set_cpu(cpu, &cur_cpu);
>>>>> - snext = __runq_pick(ops, &cur_cpu);
>>>>> + snext = __runq_pick(ops, cpumask_of(cpu));
>>>>> if ( snext == NULL )
>>>>> snext = rt_vcpu(idle_vcpu[cpu]);
>>>>>
>>>>
>>>> This bit really needs explicit mention in the changelog.
>>>
>>> Already done in response to Andrew's similar request.
>>
>> Ah, sorry -- I saw that but for some reason thought he was talking about
>> a different hunk.
>
> It was indeed, be the wording I added
>
> Note that this
> - adds a volatile qualifier to cpumask_test_and_{clear,set}_cpu()
> (should have been there from the beginning, like is the case for
> cpumask_{clear,set}_cpu())
> - replaces several cpumask_clear()+cpumask_set_cpu(, n) pairs by the
> simpler cpumask_copy(, cpumask_of(n)) (or just cpumask_of(n) if we
> can do without copying)
>
> isn't really specific to where these changes get done (as it's a
> common pattern).
Gotcha, thanks.
-George
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] introduce and used relaxed cpumask operations
2015-01-21 14:10 ` Jan Beulich
@ 2015-01-22 15:29 ` Tim Deegan
0 siblings, 0 replies; 9+ messages in thread
From: Tim Deegan @ 2015-01-22 15:29 UTC (permalink / raw)
To: Jan Beulich
Cc: Keir Fraser, George Dunlap, Andrew Cooper, Ian Jackson,
IanCampbell, xen-devel
At 14:10 +0000 on 21 Jan (1421845837), Jan Beulich wrote:
> >>> On 21.01.15 at 13:21, <andrew.cooper3@citrix.com> wrote:
> > On 19/01/15 15:58, Jan Beulich wrote:
> >> --- a/xen/common/core_parking.c
> >> +++ b/xen/common/core_parking.c
> >> @@ -75,11 +75,10 @@ static unsigned int core_parking_perform
> >> if ( core_weight < core_tmp )
> >> {
> >> core_weight = core_tmp;
> >> - cpumask_clear(&core_candidate_map);
> >> - cpumask_set_cpu(cpu, &core_candidate_map);
> >> + cpumask_copy(&core_candidate_map, cpumask_of(cpu));
> >
> > It is probably worth mentioning changes like this in the commit message,
> > as they are slightly more than just a simple removal of the lock prefix.
>
> Added.
>
> >> +static inline void __cpumask_clr_cpu(int cpu, cpumask_t *dstp)
> >> +{
> >> + __clear_bit(cpumask_check(cpu), dstp->bits);
> >> +}
> >> +
> >
> > While I can appreciate the want for a shorter function name, I feel that
> > consistency with its locked alternative is more important.
>
> I sort of expected a comment to that effect, but decided to use the
> shorter names nevertheless. Let's see what others, namely the REST
> maintainers, say.
FWIW, I prefer consistent naming (i.e. __cpumask_clear_cpu()).
Cheers,
Tim.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-01-22 15:29 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-01-19 15:58 [PATCH] introduce and used relaxed cpumask operations Jan Beulich
2015-01-21 12:21 ` Andrew Cooper
2015-01-21 14:10 ` Jan Beulich
2015-01-22 15:29 ` Tim Deegan
2015-01-21 14:28 ` George Dunlap
2015-01-21 14:35 ` Jan Beulich
2015-01-21 14:42 ` George Dunlap
2015-01-21 15:06 ` Jan Beulich
2015-01-21 15:10 ` George Dunlap
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.