* [PATCH 1/9] KVM: selftests: Add rdmsr_from_l2() implementation in Hyper-V eVMCS test
2022-11-19 1:34 [PATCH 0/9] tools: Make {clear,set}_bit() atomic for reals Sean Christopherson
@ 2022-11-19 1:34 ` Sean Christopherson
2022-11-19 1:34 ` [PATCH 2/9] KVM: selftests: Remove unused "vcpu" param to fix build error Sean Christopherson
` (8 subsequent siblings)
9 siblings, 0 replies; 13+ messages in thread
From: Sean Christopherson @ 2022-11-19 1:34 UTC (permalink / raw)
To: Yury Norov, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Marc Zyngier, Paolo Bonzini
Cc: Andy Shevchenko, Rasmus Villemoes, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, James Morse,
Alexandru Elisei, Suzuki K Poulose, Oliver Upton,
linux-perf-users, linux-kernel, linux-arm-kernel, kvmarm, kvmarm,
kvm, Sean Christopherson
Add rdmsr_from_l2() in hyperv_evmcs.c, it got left unintentionally omitted
when applying code review feeback on the fly. Intentionally duplicate
the code that's in hyperv_svm_test.c, the helper really should not exist
(L1 shouldn't clobber GPRs) and will hopefully be removed sooner than
later. Until that happens, deter other tests from using the somewhat
misleading helper (it doesn't actually read L2's MSR value).
Link: https://lore.kernel.org/all/Y2FFNO3Bu9Z3LtCW@google.com
Fixes: 860e80157068 ("KVM: selftests: Introduce rdmsr_from_l2() and use it for MSR-Bitmap tests")
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
tools/testing/selftests/kvm/x86_64/hyperv_evmcs.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/tools/testing/selftests/kvm/x86_64/hyperv_evmcs.c b/tools/testing/selftests/kvm/x86_64/hyperv_evmcs.c
index d418954590b1..ea58e5b436e8 100644
--- a/tools/testing/selftests/kvm/x86_64/hyperv_evmcs.c
+++ b/tools/testing/selftests/kvm/x86_64/hyperv_evmcs.c
@@ -31,6 +31,15 @@ static void guest_nmi_handler(struct ex_regs *regs)
{
}
+/* Exit to L1 from L2 with RDMSR instruction */
+static inline void rdmsr_from_l2(uint32_t msr)
+{
+ /* Currently, L1 doesn't preserve GPRs during vmexits. */
+ __asm__ __volatile__ ("rdmsr" : : "c"(msr) :
+ "rax", "rbx", "rdx", "rsi", "rdi", "r8", "r9",
+ "r10", "r11", "r12", "r13", "r14", "r15");
+}
+
void l2_guest_code(void)
{
u64 unused;
--
2.38.1.584.g0f3c55d4c2-goog
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 2/9] KVM: selftests: Remove unused "vcpu" param to fix build error
2022-11-19 1:34 [PATCH 0/9] tools: Make {clear,set}_bit() atomic for reals Sean Christopherson
2022-11-19 1:34 ` [PATCH 1/9] KVM: selftests: Add rdmsr_from_l2() implementation in Hyper-V eVMCS test Sean Christopherson
@ 2022-11-19 1:34 ` Sean Christopherson
2022-11-19 1:34 ` [PATCH 3/9] KVM: arm64: selftests: Enable single-step without a "full" ucall() Sean Christopherson
` (7 subsequent siblings)
9 siblings, 0 replies; 13+ messages in thread
From: Sean Christopherson @ 2022-11-19 1:34 UTC (permalink / raw)
To: Yury Norov, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Marc Zyngier, Paolo Bonzini
Cc: Andy Shevchenko, Rasmus Villemoes, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, James Morse,
Alexandru Elisei, Suzuki K Poulose, Oliver Upton,
linux-perf-users, linux-kernel, linux-arm-kernel, kvmarm, kvmarm,
kvm, Sean Christopherson
Drop the vcpu param in the Hyper-V TLB flush test's call to
vm_get_page_table_entry(). Commit 751f280017b6 ("KVM: selftests: Drop
reserved bit checks from PTE accessor") eliminated the param, but the
in-flight patch to add the Hyper-V test didn't get the memo.
Fixes: a105ac64bef6 ("KVM: selftests: Hyper-V PV TLB flush selftest")
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
tools/testing/selftests/kvm/x86_64/hyperv_tlb_flush.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/kvm/x86_64/hyperv_tlb_flush.c b/tools/testing/selftests/kvm/x86_64/hyperv_tlb_flush.c
index 44525255f5c8..68f97ff720a7 100644
--- a/tools/testing/selftests/kvm/x86_64/hyperv_tlb_flush.c
+++ b/tools/testing/selftests/kvm/x86_64/hyperv_tlb_flush.c
@@ -625,7 +625,7 @@ int main(int argc, char *argv[])
*/
gva = vm_vaddr_unused_gap(vm, NTEST_PAGES * PAGE_SIZE, KVM_UTIL_MIN_VADDR);
for (i = 0; i < NTEST_PAGES; i++) {
- pte = vm_get_page_table_entry(vm, vcpu[0], data->test_pages + i * PAGE_SIZE);
+ pte = vm_get_page_table_entry(vm, data->test_pages + i * PAGE_SIZE);
gpa = addr_hva2gpa(vm, pte);
__virt_pg_map(vm, gva + PAGE_SIZE * i, gpa & PAGE_MASK, PG_LEVEL_4K);
data->test_pages_pte[i] = gva + (gpa & ~PAGE_MASK);
--
2.38.1.584.g0f3c55d4c2-goog
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 3/9] KVM: arm64: selftests: Enable single-step without a "full" ucall()
2022-11-19 1:34 [PATCH 0/9] tools: Make {clear,set}_bit() atomic for reals Sean Christopherson
2022-11-19 1:34 ` [PATCH 1/9] KVM: selftests: Add rdmsr_from_l2() implementation in Hyper-V eVMCS test Sean Christopherson
2022-11-19 1:34 ` [PATCH 2/9] KVM: selftests: Remove unused "vcpu" param to fix build error Sean Christopherson
@ 2022-11-19 1:34 ` Sean Christopherson
2022-11-19 1:34 ` [PATCH 4/9] tools: Take @bit as an "unsigned long" in {clear,set}_bit() helpers Sean Christopherson
` (6 subsequent siblings)
9 siblings, 0 replies; 13+ messages in thread
From: Sean Christopherson @ 2022-11-19 1:34 UTC (permalink / raw)
To: Yury Norov, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Marc Zyngier, Paolo Bonzini
Cc: Andy Shevchenko, Rasmus Villemoes, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, James Morse,
Alexandru Elisei, Suzuki K Poulose, Oliver Upton,
linux-perf-users, linux-kernel, linux-arm-kernel, kvmarm, kvmarm,
kvm, Sean Christopherson
Add a new ucall hook, GUEST_UCALL_NONE(), to allow tests to make ucalls
without allocating a ucall struct, and use it to enable single-step
in ARM's debug-exceptions test. Like the disable single-step path, the
enabling path also needs to ensure that no exclusive access sequences are
attempted after enabling single-step, as the exclusive monitor is cleared
on ERET from the debug exception taken to EL2.
The test currently "works" because clear_bit() isn't actually an atomic
operation... yet.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
.../selftests/kvm/aarch64/debug-exceptions.c | 21 ++++++++++---------
.../selftests/kvm/include/ucall_common.h | 8 +++++++
2 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
index d86c4e4d1c82..c62ec4d7f6a3 100644
--- a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
+++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
@@ -239,10 +239,6 @@ static void guest_svc_handler(struct ex_regs *regs)
svc_addr = regs->pc;
}
-enum single_step_op {
- SINGLE_STEP_ENABLE = 0,
-};
-
static void guest_code_ss(int test_cnt)
{
uint64_t i;
@@ -253,8 +249,16 @@ static void guest_code_ss(int test_cnt)
w_bvr = i << 2;
w_wvr = i << 2;
- /* Enable Single Step execution */
- GUEST_SYNC(SINGLE_STEP_ENABLE);
+ /*
+ * Enable Single Step execution. Note! This _must_ be a bare
+ * ucall as the ucall() path uses atomic operations to manage
+ * the ucall structures, and the built-in "atomics" are usually
+ * implemented via exclusive access instructions. The exlusive
+ * monitor is cleared on ERET, and so taking debug exceptions
+ * during a LDREX=>STREX sequence will prevent forward progress
+ * and hang the guest/test.
+ */
+ GUEST_UCALL_NONE();
/*
* The userspace will verify that the pc is as expected during
@@ -356,12 +360,9 @@ void test_single_step_from_userspace(int test_cnt)
break;
}
- TEST_ASSERT(cmd == UCALL_SYNC,
+ TEST_ASSERT(cmd == UCALL_NONE,
"Unexpected ucall cmd 0x%lx", cmd);
- TEST_ASSERT(uc.args[1] == SINGLE_STEP_ENABLE,
- "Unexpected ucall action 0x%lx", uc.args[1]);
-
debug.control = KVM_GUESTDBG_ENABLE |
KVM_GUESTDBG_SINGLESTEP;
ss_enable = true;
diff --git a/tools/testing/selftests/kvm/include/ucall_common.h b/tools/testing/selftests/kvm/include/ucall_common.h
index bdd373189a77..1a6aaef5ccae 100644
--- a/tools/testing/selftests/kvm/include/ucall_common.h
+++ b/tools/testing/selftests/kvm/include/ucall_common.h
@@ -35,6 +35,14 @@ void ucall(uint64_t cmd, int nargs, ...);
uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc);
void ucall_init(struct kvm_vm *vm, vm_paddr_t mmio_gpa);
+/*
+ * Perform userspace call without any associated data. This bare call avoids
+ * allocating a ucall struct, which can be useful if the atomic operations in
+ * the full ucall() are problematic and/or unwanted. Note, this will come out
+ * as UCALL_NONE on the backend.
+ */
+#define GUEST_UCALL_NONE() ucall_arch_do_ucall((vm_vaddr_t)NULL)
+
#define GUEST_SYNC_ARGS(stage, arg1, arg2, arg3, arg4) \
ucall(UCALL_SYNC, 6, "hello", stage, arg1, arg2, arg3, arg4)
#define GUEST_SYNC(stage) ucall(UCALL_SYNC, 2, "hello", stage)
--
2.38.1.584.g0f3c55d4c2-goog
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 4/9] tools: Take @bit as an "unsigned long" in {clear,set}_bit() helpers
2022-11-19 1:34 [PATCH 0/9] tools: Make {clear,set}_bit() atomic for reals Sean Christopherson
` (2 preceding siblings ...)
2022-11-19 1:34 ` [PATCH 3/9] KVM: arm64: selftests: Enable single-step without a "full" ucall() Sean Christopherson
@ 2022-11-19 1:34 ` Sean Christopherson
2022-11-19 1:34 ` [PATCH 5/9] perf tools: Use dedicated non-atomic clear/set bit helpers Sean Christopherson
` (5 subsequent siblings)
9 siblings, 0 replies; 13+ messages in thread
From: Sean Christopherson @ 2022-11-19 1:34 UTC (permalink / raw)
To: Yury Norov, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Marc Zyngier, Paolo Bonzini
Cc: Andy Shevchenko, Rasmus Villemoes, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, James Morse,
Alexandru Elisei, Suzuki K Poulose, Oliver Upton,
linux-perf-users, linux-kernel, linux-arm-kernel, kvmarm, kvmarm,
kvm, Sean Christopherson
Take @bit as an unsigned long instead of a signed int in clear_bit() and
set_bit() so that they match the double-underscore versions, __clear_bit()
and __set_bit(). This will allow converting users that really don't want
atomic operations to the double-underscores without introducing a
functional change, which will in turn allow making {clear,set}_bit()
atomic (as advertised).
Practically speaking, this _should_ have no functional impact. KVM's
selftests usage is either hardcoded (Hyper-V tests) or is artificially
limited (arch_timer test and dirty_log test). In KVM, dirty_log test is
the only mildly interesting case as it's use indirectly restricted to
unsigned 32-bit values, but in theory it could generate a negative value
when cast to a signed int. But in that case, taking an "unsigned long"
is actually a bug fix.
Perf's usage is more difficult to audit, but any code that is affected
by the switch is likely already broken. perf_header__{set,clear}_feat()
and perf_file_header__read() effectively use only hardcoded enums with
small, positive values, atom_new() passes an unsigned long, but its value
is capped at 128 via NR_ATOM_PER_PAGE, etc...
The only real potential for breakage is in the perf flows that take a
"cpu", but it's unlikely perf is subtly relying on a negative index into
bitmaps, e.g. "cpu" can be "-1", but only as "not valid" placeholder.
Note, tools/testing/nvdimm/ makes heavy use of set_bit(), but that code
builds into a kernel module of sorts, i.e. pulls in all of the kernel's
header and so is getting the kernel's atomic set_bit(). The NVDIMM test
usage of atomics is likely unnecessary, e.g. ndtest_dimm_register() sets
bits in a local variable, but that's neither here nor there as far as
this change is concerned.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
tools/include/asm-generic/bitops/atomic.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/include/asm-generic/bitops/atomic.h b/tools/include/asm-generic/bitops/atomic.h
index 2f6ea28764a7..f64b049d236c 100644
--- a/tools/include/asm-generic/bitops/atomic.h
+++ b/tools/include/asm-generic/bitops/atomic.h
@@ -5,12 +5,12 @@
#include <asm/types.h>
#include <asm/bitsperlong.h>
-static inline void set_bit(int nr, unsigned long *addr)
+static inline void set_bit(unsigned long nr, unsigned long *addr)
{
addr[nr / __BITS_PER_LONG] |= 1UL << (nr % __BITS_PER_LONG);
}
-static inline void clear_bit(int nr, unsigned long *addr)
+static inline void clear_bit(unsigned long nr, unsigned long *addr)
{
addr[nr / __BITS_PER_LONG] &= ~(1UL << (nr % __BITS_PER_LONG));
}
--
2.38.1.584.g0f3c55d4c2-goog
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 5/9] perf tools: Use dedicated non-atomic clear/set bit helpers
2022-11-19 1:34 [PATCH 0/9] tools: Make {clear,set}_bit() atomic for reals Sean Christopherson
` (3 preceding siblings ...)
2022-11-19 1:34 ` [PATCH 4/9] tools: Take @bit as an "unsigned long" in {clear,set}_bit() helpers Sean Christopherson
@ 2022-11-19 1:34 ` Sean Christopherson
2022-12-02 18:16 ` Namhyung Kim
2022-11-19 1:34 ` [PATCH 6/9] KVM: selftests: Use non-atomic clear/set bit helpers in KVM tests Sean Christopherson
` (4 subsequent siblings)
9 siblings, 1 reply; 13+ messages in thread
From: Sean Christopherson @ 2022-11-19 1:34 UTC (permalink / raw)
To: Yury Norov, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Marc Zyngier, Paolo Bonzini
Cc: Andy Shevchenko, Rasmus Villemoes, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, James Morse,
Alexandru Elisei, Suzuki K Poulose, Oliver Upton,
linux-perf-users, linux-kernel, linux-arm-kernel, kvmarm, kvmarm,
kvm, Sean Christopherson
Use the dedicated non-atomic helpers for {clear,set}_bit() and their
test variants, i.e. the double-underscore versions. Depsite being
defined in atomic.h, and despite the kernel versions being atomic in the
kernel, tools' {clear,set}_bit() helpers aren't actually atomic. Move
to the double-underscore versions so that the versions that are expected
to be atomic (for kernel developers) can be made atomic without affecting
users that don't want atomic operations.
No functional change intended.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
tools/perf/bench/find-bit-bench.c | 2 +-
tools/perf/builtin-c2c.c | 6 +++---
tools/perf/builtin-kwork.c | 6 +++---
tools/perf/builtin-record.c | 6 +++---
tools/perf/builtin-sched.c | 2 +-
tools/perf/tests/bitmap.c | 2 +-
tools/perf/tests/mem2node.c | 2 +-
tools/perf/util/affinity.c | 4 ++--
| 8 ++++----
tools/perf/util/mmap.c | 6 +++---
tools/perf/util/pmu.c | 2 +-
tools/perf/util/scripting-engines/trace-event-perl.c | 2 +-
tools/perf/util/scripting-engines/trace-event-python.c | 2 +-
tools/perf/util/session.c | 2 +-
tools/perf/util/svghelper.c | 2 +-
15 files changed, 27 insertions(+), 27 deletions(-)
diff --git a/tools/perf/bench/find-bit-bench.c b/tools/perf/bench/find-bit-bench.c
index 22b5cfe97023..d103c3136983 100644
--- a/tools/perf/bench/find-bit-bench.c
+++ b/tools/perf/bench/find-bit-bench.c
@@ -70,7 +70,7 @@ static int do_for_each_set_bit(unsigned int num_bits)
bitmap_zero(to_test, num_bits);
skip = num_bits / set_bits;
for (i = 0; i < num_bits; i += skip)
- set_bit(i, to_test);
+ __set_bit(i, to_test);
for (i = 0; i < outer_iterations; i++) {
old = accumulator;
diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
index a9190458d2d5..52d94c7dd836 100644
--- a/tools/perf/builtin-c2c.c
+++ b/tools/perf/builtin-c2c.c
@@ -230,7 +230,7 @@ static void c2c_he__set_cpu(struct c2c_hist_entry *c2c_he,
"WARNING: no sample cpu value"))
return;
- set_bit(sample->cpu, c2c_he->cpuset);
+ __set_bit(sample->cpu, c2c_he->cpuset);
}
static void c2c_he__set_node(struct c2c_hist_entry *c2c_he,
@@ -247,7 +247,7 @@ static void c2c_he__set_node(struct c2c_hist_entry *c2c_he,
if (WARN_ONCE(node < 0, "WARNING: failed to find node\n"))
return;
- set_bit(node, c2c_he->nodeset);
+ __set_bit(node, c2c_he->nodeset);
if (c2c_he->paddr != sample->phys_addr) {
c2c_he->paddr_cnt++;
@@ -2318,7 +2318,7 @@ static int setup_nodes(struct perf_session *session)
continue;
perf_cpu_map__for_each_cpu(cpu, idx, map) {
- set_bit(cpu.cpu, set);
+ __set_bit(cpu.cpu, set);
if (WARN_ONCE(cpu2node[cpu.cpu] != -1, "node/cpu topology bug"))
return -EINVAL;
diff --git a/tools/perf/builtin-kwork.c b/tools/perf/builtin-kwork.c
index fb8c63656ad8..1f63e24f704e 100644
--- a/tools/perf/builtin-kwork.c
+++ b/tools/perf/builtin-kwork.c
@@ -216,7 +216,7 @@ static struct kwork_atom *atom_new(struct perf_kwork *kwork,
list_add_tail(&page->list, &kwork->atom_page_list);
found_atom:
- set_bit(i, page->bitmap);
+ __set_bit(i, page->bitmap);
atom->time = sample->time;
atom->prev = NULL;
atom->page_addr = page;
@@ -229,8 +229,8 @@ static void atom_free(struct kwork_atom *atom)
if (atom->prev != NULL)
atom_free(atom->prev);
- clear_bit(atom->bit_inpage,
- ((struct kwork_atom_page *)atom->page_addr)->bitmap);
+ __clear_bit(atom->bit_inpage,
+ ((struct kwork_atom_page *)atom->page_addr)->bitmap);
}
static void atom_del(struct kwork_atom *atom)
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index e128b855ddde..2711c141c5bf 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -3555,7 +3555,7 @@ static int record__mmap_cpu_mask_init(struct mmap_cpu_mask *mask, struct perf_cp
/* Return ENODEV is input cpu is greater than max cpu */
if ((unsigned long)cpu.cpu > mask->nbits)
return -ENODEV;
- set_bit(cpu.cpu, mask->bits);
+ __set_bit(cpu.cpu, mask->bits);
}
return 0;
@@ -3627,8 +3627,8 @@ static int record__init_thread_cpu_masks(struct record *rec, struct perf_cpu_map
pr_debug("nr_threads: %d\n", rec->nr_threads);
for (t = 0; t < rec->nr_threads; t++) {
- set_bit(perf_cpu_map__cpu(cpus, t).cpu, rec->thread_masks[t].maps.bits);
- set_bit(perf_cpu_map__cpu(cpus, t).cpu, rec->thread_masks[t].affinity.bits);
+ __set_bit(perf_cpu_map__cpu(cpus, t).cpu, rec->thread_masks[t].maps.bits);
+ __set_bit(perf_cpu_map__cpu(cpus, t).cpu, rec->thread_masks[t].affinity.bits);
if (verbose) {
pr_debug("thread_masks[%d]: ", t);
mmap_cpu_mask__scnprintf(&rec->thread_masks[t].maps, "maps");
diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index f93737eef07b..86e18575c9be 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -1573,7 +1573,7 @@ static int map_switch_event(struct perf_sched *sched, struct evsel *evsel,
if (sched->map.comp) {
cpus_nr = bitmap_weight(sched->map.comp_cpus_mask, MAX_CPUS);
- if (!test_and_set_bit(this_cpu.cpu, sched->map.comp_cpus_mask)) {
+ if (!__test_and_set_bit(this_cpu.cpu, sched->map.comp_cpus_mask)) {
sched->map.comp_cpus[cpus_nr++] = this_cpu;
new_cpu = true;
}
diff --git a/tools/perf/tests/bitmap.c b/tools/perf/tests/bitmap.c
index 4965dd666956..0173f5402a35 100644
--- a/tools/perf/tests/bitmap.c
+++ b/tools/perf/tests/bitmap.c
@@ -18,7 +18,7 @@ static unsigned long *get_bitmap(const char *str, int nbits)
if (map && bm) {
for (i = 0; i < perf_cpu_map__nr(map); i++)
- set_bit(perf_cpu_map__cpu(map, i).cpu, bm);
+ __set_bit(perf_cpu_map__cpu(map, i).cpu, bm);
}
if (map)
diff --git a/tools/perf/tests/mem2node.c b/tools/perf/tests/mem2node.c
index 4c96829510c9..a0e88c496107 100644
--- a/tools/perf/tests/mem2node.c
+++ b/tools/perf/tests/mem2node.c
@@ -33,7 +33,7 @@ static unsigned long *get_bitmap(const char *str, int nbits)
int i;
perf_cpu_map__for_each_cpu(cpu, i, map)
- set_bit(cpu.cpu, bm);
+ __set_bit(cpu.cpu, bm);
}
if (map)
diff --git a/tools/perf/util/affinity.c b/tools/perf/util/affinity.c
index 4ee96b3c755b..38dc4524b7e8 100644
--- a/tools/perf/util/affinity.c
+++ b/tools/perf/util/affinity.c
@@ -58,14 +58,14 @@ void affinity__set(struct affinity *a, int cpu)
return;
a->changed = true;
- set_bit(cpu, a->sched_cpus);
+ __set_bit(cpu, a->sched_cpus);
/*
* We ignore errors because affinity is just an optimization.
* This could happen for example with isolated CPUs or cpusets.
* In this case the IPIs inside the kernel's perf API still work.
*/
sched_setaffinity(0, cpu_set_size, (cpu_set_t *)a->sched_cpus);
- clear_bit(cpu, a->sched_cpus);
+ __clear_bit(cpu, a->sched_cpus);
}
static void __affinity__cleanup(struct affinity *a)
--git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 98dfaf84bd13..dc2ae397d400 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -79,12 +79,12 @@ struct perf_file_attr {
void perf_header__set_feat(struct perf_header *header, int feat)
{
- set_bit(feat, header->adds_features);
+ __set_bit(feat, header->adds_features);
}
void perf_header__clear_feat(struct perf_header *header, int feat)
{
- clear_bit(feat, header->adds_features);
+ __clear_bit(feat, header->adds_features);
}
bool perf_header__has_feat(const struct perf_header *header, int feat)
@@ -1358,7 +1358,7 @@ static int memory_node__read(struct memory_node *n, unsigned long idx)
rewinddir(dir);
for_each_memory(phys, dir) {
- set_bit(phys, n->set);
+ __set_bit(phys, n->set);
}
closedir(dir);
@@ -3952,7 +3952,7 @@ int perf_file_header__read(struct perf_file_header *header,
if (!test_bit(HEADER_HOSTNAME, header->adds_features)) {
bitmap_zero(header->adds_features, HEADER_FEAT_BITS);
- set_bit(HEADER_BUILD_ID, header->adds_features);
+ __set_bit(HEADER_BUILD_ID, header->adds_features);
}
}
diff --git a/tools/perf/util/mmap.c b/tools/perf/util/mmap.c
index a4dff881be39..49093b21ee2d 100644
--- a/tools/perf/util/mmap.c
+++ b/tools/perf/util/mmap.c
@@ -111,7 +111,7 @@ static int perf_mmap__aio_bind(struct mmap *map, int idx, struct perf_cpu cpu, i
pr_err("Failed to allocate node mask for mbind: error %m\n");
return -1;
}
- set_bit(node_index, node_mask);
+ __set_bit(node_index, node_mask);
if (mbind(data, mmap_len, MPOL_BIND, node_mask, node_index + 1 + 1, 0)) {
pr_err("Failed to bind [%p-%p] AIO buffer to node %lu: error %m\n",
data, data + mmap_len, node_index);
@@ -256,7 +256,7 @@ static void build_node_mask(int node, struct mmap_cpu_mask *mask)
for (idx = 0; idx < nr_cpus; idx++) {
cpu = perf_cpu_map__cpu(cpu_map, idx); /* map c index to online cpu index */
if (cpu__get_node(cpu) == node)
- set_bit(cpu.cpu, mask->bits);
+ __set_bit(cpu.cpu, mask->bits);
}
}
@@ -270,7 +270,7 @@ static int perf_mmap__setup_affinity_mask(struct mmap *map, struct mmap_params *
if (mp->affinity == PERF_AFFINITY_NODE && cpu__max_node() > 1)
build_node_mask(cpu__get_node(map->core.cpu), &map->affinity_mask);
else if (mp->affinity == PERF_AFFINITY_CPU)
- set_bit(map->core.cpu.cpu, map->affinity_mask.bits);
+ __set_bit(map->core.cpu.cpu, map->affinity_mask.bits);
return 0;
}
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 03284059175f..371d8f7a3de3 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -1513,7 +1513,7 @@ void perf_pmu__set_format(unsigned long *bits, long from, long to)
memset(bits, 0, BITS_TO_BYTES(PERF_PMU_FORMAT_BITS));
for (b = from; b <= to; b++)
- set_bit(b, bits);
+ __set_bit(b, bits);
}
void perf_pmu__del_formats(struct list_head *formats)
diff --git a/tools/perf/util/scripting-engines/trace-event-perl.c b/tools/perf/util/scripting-engines/trace-event-perl.c
index a5d945415bbc..5b602b6d4685 100644
--- a/tools/perf/util/scripting-engines/trace-event-perl.c
+++ b/tools/perf/util/scripting-engines/trace-event-perl.c
@@ -365,7 +365,7 @@ static void perl_process_tracepoint(struct perf_sample *sample,
sprintf(handler, "%s::%s", event->system, event->name);
- if (!test_and_set_bit(event->id, events_defined))
+ if (!__test_and_set_bit(event->id, events_defined))
define_event_symbols(event, handler, event->print_fmt.args);
s = nsecs / NSEC_PER_SEC;
diff --git a/tools/perf/util/scripting-engines/trace-event-python.c b/tools/perf/util/scripting-engines/trace-event-python.c
index 1f2040f36d4e..0f229fa29163 100644
--- a/tools/perf/util/scripting-engines/trace-event-python.c
+++ b/tools/perf/util/scripting-engines/trace-event-python.c
@@ -933,7 +933,7 @@ static void python_process_tracepoint(struct perf_sample *sample,
sprintf(handler_name, "%s__%s", event->system, event->name);
- if (!test_and_set_bit(event->id, events_defined))
+ if (!__test_and_set_bit(event->id, events_defined))
define_event_symbols(event, handler_name, event->print_fmt.args);
handler = get_handler(handler_name);
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 1a4f10de29ff..873fd51ec1b2 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -2748,7 +2748,7 @@ int perf_session__cpu_bitmap(struct perf_session *session,
goto out_delete_map;
}
- set_bit(cpu.cpu, cpu_bitmap);
+ __set_bit(cpu.cpu, cpu_bitmap);
}
err = 0;
diff --git a/tools/perf/util/svghelper.c b/tools/perf/util/svghelper.c
index 1e0c731fc539..5c62d3118c41 100644
--- a/tools/perf/util/svghelper.c
+++ b/tools/perf/util/svghelper.c
@@ -741,7 +741,7 @@ static int str_to_bitmap(char *s, cpumask_t *b, int nr_cpus)
break;
}
- set_bit(c.cpu, cpumask_bits(b));
+ __set_bit(c.cpu, cpumask_bits(b));
}
perf_cpu_map__put(m);
--
2.38.1.584.g0f3c55d4c2-goog
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 5/9] perf tools: Use dedicated non-atomic clear/set bit helpers
2022-11-19 1:34 ` [PATCH 5/9] perf tools: Use dedicated non-atomic clear/set bit helpers Sean Christopherson
@ 2022-12-02 18:16 ` Namhyung Kim
2022-12-02 18:27 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 13+ messages in thread
From: Namhyung Kim @ 2022-12-02 18:16 UTC (permalink / raw)
To: Sean Christopherson
Cc: Yury Norov, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Marc Zyngier, Paolo Bonzini, Andy Shevchenko, Rasmus Villemoes,
Mark Rutland, Alexander Shishkin, Jiri Olsa, James Morse,
Alexandru Elisei, Suzuki K Poulose, Oliver Upton,
linux-perf-users, linux-kernel, linux-arm-kernel, kvmarm, kvmarm,
kvm
Hello,
On Fri, Nov 18, 2022 at 5:35 PM Sean Christopherson <seanjc@google.com> wrote:
>
> Use the dedicated non-atomic helpers for {clear,set}_bit() and their
> test variants, i.e. the double-underscore versions. Depsite being
> defined in atomic.h, and despite the kernel versions being atomic in the
> kernel, tools' {clear,set}_bit() helpers aren't actually atomic. Move
> to the double-underscore versions so that the versions that are expected
> to be atomic (for kernel developers) can be made atomic without affecting
> users that don't want atomic operations.
>
> No functional change intended.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
Acked-by: Namhyung Kim <namhyung@kernel.org>
Thanks,
Namhyung
> ---
> tools/perf/bench/find-bit-bench.c | 2 +-
> tools/perf/builtin-c2c.c | 6 +++---
> tools/perf/builtin-kwork.c | 6 +++---
> tools/perf/builtin-record.c | 6 +++---
> tools/perf/builtin-sched.c | 2 +-
> tools/perf/tests/bitmap.c | 2 +-
> tools/perf/tests/mem2node.c | 2 +-
> tools/perf/util/affinity.c | 4 ++--
> tools/perf/util/header.c | 8 ++++----
> tools/perf/util/mmap.c | 6 +++---
> tools/perf/util/pmu.c | 2 +-
> tools/perf/util/scripting-engines/trace-event-perl.c | 2 +-
> tools/perf/util/scripting-engines/trace-event-python.c | 2 +-
> tools/perf/util/session.c | 2 +-
> tools/perf/util/svghelper.c | 2 +-
> 15 files changed, 27 insertions(+), 27 deletions(-)
>
> diff --git a/tools/perf/bench/find-bit-bench.c b/tools/perf/bench/find-bit-bench.c
> index 22b5cfe97023..d103c3136983 100644
> --- a/tools/perf/bench/find-bit-bench.c
> +++ b/tools/perf/bench/find-bit-bench.c
> @@ -70,7 +70,7 @@ static int do_for_each_set_bit(unsigned int num_bits)
> bitmap_zero(to_test, num_bits);
> skip = num_bits / set_bits;
> for (i = 0; i < num_bits; i += skip)
> - set_bit(i, to_test);
> + __set_bit(i, to_test);
>
> for (i = 0; i < outer_iterations; i++) {
> old = accumulator;
> diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> index a9190458d2d5..52d94c7dd836 100644
> --- a/tools/perf/builtin-c2c.c
> +++ b/tools/perf/builtin-c2c.c
> @@ -230,7 +230,7 @@ static void c2c_he__set_cpu(struct c2c_hist_entry *c2c_he,
> "WARNING: no sample cpu value"))
> return;
>
> - set_bit(sample->cpu, c2c_he->cpuset);
> + __set_bit(sample->cpu, c2c_he->cpuset);
> }
>
> static void c2c_he__set_node(struct c2c_hist_entry *c2c_he,
> @@ -247,7 +247,7 @@ static void c2c_he__set_node(struct c2c_hist_entry *c2c_he,
> if (WARN_ONCE(node < 0, "WARNING: failed to find node\n"))
> return;
>
> - set_bit(node, c2c_he->nodeset);
> + __set_bit(node, c2c_he->nodeset);
>
> if (c2c_he->paddr != sample->phys_addr) {
> c2c_he->paddr_cnt++;
> @@ -2318,7 +2318,7 @@ static int setup_nodes(struct perf_session *session)
> continue;
>
> perf_cpu_map__for_each_cpu(cpu, idx, map) {
> - set_bit(cpu.cpu, set);
> + __set_bit(cpu.cpu, set);
>
> if (WARN_ONCE(cpu2node[cpu.cpu] != -1, "node/cpu topology bug"))
> return -EINVAL;
> diff --git a/tools/perf/builtin-kwork.c b/tools/perf/builtin-kwork.c
> index fb8c63656ad8..1f63e24f704e 100644
> --- a/tools/perf/builtin-kwork.c
> +++ b/tools/perf/builtin-kwork.c
> @@ -216,7 +216,7 @@ static struct kwork_atom *atom_new(struct perf_kwork *kwork,
> list_add_tail(&page->list, &kwork->atom_page_list);
>
> found_atom:
> - set_bit(i, page->bitmap);
> + __set_bit(i, page->bitmap);
> atom->time = sample->time;
> atom->prev = NULL;
> atom->page_addr = page;
> @@ -229,8 +229,8 @@ static void atom_free(struct kwork_atom *atom)
> if (atom->prev != NULL)
> atom_free(atom->prev);
>
> - clear_bit(atom->bit_inpage,
> - ((struct kwork_atom_page *)atom->page_addr)->bitmap);
> + __clear_bit(atom->bit_inpage,
> + ((struct kwork_atom_page *)atom->page_addr)->bitmap);
> }
>
> static void atom_del(struct kwork_atom *atom)
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index e128b855ddde..2711c141c5bf 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -3555,7 +3555,7 @@ static int record__mmap_cpu_mask_init(struct mmap_cpu_mask *mask, struct perf_cp
> /* Return ENODEV is input cpu is greater than max cpu */
> if ((unsigned long)cpu.cpu > mask->nbits)
> return -ENODEV;
> - set_bit(cpu.cpu, mask->bits);
> + __set_bit(cpu.cpu, mask->bits);
> }
>
> return 0;
> @@ -3627,8 +3627,8 @@ static int record__init_thread_cpu_masks(struct record *rec, struct perf_cpu_map
> pr_debug("nr_threads: %d\n", rec->nr_threads);
>
> for (t = 0; t < rec->nr_threads; t++) {
> - set_bit(perf_cpu_map__cpu(cpus, t).cpu, rec->thread_masks[t].maps.bits);
> - set_bit(perf_cpu_map__cpu(cpus, t).cpu, rec->thread_masks[t].affinity.bits);
> + __set_bit(perf_cpu_map__cpu(cpus, t).cpu, rec->thread_masks[t].maps.bits);
> + __set_bit(perf_cpu_map__cpu(cpus, t).cpu, rec->thread_masks[t].affinity.bits);
> if (verbose) {
> pr_debug("thread_masks[%d]: ", t);
> mmap_cpu_mask__scnprintf(&rec->thread_masks[t].maps, "maps");
> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index f93737eef07b..86e18575c9be 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -1573,7 +1573,7 @@ static int map_switch_event(struct perf_sched *sched, struct evsel *evsel,
>
> if (sched->map.comp) {
> cpus_nr = bitmap_weight(sched->map.comp_cpus_mask, MAX_CPUS);
> - if (!test_and_set_bit(this_cpu.cpu, sched->map.comp_cpus_mask)) {
> + if (!__test_and_set_bit(this_cpu.cpu, sched->map.comp_cpus_mask)) {
> sched->map.comp_cpus[cpus_nr++] = this_cpu;
> new_cpu = true;
> }
> diff --git a/tools/perf/tests/bitmap.c b/tools/perf/tests/bitmap.c
> index 4965dd666956..0173f5402a35 100644
> --- a/tools/perf/tests/bitmap.c
> +++ b/tools/perf/tests/bitmap.c
> @@ -18,7 +18,7 @@ static unsigned long *get_bitmap(const char *str, int nbits)
>
> if (map && bm) {
> for (i = 0; i < perf_cpu_map__nr(map); i++)
> - set_bit(perf_cpu_map__cpu(map, i).cpu, bm);
> + __set_bit(perf_cpu_map__cpu(map, i).cpu, bm);
> }
>
> if (map)
> diff --git a/tools/perf/tests/mem2node.c b/tools/perf/tests/mem2node.c
> index 4c96829510c9..a0e88c496107 100644
> --- a/tools/perf/tests/mem2node.c
> +++ b/tools/perf/tests/mem2node.c
> @@ -33,7 +33,7 @@ static unsigned long *get_bitmap(const char *str, int nbits)
> int i;
>
> perf_cpu_map__for_each_cpu(cpu, i, map)
> - set_bit(cpu.cpu, bm);
> + __set_bit(cpu.cpu, bm);
> }
>
> if (map)
> diff --git a/tools/perf/util/affinity.c b/tools/perf/util/affinity.c
> index 4ee96b3c755b..38dc4524b7e8 100644
> --- a/tools/perf/util/affinity.c
> +++ b/tools/perf/util/affinity.c
> @@ -58,14 +58,14 @@ void affinity__set(struct affinity *a, int cpu)
> return;
>
> a->changed = true;
> - set_bit(cpu, a->sched_cpus);
> + __set_bit(cpu, a->sched_cpus);
> /*
> * We ignore errors because affinity is just an optimization.
> * This could happen for example with isolated CPUs or cpusets.
> * In this case the IPIs inside the kernel's perf API still work.
> */
> sched_setaffinity(0, cpu_set_size, (cpu_set_t *)a->sched_cpus);
> - clear_bit(cpu, a->sched_cpus);
> + __clear_bit(cpu, a->sched_cpus);
> }
>
> static void __affinity__cleanup(struct affinity *a)
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 98dfaf84bd13..dc2ae397d400 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -79,12 +79,12 @@ struct perf_file_attr {
>
> void perf_header__set_feat(struct perf_header *header, int feat)
> {
> - set_bit(feat, header->adds_features);
> + __set_bit(feat, header->adds_features);
> }
>
> void perf_header__clear_feat(struct perf_header *header, int feat)
> {
> - clear_bit(feat, header->adds_features);
> + __clear_bit(feat, header->adds_features);
> }
>
> bool perf_header__has_feat(const struct perf_header *header, int feat)
> @@ -1358,7 +1358,7 @@ static int memory_node__read(struct memory_node *n, unsigned long idx)
> rewinddir(dir);
>
> for_each_memory(phys, dir) {
> - set_bit(phys, n->set);
> + __set_bit(phys, n->set);
> }
>
> closedir(dir);
> @@ -3952,7 +3952,7 @@ int perf_file_header__read(struct perf_file_header *header,
>
> if (!test_bit(HEADER_HOSTNAME, header->adds_features)) {
> bitmap_zero(header->adds_features, HEADER_FEAT_BITS);
> - set_bit(HEADER_BUILD_ID, header->adds_features);
> + __set_bit(HEADER_BUILD_ID, header->adds_features);
> }
> }
>
> diff --git a/tools/perf/util/mmap.c b/tools/perf/util/mmap.c
> index a4dff881be39..49093b21ee2d 100644
> --- a/tools/perf/util/mmap.c
> +++ b/tools/perf/util/mmap.c
> @@ -111,7 +111,7 @@ static int perf_mmap__aio_bind(struct mmap *map, int idx, struct perf_cpu cpu, i
> pr_err("Failed to allocate node mask for mbind: error %m\n");
> return -1;
> }
> - set_bit(node_index, node_mask);
> + __set_bit(node_index, node_mask);
> if (mbind(data, mmap_len, MPOL_BIND, node_mask, node_index + 1 + 1, 0)) {
> pr_err("Failed to bind [%p-%p] AIO buffer to node %lu: error %m\n",
> data, data + mmap_len, node_index);
> @@ -256,7 +256,7 @@ static void build_node_mask(int node, struct mmap_cpu_mask *mask)
> for (idx = 0; idx < nr_cpus; idx++) {
> cpu = perf_cpu_map__cpu(cpu_map, idx); /* map c index to online cpu index */
> if (cpu__get_node(cpu) == node)
> - set_bit(cpu.cpu, mask->bits);
> + __set_bit(cpu.cpu, mask->bits);
> }
> }
>
> @@ -270,7 +270,7 @@ static int perf_mmap__setup_affinity_mask(struct mmap *map, struct mmap_params *
> if (mp->affinity == PERF_AFFINITY_NODE && cpu__max_node() > 1)
> build_node_mask(cpu__get_node(map->core.cpu), &map->affinity_mask);
> else if (mp->affinity == PERF_AFFINITY_CPU)
> - set_bit(map->core.cpu.cpu, map->affinity_mask.bits);
> + __set_bit(map->core.cpu.cpu, map->affinity_mask.bits);
>
> return 0;
> }
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index 03284059175f..371d8f7a3de3 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -1513,7 +1513,7 @@ void perf_pmu__set_format(unsigned long *bits, long from, long to)
>
> memset(bits, 0, BITS_TO_BYTES(PERF_PMU_FORMAT_BITS));
> for (b = from; b <= to; b++)
> - set_bit(b, bits);
> + __set_bit(b, bits);
> }
>
> void perf_pmu__del_formats(struct list_head *formats)
> diff --git a/tools/perf/util/scripting-engines/trace-event-perl.c b/tools/perf/util/scripting-engines/trace-event-perl.c
> index a5d945415bbc..5b602b6d4685 100644
> --- a/tools/perf/util/scripting-engines/trace-event-perl.c
> +++ b/tools/perf/util/scripting-engines/trace-event-perl.c
> @@ -365,7 +365,7 @@ static void perl_process_tracepoint(struct perf_sample *sample,
>
> sprintf(handler, "%s::%s", event->system, event->name);
>
> - if (!test_and_set_bit(event->id, events_defined))
> + if (!__test_and_set_bit(event->id, events_defined))
> define_event_symbols(event, handler, event->print_fmt.args);
>
> s = nsecs / NSEC_PER_SEC;
> diff --git a/tools/perf/util/scripting-engines/trace-event-python.c b/tools/perf/util/scripting-engines/trace-event-python.c
> index 1f2040f36d4e..0f229fa29163 100644
> --- a/tools/perf/util/scripting-engines/trace-event-python.c
> +++ b/tools/perf/util/scripting-engines/trace-event-python.c
> @@ -933,7 +933,7 @@ static void python_process_tracepoint(struct perf_sample *sample,
>
> sprintf(handler_name, "%s__%s", event->system, event->name);
>
> - if (!test_and_set_bit(event->id, events_defined))
> + if (!__test_and_set_bit(event->id, events_defined))
> define_event_symbols(event, handler_name, event->print_fmt.args);
>
> handler = get_handler(handler_name);
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 1a4f10de29ff..873fd51ec1b2 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -2748,7 +2748,7 @@ int perf_session__cpu_bitmap(struct perf_session *session,
> goto out_delete_map;
> }
>
> - set_bit(cpu.cpu, cpu_bitmap);
> + __set_bit(cpu.cpu, cpu_bitmap);
> }
>
> err = 0;
> diff --git a/tools/perf/util/svghelper.c b/tools/perf/util/svghelper.c
> index 1e0c731fc539..5c62d3118c41 100644
> --- a/tools/perf/util/svghelper.c
> +++ b/tools/perf/util/svghelper.c
> @@ -741,7 +741,7 @@ static int str_to_bitmap(char *s, cpumask_t *b, int nr_cpus)
> break;
> }
>
> - set_bit(c.cpu, cpumask_bits(b));
> + __set_bit(c.cpu, cpumask_bits(b));
> }
>
> perf_cpu_map__put(m);
> --
> 2.38.1.584.g0f3c55d4c2-goog
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 5/9] perf tools: Use dedicated non-atomic clear/set bit helpers
2022-12-02 18:16 ` Namhyung Kim
@ 2022-12-02 18:27 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 13+ messages in thread
From: Arnaldo Carvalho de Melo @ 2022-12-02 18:27 UTC (permalink / raw)
To: Namhyung Kim
Cc: Sean Christopherson, Yury Norov, Peter Zijlstra, Ingo Molnar,
Marc Zyngier, Paolo Bonzini, Andy Shevchenko, Rasmus Villemoes,
Mark Rutland, Alexander Shishkin, Jiri Olsa, James Morse,
Alexandru Elisei, Suzuki K Poulose, Oliver Upton,
linux-perf-users, linux-kernel, linux-arm-kernel, kvmarm, kvmarm,
kvm
Em Fri, Dec 02, 2022 at 10:16:02AM -0800, Namhyung Kim escreveu:
> Hello,
>
> On Fri, Nov 18, 2022 at 5:35 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > Use the dedicated non-atomic helpers for {clear,set}_bit() and their
> > test variants, i.e. the double-underscore versions. Depsite being
> > defined in atomic.h, and despite the kernel versions being atomic in the
> > kernel, tools' {clear,set}_bit() helpers aren't actually atomic. Move
> > to the double-underscore versions so that the versions that are expected
> > to be atomic (for kernel developers) can be made atomic without affecting
> > users that don't want atomic operations.
> >
> > No functional change intended.
> >
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
>
> Acked-by: Namhyung Kim <namhyung@kernel.org>
Thanks, applied.
- Arnaldo
> Thanks,
> Namhyung
>
>
> > ---
> > tools/perf/bench/find-bit-bench.c | 2 +-
> > tools/perf/builtin-c2c.c | 6 +++---
> > tools/perf/builtin-kwork.c | 6 +++---
> > tools/perf/builtin-record.c | 6 +++---
> > tools/perf/builtin-sched.c | 2 +-
> > tools/perf/tests/bitmap.c | 2 +-
> > tools/perf/tests/mem2node.c | 2 +-
> > tools/perf/util/affinity.c | 4 ++--
> > tools/perf/util/header.c | 8 ++++----
> > tools/perf/util/mmap.c | 6 +++---
> > tools/perf/util/pmu.c | 2 +-
> > tools/perf/util/scripting-engines/trace-event-perl.c | 2 +-
> > tools/perf/util/scripting-engines/trace-event-python.c | 2 +-
> > tools/perf/util/session.c | 2 +-
> > tools/perf/util/svghelper.c | 2 +-
> > 15 files changed, 27 insertions(+), 27 deletions(-)
> >
> > diff --git a/tools/perf/bench/find-bit-bench.c b/tools/perf/bench/find-bit-bench.c
> > index 22b5cfe97023..d103c3136983 100644
> > --- a/tools/perf/bench/find-bit-bench.c
> > +++ b/tools/perf/bench/find-bit-bench.c
> > @@ -70,7 +70,7 @@ static int do_for_each_set_bit(unsigned int num_bits)
> > bitmap_zero(to_test, num_bits);
> > skip = num_bits / set_bits;
> > for (i = 0; i < num_bits; i += skip)
> > - set_bit(i, to_test);
> > + __set_bit(i, to_test);
> >
> > for (i = 0; i < outer_iterations; i++) {
> > old = accumulator;
> > diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> > index a9190458d2d5..52d94c7dd836 100644
> > --- a/tools/perf/builtin-c2c.c
> > +++ b/tools/perf/builtin-c2c.c
> > @@ -230,7 +230,7 @@ static void c2c_he__set_cpu(struct c2c_hist_entry *c2c_he,
> > "WARNING: no sample cpu value"))
> > return;
> >
> > - set_bit(sample->cpu, c2c_he->cpuset);
> > + __set_bit(sample->cpu, c2c_he->cpuset);
> > }
> >
> > static void c2c_he__set_node(struct c2c_hist_entry *c2c_he,
> > @@ -247,7 +247,7 @@ static void c2c_he__set_node(struct c2c_hist_entry *c2c_he,
> > if (WARN_ONCE(node < 0, "WARNING: failed to find node\n"))
> > return;
> >
> > - set_bit(node, c2c_he->nodeset);
> > + __set_bit(node, c2c_he->nodeset);
> >
> > if (c2c_he->paddr != sample->phys_addr) {
> > c2c_he->paddr_cnt++;
> > @@ -2318,7 +2318,7 @@ static int setup_nodes(struct perf_session *session)
> > continue;
> >
> > perf_cpu_map__for_each_cpu(cpu, idx, map) {
> > - set_bit(cpu.cpu, set);
> > + __set_bit(cpu.cpu, set);
> >
> > if (WARN_ONCE(cpu2node[cpu.cpu] != -1, "node/cpu topology bug"))
> > return -EINVAL;
> > diff --git a/tools/perf/builtin-kwork.c b/tools/perf/builtin-kwork.c
> > index fb8c63656ad8..1f63e24f704e 100644
> > --- a/tools/perf/builtin-kwork.c
> > +++ b/tools/perf/builtin-kwork.c
> > @@ -216,7 +216,7 @@ static struct kwork_atom *atom_new(struct perf_kwork *kwork,
> > list_add_tail(&page->list, &kwork->atom_page_list);
> >
> > found_atom:
> > - set_bit(i, page->bitmap);
> > + __set_bit(i, page->bitmap);
> > atom->time = sample->time;
> > atom->prev = NULL;
> > atom->page_addr = page;
> > @@ -229,8 +229,8 @@ static void atom_free(struct kwork_atom *atom)
> > if (atom->prev != NULL)
> > atom_free(atom->prev);
> >
> > - clear_bit(atom->bit_inpage,
> > - ((struct kwork_atom_page *)atom->page_addr)->bitmap);
> > + __clear_bit(atom->bit_inpage,
> > + ((struct kwork_atom_page *)atom->page_addr)->bitmap);
> > }
> >
> > static void atom_del(struct kwork_atom *atom)
> > diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> > index e128b855ddde..2711c141c5bf 100644
> > --- a/tools/perf/builtin-record.c
> > +++ b/tools/perf/builtin-record.c
> > @@ -3555,7 +3555,7 @@ static int record__mmap_cpu_mask_init(struct mmap_cpu_mask *mask, struct perf_cp
> > /* Return ENODEV is input cpu is greater than max cpu */
> > if ((unsigned long)cpu.cpu > mask->nbits)
> > return -ENODEV;
> > - set_bit(cpu.cpu, mask->bits);
> > + __set_bit(cpu.cpu, mask->bits);
> > }
> >
> > return 0;
> > @@ -3627,8 +3627,8 @@ static int record__init_thread_cpu_masks(struct record *rec, struct perf_cpu_map
> > pr_debug("nr_threads: %d\n", rec->nr_threads);
> >
> > for (t = 0; t < rec->nr_threads; t++) {
> > - set_bit(perf_cpu_map__cpu(cpus, t).cpu, rec->thread_masks[t].maps.bits);
> > - set_bit(perf_cpu_map__cpu(cpus, t).cpu, rec->thread_masks[t].affinity.bits);
> > + __set_bit(perf_cpu_map__cpu(cpus, t).cpu, rec->thread_masks[t].maps.bits);
> > + __set_bit(perf_cpu_map__cpu(cpus, t).cpu, rec->thread_masks[t].affinity.bits);
> > if (verbose) {
> > pr_debug("thread_masks[%d]: ", t);
> > mmap_cpu_mask__scnprintf(&rec->thread_masks[t].maps, "maps");
> > diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> > index f93737eef07b..86e18575c9be 100644
> > --- a/tools/perf/builtin-sched.c
> > +++ b/tools/perf/builtin-sched.c
> > @@ -1573,7 +1573,7 @@ static int map_switch_event(struct perf_sched *sched, struct evsel *evsel,
> >
> > if (sched->map.comp) {
> > cpus_nr = bitmap_weight(sched->map.comp_cpus_mask, MAX_CPUS);
> > - if (!test_and_set_bit(this_cpu.cpu, sched->map.comp_cpus_mask)) {
> > + if (!__test_and_set_bit(this_cpu.cpu, sched->map.comp_cpus_mask)) {
> > sched->map.comp_cpus[cpus_nr++] = this_cpu;
> > new_cpu = true;
> > }
> > diff --git a/tools/perf/tests/bitmap.c b/tools/perf/tests/bitmap.c
> > index 4965dd666956..0173f5402a35 100644
> > --- a/tools/perf/tests/bitmap.c
> > +++ b/tools/perf/tests/bitmap.c
> > @@ -18,7 +18,7 @@ static unsigned long *get_bitmap(const char *str, int nbits)
> >
> > if (map && bm) {
> > for (i = 0; i < perf_cpu_map__nr(map); i++)
> > - set_bit(perf_cpu_map__cpu(map, i).cpu, bm);
> > + __set_bit(perf_cpu_map__cpu(map, i).cpu, bm);
> > }
> >
> > if (map)
> > diff --git a/tools/perf/tests/mem2node.c b/tools/perf/tests/mem2node.c
> > index 4c96829510c9..a0e88c496107 100644
> > --- a/tools/perf/tests/mem2node.c
> > +++ b/tools/perf/tests/mem2node.c
> > @@ -33,7 +33,7 @@ static unsigned long *get_bitmap(const char *str, int nbits)
> > int i;
> >
> > perf_cpu_map__for_each_cpu(cpu, i, map)
> > - set_bit(cpu.cpu, bm);
> > + __set_bit(cpu.cpu, bm);
> > }
> >
> > if (map)
> > diff --git a/tools/perf/util/affinity.c b/tools/perf/util/affinity.c
> > index 4ee96b3c755b..38dc4524b7e8 100644
> > --- a/tools/perf/util/affinity.c
> > +++ b/tools/perf/util/affinity.c
> > @@ -58,14 +58,14 @@ void affinity__set(struct affinity *a, int cpu)
> > return;
> >
> > a->changed = true;
> > - set_bit(cpu, a->sched_cpus);
> > + __set_bit(cpu, a->sched_cpus);
> > /*
> > * We ignore errors because affinity is just an optimization.
> > * This could happen for example with isolated CPUs or cpusets.
> > * In this case the IPIs inside the kernel's perf API still work.
> > */
> > sched_setaffinity(0, cpu_set_size, (cpu_set_t *)a->sched_cpus);
> > - clear_bit(cpu, a->sched_cpus);
> > + __clear_bit(cpu, a->sched_cpus);
> > }
> >
> > static void __affinity__cleanup(struct affinity *a)
> > diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> > index 98dfaf84bd13..dc2ae397d400 100644
> > --- a/tools/perf/util/header.c
> > +++ b/tools/perf/util/header.c
> > @@ -79,12 +79,12 @@ struct perf_file_attr {
> >
> > void perf_header__set_feat(struct perf_header *header, int feat)
> > {
> > - set_bit(feat, header->adds_features);
> > + __set_bit(feat, header->adds_features);
> > }
> >
> > void perf_header__clear_feat(struct perf_header *header, int feat)
> > {
> > - clear_bit(feat, header->adds_features);
> > + __clear_bit(feat, header->adds_features);
> > }
> >
> > bool perf_header__has_feat(const struct perf_header *header, int feat)
> > @@ -1358,7 +1358,7 @@ static int memory_node__read(struct memory_node *n, unsigned long idx)
> > rewinddir(dir);
> >
> > for_each_memory(phys, dir) {
> > - set_bit(phys, n->set);
> > + __set_bit(phys, n->set);
> > }
> >
> > closedir(dir);
> > @@ -3952,7 +3952,7 @@ int perf_file_header__read(struct perf_file_header *header,
> >
> > if (!test_bit(HEADER_HOSTNAME, header->adds_features)) {
> > bitmap_zero(header->adds_features, HEADER_FEAT_BITS);
> > - set_bit(HEADER_BUILD_ID, header->adds_features);
> > + __set_bit(HEADER_BUILD_ID, header->adds_features);
> > }
> > }
> >
> > diff --git a/tools/perf/util/mmap.c b/tools/perf/util/mmap.c
> > index a4dff881be39..49093b21ee2d 100644
> > --- a/tools/perf/util/mmap.c
> > +++ b/tools/perf/util/mmap.c
> > @@ -111,7 +111,7 @@ static int perf_mmap__aio_bind(struct mmap *map, int idx, struct perf_cpu cpu, i
> > pr_err("Failed to allocate node mask for mbind: error %m\n");
> > return -1;
> > }
> > - set_bit(node_index, node_mask);
> > + __set_bit(node_index, node_mask);
> > if (mbind(data, mmap_len, MPOL_BIND, node_mask, node_index + 1 + 1, 0)) {
> > pr_err("Failed to bind [%p-%p] AIO buffer to node %lu: error %m\n",
> > data, data + mmap_len, node_index);
> > @@ -256,7 +256,7 @@ static void build_node_mask(int node, struct mmap_cpu_mask *mask)
> > for (idx = 0; idx < nr_cpus; idx++) {
> > cpu = perf_cpu_map__cpu(cpu_map, idx); /* map c index to online cpu index */
> > if (cpu__get_node(cpu) == node)
> > - set_bit(cpu.cpu, mask->bits);
> > + __set_bit(cpu.cpu, mask->bits);
> > }
> > }
> >
> > @@ -270,7 +270,7 @@ static int perf_mmap__setup_affinity_mask(struct mmap *map, struct mmap_params *
> > if (mp->affinity == PERF_AFFINITY_NODE && cpu__max_node() > 1)
> > build_node_mask(cpu__get_node(map->core.cpu), &map->affinity_mask);
> > else if (mp->affinity == PERF_AFFINITY_CPU)
> > - set_bit(map->core.cpu.cpu, map->affinity_mask.bits);
> > + __set_bit(map->core.cpu.cpu, map->affinity_mask.bits);
> >
> > return 0;
> > }
> > diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> > index 03284059175f..371d8f7a3de3 100644
> > --- a/tools/perf/util/pmu.c
> > +++ b/tools/perf/util/pmu.c
> > @@ -1513,7 +1513,7 @@ void perf_pmu__set_format(unsigned long *bits, long from, long to)
> >
> > memset(bits, 0, BITS_TO_BYTES(PERF_PMU_FORMAT_BITS));
> > for (b = from; b <= to; b++)
> > - set_bit(b, bits);
> > + __set_bit(b, bits);
> > }
> >
> > void perf_pmu__del_formats(struct list_head *formats)
> > diff --git a/tools/perf/util/scripting-engines/trace-event-perl.c b/tools/perf/util/scripting-engines/trace-event-perl.c
> > index a5d945415bbc..5b602b6d4685 100644
> > --- a/tools/perf/util/scripting-engines/trace-event-perl.c
> > +++ b/tools/perf/util/scripting-engines/trace-event-perl.c
> > @@ -365,7 +365,7 @@ static void perl_process_tracepoint(struct perf_sample *sample,
> >
> > sprintf(handler, "%s::%s", event->system, event->name);
> >
> > - if (!test_and_set_bit(event->id, events_defined))
> > + if (!__test_and_set_bit(event->id, events_defined))
> > define_event_symbols(event, handler, event->print_fmt.args);
> >
> > s = nsecs / NSEC_PER_SEC;
> > diff --git a/tools/perf/util/scripting-engines/trace-event-python.c b/tools/perf/util/scripting-engines/trace-event-python.c
> > index 1f2040f36d4e..0f229fa29163 100644
> > --- a/tools/perf/util/scripting-engines/trace-event-python.c
> > +++ b/tools/perf/util/scripting-engines/trace-event-python.c
> > @@ -933,7 +933,7 @@ static void python_process_tracepoint(struct perf_sample *sample,
> >
> > sprintf(handler_name, "%s__%s", event->system, event->name);
> >
> > - if (!test_and_set_bit(event->id, events_defined))
> > + if (!__test_and_set_bit(event->id, events_defined))
> > define_event_symbols(event, handler_name, event->print_fmt.args);
> >
> > handler = get_handler(handler_name);
> > diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> > index 1a4f10de29ff..873fd51ec1b2 100644
> > --- a/tools/perf/util/session.c
> > +++ b/tools/perf/util/session.c
> > @@ -2748,7 +2748,7 @@ int perf_session__cpu_bitmap(struct perf_session *session,
> > goto out_delete_map;
> > }
> >
> > - set_bit(cpu.cpu, cpu_bitmap);
> > + __set_bit(cpu.cpu, cpu_bitmap);
> > }
> >
> > err = 0;
> > diff --git a/tools/perf/util/svghelper.c b/tools/perf/util/svghelper.c
> > index 1e0c731fc539..5c62d3118c41 100644
> > --- a/tools/perf/util/svghelper.c
> > +++ b/tools/perf/util/svghelper.c
> > @@ -741,7 +741,7 @@ static int str_to_bitmap(char *s, cpumask_t *b, int nr_cpus)
> > break;
> > }
> >
> > - set_bit(c.cpu, cpumask_bits(b));
> > + __set_bit(c.cpu, cpumask_bits(b));
> > }
> >
> > perf_cpu_map__put(m);
> > --
> > 2.38.1.584.g0f3c55d4c2-goog
> >
--
- Arnaldo
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 6/9] KVM: selftests: Use non-atomic clear/set bit helpers in KVM tests
2022-11-19 1:34 [PATCH 0/9] tools: Make {clear,set}_bit() atomic for reals Sean Christopherson
` (4 preceding siblings ...)
2022-11-19 1:34 ` [PATCH 5/9] perf tools: Use dedicated non-atomic clear/set bit helpers Sean Christopherson
@ 2022-11-19 1:34 ` Sean Christopherson
2022-11-19 1:34 ` [PATCH 7/9] tools: Drop conflicting non-atomic test_and_{clear,set}_bit() helpers Sean Christopherson
` (3 subsequent siblings)
9 siblings, 0 replies; 13+ messages in thread
From: Sean Christopherson @ 2022-11-19 1:34 UTC (permalink / raw)
To: Yury Norov, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Marc Zyngier, Paolo Bonzini
Cc: Andy Shevchenko, Rasmus Villemoes, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, James Morse,
Alexandru Elisei, Suzuki K Poulose, Oliver Upton,
linux-perf-users, linux-kernel, linux-arm-kernel, kvmarm, kvmarm,
kvm, Sean Christopherson
Use the dedicated non-atomic helpers for {clear,set}_bit() and their
test variants, i.e. the double-underscore versions. Depsite being
defined in atomic.h, and despite the kernel versions being atomic in the
kernel, tools' {clear,set}_bit() helpers aren't actually atomic. Move
to the double-underscore versions so that the versions that are expected
to be atomic (for kernel developers) can be made atomic without affecting
users that don't want atomic operations.
Leave the usage in ucall_free() as-is, it's the one place in tools/ that
actually wants/needs atomic behavior.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
.../selftests/kvm/aarch64/arch_timer.c | 2 +-
tools/testing/selftests/kvm/dirty_log_test.c | 34 +++++++++----------
.../selftests/kvm/x86_64/hyperv_evmcs.c | 4 +--
.../selftests/kvm/x86_64/hyperv_svm_test.c | 4 +--
4 files changed, 22 insertions(+), 22 deletions(-)
diff --git a/tools/testing/selftests/kvm/aarch64/arch_timer.c b/tools/testing/selftests/kvm/aarch64/arch_timer.c
index f2a96779716a..26556a266021 100644
--- a/tools/testing/selftests/kvm/aarch64/arch_timer.c
+++ b/tools/testing/selftests/kvm/aarch64/arch_timer.c
@@ -222,7 +222,7 @@ static void *test_vcpu_run(void *arg)
/* Currently, any exit from guest is an indication of completion */
pthread_mutex_lock(&vcpu_done_map_lock);
- set_bit(vcpu_idx, vcpu_done_map);
+ __set_bit(vcpu_idx, vcpu_done_map);
pthread_mutex_unlock(&vcpu_done_map_lock);
switch (get_ucall(vcpu, &uc)) {
diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
index a38c4369fb8e..a75548865f6b 100644
--- a/tools/testing/selftests/kvm/dirty_log_test.c
+++ b/tools/testing/selftests/kvm/dirty_log_test.c
@@ -44,20 +44,20 @@
# define BITOP_LE_SWIZZLE ((BITS_PER_LONG-1) & ~0x7)
# define test_bit_le(nr, addr) \
test_bit((nr) ^ BITOP_LE_SWIZZLE, addr)
-# define set_bit_le(nr, addr) \
- set_bit((nr) ^ BITOP_LE_SWIZZLE, addr)
-# define clear_bit_le(nr, addr) \
- clear_bit((nr) ^ BITOP_LE_SWIZZLE, addr)
-# define test_and_set_bit_le(nr, addr) \
- test_and_set_bit((nr) ^ BITOP_LE_SWIZZLE, addr)
-# define test_and_clear_bit_le(nr, addr) \
- test_and_clear_bit((nr) ^ BITOP_LE_SWIZZLE, addr)
+# define __set_bit_le(nr, addr) \
+ __set_bit((nr) ^ BITOP_LE_SWIZZLE, addr)
+# define __clear_bit_le(nr, addr) \
+ __clear_bit((nr) ^ BITOP_LE_SWIZZLE, addr)
+# define __test_and_set_bit_le(nr, addr) \
+ __test_and_set_bit((nr) ^ BITOP_LE_SWIZZLE, addr)
+# define __test_and_clear_bit_le(nr, addr) \
+ __test_and_clear_bit((nr) ^ BITOP_LE_SWIZZLE, addr)
#else
-# define test_bit_le test_bit
-# define set_bit_le set_bit
-# define clear_bit_le clear_bit
-# define test_and_set_bit_le test_and_set_bit
-# define test_and_clear_bit_le test_and_clear_bit
+# define test_bit_le test_bit
+# define __set_bit_le __set_bit
+# define __clear_bit_le __clear_bit
+# define __test_and_set_bit_le __test_and_set_bit
+# define __test_and_clear_bit_le __test_and_clear_bit
#endif
#define TEST_DIRTY_RING_COUNT 65536
@@ -305,7 +305,7 @@ static uint32_t dirty_ring_collect_one(struct kvm_dirty_gfn *dirty_gfns,
TEST_ASSERT(cur->offset < num_pages, "Offset overflow: "
"0x%llx >= 0x%x", cur->offset, num_pages);
//pr_info("fetch 0x%x page %llu\n", *fetch_index, cur->offset);
- set_bit_le(cur->offset, bitmap);
+ __set_bit_le(cur->offset, bitmap);
dirty_ring_last_page = cur->offset;
dirty_gfn_set_collected(cur);
(*fetch_index)++;
@@ -560,7 +560,7 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap)
value_ptr = host_test_mem + page * host_page_size;
/* If this is a special page that we were tracking... */
- if (test_and_clear_bit_le(page, host_bmap_track)) {
+ if (__test_and_clear_bit_le(page, host_bmap_track)) {
host_track_next_count++;
TEST_ASSERT(test_bit_le(page, bmap),
"Page %"PRIu64" should have its dirty bit "
@@ -568,7 +568,7 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap)
page);
}
- if (test_and_clear_bit_le(page, bmap)) {
+ if (__test_and_clear_bit_le(page, bmap)) {
bool matched;
host_dirty_count++;
@@ -661,7 +661,7 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap)
* should report its dirtyness in the
* next run
*/
- set_bit_le(page, host_bmap_track);
+ __set_bit_le(page, host_bmap_track);
}
}
}
diff --git a/tools/testing/selftests/kvm/x86_64/hyperv_evmcs.c b/tools/testing/selftests/kvm/x86_64/hyperv_evmcs.c
index ea58e5b436e8..ab51865c80be 100644
--- a/tools/testing/selftests/kvm/x86_64/hyperv_evmcs.c
+++ b/tools/testing/selftests/kvm/x86_64/hyperv_evmcs.c
@@ -142,7 +142,7 @@ void guest_code(struct vmx_pages *vmx_pages, struct hyperv_test_pages *hv_pages,
/* Intercept RDMSR 0xc0000100 */
vmwrite(CPU_BASED_VM_EXEC_CONTROL, vmreadz(CPU_BASED_VM_EXEC_CONTROL) |
CPU_BASED_USE_MSR_BITMAPS);
- set_bit(MSR_FS_BASE & 0x1fff, vmx_pages->msr + 0x400);
+ __set_bit(MSR_FS_BASE & 0x1fff, vmx_pages->msr + 0x400);
GUEST_ASSERT(!vmresume());
GUEST_ASSERT(vmreadz(VM_EXIT_REASON) == EXIT_REASON_MSR_READ);
current_evmcs->guest_rip += 2; /* rdmsr */
@@ -154,7 +154,7 @@ void guest_code(struct vmx_pages *vmx_pages, struct hyperv_test_pages *hv_pages,
current_evmcs->guest_rip += 2; /* rdmsr */
/* Intercept RDMSR 0xc0000101 without telling KVM about it */
- set_bit(MSR_GS_BASE & 0x1fff, vmx_pages->msr + 0x400);
+ __set_bit(MSR_GS_BASE & 0x1fff, vmx_pages->msr + 0x400);
/* Make sure HV_VMX_ENLIGHTENED_CLEAN_FIELD_MSR_BITMAP is set */
current_evmcs->hv_clean_fields |= HV_VMX_ENLIGHTENED_CLEAN_FIELD_MSR_BITMAP;
GUEST_ASSERT(!vmresume());
diff --git a/tools/testing/selftests/kvm/x86_64/hyperv_svm_test.c b/tools/testing/selftests/kvm/x86_64/hyperv_svm_test.c
index 3b3cc94ba8e4..68a7d354ea07 100644
--- a/tools/testing/selftests/kvm/x86_64/hyperv_svm_test.c
+++ b/tools/testing/selftests/kvm/x86_64/hyperv_svm_test.c
@@ -103,7 +103,7 @@ static void __attribute__((__flatten__)) guest_code(struct svm_test_data *svm,
/* Intercept RDMSR 0xc0000100 */
vmcb->control.intercept |= 1ULL << INTERCEPT_MSR_PROT;
- set_bit(2 * (MSR_FS_BASE & 0x1fff), svm->msr + 0x800);
+ __set_bit(2 * (MSR_FS_BASE & 0x1fff), svm->msr + 0x800);
run_guest(vmcb, svm->vmcb_gpa);
GUEST_ASSERT(vmcb->control.exit_code == SVM_EXIT_MSR);
vmcb->save.rip += 2; /* rdmsr */
@@ -115,7 +115,7 @@ static void __attribute__((__flatten__)) guest_code(struct svm_test_data *svm,
vmcb->save.rip += 2; /* rdmsr */
/* Intercept RDMSR 0xc0000101 without telling KVM about it */
- set_bit(2 * (MSR_GS_BASE & 0x1fff), svm->msr + 0x800);
+ __set_bit(2 * (MSR_GS_BASE & 0x1fff), svm->msr + 0x800);
/* Make sure HV_VMX_ENLIGHTENED_CLEAN_FIELD_MSR_BITMAP is set */
vmcb->control.clean |= HV_VMCB_NESTED_ENLIGHTENMENTS;
run_guest(vmcb, svm->vmcb_gpa);
--
2.38.1.584.g0f3c55d4c2-goog
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 7/9] tools: Drop conflicting non-atomic test_and_{clear,set}_bit() helpers
2022-11-19 1:34 [PATCH 0/9] tools: Make {clear,set}_bit() atomic for reals Sean Christopherson
` (5 preceding siblings ...)
2022-11-19 1:34 ` [PATCH 6/9] KVM: selftests: Use non-atomic clear/set bit helpers in KVM tests Sean Christopherson
@ 2022-11-19 1:34 ` Sean Christopherson
2022-11-19 1:34 ` [PATCH 8/9] tools: Drop "atomic_" prefix from atomic test_and_set_bit() Sean Christopherson
` (2 subsequent siblings)
9 siblings, 0 replies; 13+ messages in thread
From: Sean Christopherson @ 2022-11-19 1:34 UTC (permalink / raw)
To: Yury Norov, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Marc Zyngier, Paolo Bonzini
Cc: Andy Shevchenko, Rasmus Villemoes, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, James Morse,
Alexandru Elisei, Suzuki K Poulose, Oliver Upton,
linux-perf-users, linux-kernel, linux-arm-kernel, kvmarm, kvmarm,
kvm, Sean Christopherson
Drop tools' non-atomic test_and_set_bit() and test_and_clear_bit() helpers
now that all users are gone. The names will be claimed in the future for
atomic versions.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
tools/include/linux/bitmap.h | 34 ----------------------------------
1 file changed, 34 deletions(-)
diff --git a/tools/include/linux/bitmap.h b/tools/include/linux/bitmap.h
index 65d0747c5205..f3566ea0f932 100644
--- a/tools/include/linux/bitmap.h
+++ b/tools/include/linux/bitmap.h
@@ -77,40 +77,6 @@ static inline void bitmap_or(unsigned long *dst, const unsigned long *src1,
__bitmap_or(dst, src1, src2, nbits);
}
-/**
- * test_and_set_bit - Set a bit and return its old value
- * @nr: Bit to set
- * @addr: Address to count from
- */
-static inline int test_and_set_bit(int nr, unsigned long *addr)
-{
- unsigned long mask = BIT_MASK(nr);
- unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
- unsigned long old;
-
- old = *p;
- *p = old | mask;
-
- return (old & mask) != 0;
-}
-
-/**
- * test_and_clear_bit - Clear a bit and return its old value
- * @nr: Bit to clear
- * @addr: Address to count from
- */
-static inline int test_and_clear_bit(int nr, unsigned long *addr)
-{
- unsigned long mask = BIT_MASK(nr);
- unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
- unsigned long old;
-
- old = *p;
- *p = old & ~mask;
-
- return (old & mask) != 0;
-}
-
/**
* bitmap_zalloc - Allocate bitmap
* @nbits: Number of bits
--
2.38.1.584.g0f3c55d4c2-goog
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 8/9] tools: Drop "atomic_" prefix from atomic test_and_set_bit()
2022-11-19 1:34 [PATCH 0/9] tools: Make {clear,set}_bit() atomic for reals Sean Christopherson
` (6 preceding siblings ...)
2022-11-19 1:34 ` [PATCH 7/9] tools: Drop conflicting non-atomic test_and_{clear,set}_bit() helpers Sean Christopherson
@ 2022-11-19 1:34 ` Sean Christopherson
2022-11-19 1:34 ` [PATCH 9/9] tools: KVM: selftests: Convert clear/set_bit() to actual atomics Sean Christopherson
2022-12-02 18:23 ` [PATCH 0/9] tools: Make {clear,set}_bit() atomic for reals Paolo Bonzini
9 siblings, 0 replies; 13+ messages in thread
From: Sean Christopherson @ 2022-11-19 1:34 UTC (permalink / raw)
To: Yury Norov, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Marc Zyngier, Paolo Bonzini
Cc: Andy Shevchenko, Rasmus Villemoes, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, James Morse,
Alexandru Elisei, Suzuki K Poulose, Oliver Upton,
linux-perf-users, linux-kernel, linux-arm-kernel, kvmarm, kvmarm,
kvm, Sean Christopherson
Drop the "atomic_" prefix from tools' atomic_test_and_set_bit() to
match the kernel nomenclature where test_and_set_bit() is atomic,
and __test_and_set_bit() provides the non-atomic variant.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
tools/arch/x86/include/asm/atomic.h | 3 +--
tools/include/asm-generic/atomic-gcc.h | 2 +-
tools/testing/selftests/kvm/lib/ucall_common.c | 2 +-
3 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/tools/arch/x86/include/asm/atomic.h b/tools/arch/x86/include/asm/atomic.h
index 01cc27ec4520..a42733af7d51 100644
--- a/tools/arch/x86/include/asm/atomic.h
+++ b/tools/arch/x86/include/asm/atomic.h
@@ -71,10 +71,9 @@ static __always_inline int atomic_cmpxchg(atomic_t *v, int old, int new)
return cmpxchg(&v->counter, old, new);
}
-static inline int atomic_test_and_set_bit(long nr, unsigned long *addr)
+static inline int test_and_set_bit(long nr, unsigned long *addr)
{
GEN_BINARY_RMWcc(LOCK_PREFIX __ASM_SIZE(bts), *addr, "Ir", nr, "%0", "c");
-
}
#endif /* _TOOLS_LINUX_ASM_X86_ATOMIC_H */
diff --git a/tools/include/asm-generic/atomic-gcc.h b/tools/include/asm-generic/atomic-gcc.h
index 6daa68bf5b9e..37ef522aaac4 100644
--- a/tools/include/asm-generic/atomic-gcc.h
+++ b/tools/include/asm-generic/atomic-gcc.h
@@ -70,7 +70,7 @@ static inline int atomic_cmpxchg(atomic_t *v, int oldval, int newval)
return cmpxchg(&(v)->counter, oldval, newval);
}
-static inline int atomic_test_and_set_bit(long nr, unsigned long *addr)
+static inline int test_and_set_bit(long nr, unsigned long *addr)
{
unsigned long mask = BIT_MASK(nr);
long old;
diff --git a/tools/testing/selftests/kvm/lib/ucall_common.c b/tools/testing/selftests/kvm/lib/ucall_common.c
index fcae96461e46..820ce6c82829 100644
--- a/tools/testing/selftests/kvm/lib/ucall_common.c
+++ b/tools/testing/selftests/kvm/lib/ucall_common.c
@@ -44,7 +44,7 @@ static struct ucall *ucall_alloc(void)
GUEST_ASSERT(ucall_pool);
for (i = 0; i < KVM_MAX_VCPUS; ++i) {
- if (!atomic_test_and_set_bit(i, ucall_pool->in_use)) {
+ if (!test_and_set_bit(i, ucall_pool->in_use)) {
uc = &ucall_pool->ucalls[i];
memset(uc->args, 0, sizeof(uc->args));
return uc;
--
2.38.1.584.g0f3c55d4c2-goog
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 9/9] tools: KVM: selftests: Convert clear/set_bit() to actual atomics
2022-11-19 1:34 [PATCH 0/9] tools: Make {clear,set}_bit() atomic for reals Sean Christopherson
` (7 preceding siblings ...)
2022-11-19 1:34 ` [PATCH 8/9] tools: Drop "atomic_" prefix from atomic test_and_set_bit() Sean Christopherson
@ 2022-11-19 1:34 ` Sean Christopherson
2022-12-02 18:23 ` [PATCH 0/9] tools: Make {clear,set}_bit() atomic for reals Paolo Bonzini
9 siblings, 0 replies; 13+ messages in thread
From: Sean Christopherson @ 2022-11-19 1:34 UTC (permalink / raw)
To: Yury Norov, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Marc Zyngier, Paolo Bonzini
Cc: Andy Shevchenko, Rasmus Villemoes, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, James Morse,
Alexandru Elisei, Suzuki K Poulose, Oliver Upton,
linux-perf-users, linux-kernel, linux-arm-kernel, kvmarm, kvmarm,
kvm, Sean Christopherson
Convert {clear,set}_bit() to atomics as KVM's ucall implementation relies
on clear_bit() being atomic, they are defined in atomic.h, and the same
helpers in the kernel proper are atomic.
KVM's ucall infrastructure is the only user of clear_bit() in tools/, and
there are no true set_bit() users. tools/testing/nvdimm/ does make heavy
use of set_bit(), but that code builds into a kernel module of sorts, i.e.
pulls in all of the kernel's header and so is already getting the kernel's
atomic set_bit().
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
tools/arch/x86/include/asm/atomic.h | 5 +++++
tools/include/asm-generic/atomic-gcc.h | 11 +++++++++++
tools/include/asm-generic/bitops/atomic.h | 15 ++++++---------
3 files changed, 22 insertions(+), 9 deletions(-)
diff --git a/tools/arch/x86/include/asm/atomic.h b/tools/arch/x86/include/asm/atomic.h
index a42733af7d51..365cf182df12 100644
--- a/tools/arch/x86/include/asm/atomic.h
+++ b/tools/arch/x86/include/asm/atomic.h
@@ -76,4 +76,9 @@ static inline int test_and_set_bit(long nr, unsigned long *addr)
GEN_BINARY_RMWcc(LOCK_PREFIX __ASM_SIZE(bts), *addr, "Ir", nr, "%0", "c");
}
+static inline int test_and_clear_bit(long nr, unsigned long *addr)
+{
+ GEN_BINARY_RMWcc(LOCK_PREFIX __ASM_SIZE(btc), *addr, "Ir", nr, "%0", "c");
+}
+
#endif /* _TOOLS_LINUX_ASM_X86_ATOMIC_H */
diff --git a/tools/include/asm-generic/atomic-gcc.h b/tools/include/asm-generic/atomic-gcc.h
index 37ef522aaac4..9b3c528bab92 100644
--- a/tools/include/asm-generic/atomic-gcc.h
+++ b/tools/include/asm-generic/atomic-gcc.h
@@ -81,4 +81,15 @@ static inline int test_and_set_bit(long nr, unsigned long *addr)
return !!(old & mask);
}
+static inline int test_and_clear_bit(long nr, unsigned long *addr)
+{
+ unsigned long mask = BIT_MASK(nr);
+ long old;
+
+ addr += BIT_WORD(nr);
+
+ old = __sync_fetch_and_and(addr, ~mask);
+ return !!(old & mask);
+}
+
#endif /* __TOOLS_ASM_GENERIC_ATOMIC_H */
diff --git a/tools/include/asm-generic/bitops/atomic.h b/tools/include/asm-generic/bitops/atomic.h
index f64b049d236c..ab37a221b41a 100644
--- a/tools/include/asm-generic/bitops/atomic.h
+++ b/tools/include/asm-generic/bitops/atomic.h
@@ -5,14 +5,11 @@
#include <asm/types.h>
#include <asm/bitsperlong.h>
-static inline void set_bit(unsigned long nr, unsigned long *addr)
-{
- addr[nr / __BITS_PER_LONG] |= 1UL << (nr % __BITS_PER_LONG);
-}
-
-static inline void clear_bit(unsigned long nr, unsigned long *addr)
-{
- addr[nr / __BITS_PER_LONG] &= ~(1UL << (nr % __BITS_PER_LONG));
-}
+/*
+ * Just alias the test versions, all of the compiler built-in atomics "fetch",
+ * and optimizing compile-time constants on x86 isn't worth the complexity.
+ */
+#define set_bit test_and_set_bit
+#define clear_bit test_and_clear_bit
#endif /* _TOOLS_LINUX_ASM_GENERIC_BITOPS_ATOMIC_H_ */
--
2.38.1.584.g0f3c55d4c2-goog
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 0/9] tools: Make {clear,set}_bit() atomic for reals
2022-11-19 1:34 [PATCH 0/9] tools: Make {clear,set}_bit() atomic for reals Sean Christopherson
` (8 preceding siblings ...)
2022-11-19 1:34 ` [PATCH 9/9] tools: KVM: selftests: Convert clear/set_bit() to actual atomics Sean Christopherson
@ 2022-12-02 18:23 ` Paolo Bonzini
9 siblings, 0 replies; 13+ messages in thread
From: Paolo Bonzini @ 2022-12-02 18:23 UTC (permalink / raw)
To: Sean Christopherson, Yury Norov, Peter Zijlstra, Ingo Molnar,
Arnaldo Carvalho de Melo, Marc Zyngier
Cc: Andy Shevchenko, Rasmus Villemoes, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, James Morse,
Alexandru Elisei, Suzuki K Poulose, Oliver Upton,
linux-perf-users, linux-kernel, linux-arm-kernel, kvmarm, kvmarm,
kvm
On 11/19/22 02:34, Sean Christopherson wrote:
> For obvious reasons I'd like to route the this through Paolo's tree.
> In theory, taking just patch 5 through tip would work, but creating a
> topic branch seems like the way to go, though maybe I'm being overly
> paranoid. The current tip/perf/core doesn't have any conflicts, nor does
> it have new set_bit() or clear_bit() users.
>
>
> Code sitting in kvm/queue for 6.2 adds functionality that relies on
> clear_bit() being an atomic operation. Unfortunately, despite being
> implemented in atomic.h (among other strong hits that they should be
> atomic), clear_bit() and set_bit() aren't actually atomic (and of course
> I realized this _just_ after everything got queued up).
>
> Move current tools/ users of clear_bit() and set_bit() to the
> double-underscore versions (which tools/ already provides and documents
> as being non-atomic), and then implement clear_bit() and set_bit() as
> actual atomics to fix the KVM selftests bug.
>
> Perf and KVM are the only affected users. NVDIMM also has test code
> in tools/, but that builds against the kernel proper. The KVM code is
> well tested and fully audited. The perf code is lightly tested; if I
> understand the build system, it's probably not even fully compile tested.
>
> Patches 1 and 2 are completely unrelated and are fixes for patches
> sitting in kvm/queue. Paolo, they can be squashed if you want to rewrite
> history.
>
> Patch 3 fixes a hilarious collision in a KVM ARM selftest that will arise
> when clear_bit() is converted to an atomic.
>
> Patch 4 changes clear_bit() and set_bit() to take an "unsigned long"
> instead of an "int" so that patches 5-6 aren't accompanied by functional
> changes. I.e. if something in perf is somehow relying on "bit" being a
> signed int, failures will bisect to patch 4 and not to the
> supposed-to-be-a-nop conversion to __clear_bit() and __set_bit().
>
> Patch 5-9 switch perf+KVM and complete the conversion.
>
> Applies on:
>
> git://git.kernel.org/pub/scm/virt/kvm/kvm.git queue
Queued, thanks Namhyung for the ACK!
Paolo
>
> Sean Christopherson (9):
> KVM: selftests: Add rdmsr_from_l2() implementation in Hyper-V eVMCS
> test
> KVM: selftests: Remove unused "vcpu" param to fix build error
> KVM: arm64: selftests: Enable single-step without a "full" ucall()
> tools: Take @bit as an "unsigned long" in {clear,set}_bit() helpers
> perf tools: Use dedicated non-atomic clear/set bit helpers
> KVM: selftests: Use non-atomic clear/set bit helpers in KVM tests
> tools: Drop conflicting non-atomic test_and_{clear,set}_bit() helpers
> tools: Drop "atomic_" prefix from atomic test_and_set_bit()
> tools: KVM: selftests: Convert clear/set_bit() to actual atomics
>
> tools/arch/x86/include/asm/atomic.h | 6 +++-
> tools/include/asm-generic/atomic-gcc.h | 13 ++++++-
> tools/include/asm-generic/bitops/atomic.h | 15 ++++----
> tools/include/linux/bitmap.h | 34 -------------------
> tools/perf/bench/find-bit-bench.c | 2 +-
> tools/perf/builtin-c2c.c | 6 ++--
> tools/perf/builtin-kwork.c | 6 ++--
> tools/perf/builtin-record.c | 6 ++--
> tools/perf/builtin-sched.c | 2 +-
> tools/perf/tests/bitmap.c | 2 +-
> tools/perf/tests/mem2node.c | 2 +-
> tools/perf/util/affinity.c | 4 +--
> tools/perf/util/header.c | 8 ++---
> tools/perf/util/mmap.c | 6 ++--
> tools/perf/util/pmu.c | 2 +-
> .../util/scripting-engines/trace-event-perl.c | 2 +-
> .../scripting-engines/trace-event-python.c | 2 +-
> tools/perf/util/session.c | 2 +-
> tools/perf/util/svghelper.c | 2 +-
> .../selftests/kvm/aarch64/arch_timer.c | 2 +-
> .../selftests/kvm/aarch64/debug-exceptions.c | 21 ++++++------
> tools/testing/selftests/kvm/dirty_log_test.c | 34 +++++++++----------
> .../selftests/kvm/include/ucall_common.h | 8 +++++
> .../testing/selftests/kvm/lib/ucall_common.c | 2 +-
> .../selftests/kvm/x86_64/hyperv_evmcs.c | 13 +++++--
> .../selftests/kvm/x86_64/hyperv_svm_test.c | 4 +--
> .../selftests/kvm/x86_64/hyperv_tlb_flush.c | 2 +-
> 27 files changed, 102 insertions(+), 106 deletions(-)
>
>
> base-commit: 3321eef4acb51c303f0598d8a8493ca58528a054
^ permalink raw reply [flat|nested] 13+ messages in thread