Hi Paolo and Sean,
On 7/15/22 1:35 AM, Sean Christopherson wrote:
On Thu, Jul 14, 2022, Paolo Bonzini wrote:
On 7/14/22 10:06, Gavin Shan wrote:
In rseq_test, there are two threads created. Those two threads are
'main' and 'migration_thread' separately. We also have the assumption
that non-migration status on 'migration-worker' thread guarantees the
same non-migration status on 'main' thread. Unfortunately, the assumption
isn't true. The 'main' thread can be migrated from one CPU to another
one between the calls to sched_getcpu() and READ_ONCE(__rseq.cpu_id).
The following assert is raised eventually because of the mismatched
CPU numbers.
The issue can be reproduced on arm64 system occasionally.
Hmm, this does not seem a correct patch - the threads are already
synchronizing using seq_cnt, like this:
migration main
---------------------- --------------------------------
seq_cnt = 1
smp_wmb()
snapshot = 0
smp_rmb()
cpu = sched_getcpu() reads 23
sched_setaffinity()
rseq_cpu = __rseq.cpuid reads 35
smp_rmb()
snapshot != seq_cnt -> retry
smp_wmb()
seq_cnt = 2
sched_setaffinity() is guaranteed to block until the task is enqueued on an
allowed CPU.
Yes, and retrying could suppress detection of kernel bugs that this test is intended
to catch.
Well, I don't think migration_worker() does correct thing, if I'm understanding
correctly. The intention seems to force migration on 'main' thread by 'migration'
thread? If that is the case, I don't think the following function call has correct
parameters.
r = sched_setaffinity(0, sizeof(allowed_mask), &allowed_mask);
it should be something like:
r = sched_setaffinity(getpid(), sizeof(allowed_mask), &allowed_mask);
If we're using sched_setaffinity(0, ...) in the 'migration' thread, the CPU
affinity of 'main' thread won't be affected. It means 'main' thread can be
migrated from one CPU to another at any time, even in the following point:
int main(...)
{
:
/*
* migration can happen immediately after sched_getcpu(). If
* CPU affinity of 'main' thread is sticky to one particular
* CPU, which 'migration' thread supposes to do, then there
* should have no migration.
*/
cpu = sched_getcpu();
rseq_cpu = READ_ONCE(__rseq.cpu_id);
:
}
So I think the correct fix is to have sched_setaffinity(getpid(), ...) ?
Please refer to the manpage.
https://man7.org/linux/man-pages/man2/sched_setaffinity.2.html
'If pid is zero, then the calling thread is used'
Can you check that smp_rmb() and smp_wmb() generate correct instructions on
arm64?
That seems like the most likely scenario (or a kernel bug), I distinctly remember
the barriers provided by tools/ being rather bizarre.
I don't see any problems for smp_rmb() and smp_wmb() in my case. They have
been translated to correct instructions, as expected.
#define smp_mb() asm volatile("dmb ish" ::: "memory")
#define smp_wmb() asm volatile("dmb ishst" ::: "memory")
#define smp_rmb() asm volatile("dmb ishld" ::: "memory")
--------------
One more experiment for sched_setaffinity(). I run the following program,
the CPU affinity of 'main' thread isn't changed, until the correct
parameter is used, to have sched_setaffinity(getpid(), ...).
sched_setaffinity(0, ...)
-------------------------
[root@virtlab-arm01 tmp]# ./a
thread_func: cpu=0
main: mask=0x000000ff
main: mask=0x000000ff
main: mask=0x000000ff
main: mask=0x000000ff
main: mask=0x000000ff
main: mask=0x000000ff
main: mask=0x000000ff
main: mask=0x000000ff
main: mask=0x000000ff
thread_func: cpu=1
main: mask=0x000000ff
main: mask=0x000000ff
main: mask=0x000000ff
main: mask=0x000000ff
main: mask=0x000000ff
main: mask=0x000000ff
main: mask=0x000000ff
main: mask=0x000000ff
main: mask=0x000000ff
main: mask=0x000000ff
:
sched_setaffinity(getpid(), ...)
--------------------------------
thread_func: cpu=198
main: mask=0x00000001
main: mask=0x00000001
main: mask=0x00000001
main: mask=0x00000001
main: mask=0x00000001
main: mask=0x00000001
main: mask=0x00000001
main: mask=0x00000001
main: mask=0x00000001
thread_func: cpu=198
main: mask=0x00000002
main: mask=0x00000002
main: mask=0x00000002
main: mask=0x00000002
main: mask=0x00000002
main: mask=0x00000002
main: mask=0x00000002
main: mask=0x00000002
main: mask=0x00000002
main: mask=0x00000002
:
#define _GNU_SOURCE
#include <stdio.h>
#include <stdlib.h>
#include <sys/types.h>
#include <unistd.h>
#include <pthread.h>
#include <sched.h>
#define NR_CPUS 8
static int thread_exit = 0;
static void *thread_func(void *data)
{
cpu_set_t allowed_mask;
int ret, i;
for (i = 0; i < NR_CPUS; i++) {
CPU_ZERO(&allowed_mask);
CPU_SET(i, &allowed_mask);
#if 1
sched_setaffinity(0, sizeof(allowed_mask), &allowed_mask);
#else
sched_setaffinity(getpid(), sizeof(allowed_mask), &allowed_mask);
#endif
fprintf(stdout, "%s: cpu=%d\n", __func__, sched_getcpu());
sleep(1);
}
thread_exit = 1;
return NULL;
}
int main(int argc, char **argv)
{
pthread_t thread;
cpu_set_t allowed_mask;
int mask, i, count = 0;
pthread_create(&thread, NULL, thread_func, NULL);
while (!thread_exit) {
usleep(100000);
mask = 0;
sched_getaffinity(0, sizeof(allowed_mask), &allowed_mask);
for (i = 0; i < NR_CPUS; i++) {
if (CPU_ISSET(i, &allowed_mask))
mask |= (1 << i);
}
fprintf(stdout, "%s: mask=0x%08x\n", __func__, mask);
}
return 0;
}
Thanks,
Gavin
