Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 1/1] selftests: KVM: set affinity of VM to right CPUs
To:     Sean Christopherson <seanjc@google.com>
Cc:     kvm@vger.kernel.org, linux-kselftest@vger.kernel.org,
        pbonzini@redhat.com, shuah@kernel.org, linux-kernel@vger.kernel.org
References: <20210924233037.4329-1-dongli.zhang@oracle.com>
 <YVIZ/67cfjk18mbe@google.com>
From:   Dongli Zhang <dongli.zhang@oracle.com>
Message-ID: <5b0a16a9-e98e-368f-4ecd-359c58ae34c4@oracle.com>
Date:   Tue, 28 Sep 2021 10:37:03 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.6.1
In-Reply-To: <YVIZ/67cfjk18mbe@google.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?WVZveWZ6T0NJM0hsei9JVDlNMGx3T3VhTjQrUlVLc1VJRmFISVhrSWVZWngx?=
 =?utf-8?B?Ym1QYzFKdGpSRE1uVFhVaEQ5RHhDVjhWK003YUtuU3J4dUZWa0pldWk4U2U3?=
 =?utf-8?B?ejZRNlJhUWFGS3ozY2lKRytZZlNVb1dVVTJ1Ums5cmVzM1dNUUxERlR3Z1Iz?=
 =?utf-8?B?WGliSlRoRDl0dDBEcjNMVDd2QWFzNjZnK1QxY1dFQjNCRFV1dnFtamRnSzN6?=
 =?utf-8?B?bWd0dS8venhQL2JFQnRjSXZ6NDF0Qmg0QnEzVUNZK2NwOWgwTFhMMDBhNUJF?=
 =?utf-8?B?WmxTKy90bGw4cUx6aWlLSW14bGhRRzVUVFVoM2l6TkxLOURqamgvdUNjdTUx?=
 =?utf-8?B?WTlUczFuQ3IwNVlrWnFTYUNhVTVYbysvbEkzNnhWZUZ5VTAvcHV3STU4dm5w?=
 =?utf-8?B?SzlCLzlIUmZVS2ZtaFNPVWIzNzdDY2pUYVNMaEZnUlIyU0s0bjBwL0lML01H?=
 =?utf-8?B?ZE5uMUVKeEtFRC9ldDF1SHd2aDh6aGxSMWVvWTlic0FtU0hBS2x0S25QK2g5?=
 =?utf-8?B?aEQxeStveU1HYTZlM2drdVhPeVQyUkdhNVlKME5aVzdTZVkrVFg2YnBXSm5X?=
 =?utf-8?B?MGhhNkhOV0xPVWZwR1J4RDF5cVU5dDhZbDlqSTNPUE1ZUVNOQTRHc0F5Q0t6?=
 =?utf-8?B?YWs1cFZDeEtlNUNUN1Y1YjQxUFJNbEQ0bDRkeUVjYmFsK09Ob0F3czEyNXMw?=
 =?utf-8?B?N0kwYmhyQ0JlM2VoZWtkSmJhSkRkR1ZUNzlvb00xZ3EvMUE0T1k1eWxaYTJ5?=
 =?utf-8?B?U1FSUDNCT0JtQ2NIZjZmTWp0THJkZ21kR2c3S01JQmxJbUpNbWk4SWpUajB5?=
 =?utf-8?B?NkV5Uk1SbjZja1UxWVdRMVkzV21MZ2UzOXNPVG5qaHloU2c3VW9nWVBXOGZv?=
 =?utf-8?B?Z29CT29BZERpQW4rb2l1c3UrR29LY0Q1M0ttV2h1ampydUdwVVZSazhKUGV2?=
 =?utf-8?B?MHlJTGdCZmVwOXZZeGFIQUVpTWgzakt5WWNBVzdGU3EwdG94d3ZwM3RLUnQ1?=
 =?utf-8?B?M00zZDZkSEdyUHZQQURKdTBLdG5yNDhWVXVKZmd1cTN5bXVZbEN3V1dGY29k?=
 =?utf-8?B?UlBzUjBoUkMrU3cwQ2FtOE9rc0Rnck1XbXFsblRtUUd3N1ZhWlNJbGtpWmRk?=
 =?utf-8?B?ZFR5ZmVLeHRRY3g3SmRXdFZVMmhwK0ZRV0hVRG9qRVhxTU92WmNzSk5sSFpu?=
 =?utf-8?B?NTRlREM0NzVJbjcrVVVlaE55aUFyS2FGaTN3c0xneGM1aSs0MkhMNy80Y25i?=
 =?utf-8?B?Yi9vbnFVWDdiaWE3UGs0b1pXeUlGZ2hKVVc5Uzh3aDhDNWtlL3IwSmdnRkVM?=
 =?utf-8?B?dUd4OVdjdHhuS0lBdkZ4YVc2dnNjS1RNSityejVLKzM4ckNpTVVpb1NDM3Rs?=
 =?utf-8?B?TDlSdDJvT29SWTFRVEJCOG16VkRaRHFsVEhDcXh1RW1YSU5VZlQrQWpQMEJY?=
 =?utf-8?B?SXN5NitDNEpzdHh3OCtBaVdmNk5hOVlQQVhub2I3eDl5UmVvY1g2QnZWcjhW?=
 =?utf-8?B?d2g5elZxSUNTV0s3TnVTOVplNlJXYmlPeG9CVHFqRnM0VmgvQ1lwUkM4SkRq?=
 =?utf-8?B?RGx2M2owdHB1ek9xcnAybkNWNG1EMTA3ODF5SlJablNYbng3YkplMzQyVFY4?=
 =?utf-8?B?cFNsVFFtdytiZUUxUGU0R2ZKdXZZUDBsWDJkUUVUdUZEdDdicUtwYmR0YWZC?=
 =?utf-8?B?MUJuS0x6VDRBZUd6b0tjcGlnc0prdGw2bU1mWnhQcHNzcFBMaWl0S0xIaDAy?=
 =?utf-8?B?UWV3THNLMUllSzV1U29rK2lkQVlVdEJKS3JOaGpGYmg2OGRZWkswcWlTSjJy?=
 =?utf-8?B?MlVpd1lRVGw5TWVZaUNoQT09?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 853a00a6-79cd-449c-a74f-08d982a6972a
X-MS-Exchange-CrossTenant-AuthSource: BYAPR10MB2663.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Sep 2021 17:37:06.3142
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: NYody7DZnse6YxuRUaspPaUdebGqF5D+wtM9sM3wmpTEooZetexRwGbBEmR8ol26+r2OCEBpKEFPjEwgfGvXXg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR10MB3543
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10121 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 phishscore=0 mlxscore=0
 mlxlogscore=999 malwarescore=0 spamscore=0 bulkscore=0 suspectscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2109230001
 definitions=main-2109280103
X-Proofpoint-ORIG-GUID: AzoJEBrVERypL4LeqZwSOUnpcI2LI00T
X-Proofpoint-GUID: AzoJEBrVERypL4LeqZwSOUnpcI2LI00T
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 9/27/21 12:22 PM, Sean Christopherson wrote:
> On Fri, Sep 24, 2021, Dongli Zhang wrote:
>> The nr_cpus = CPU_COUNT(&possible_mask) is the number of available CPUs in
>> possible_mask. As a result, the "cpu = i % nr_cpus" may always return CPU
>> that is not available in possible_mask.
>>
>> Suppose the server has 8 CPUs. The below Failure is encountered immediately
>> if the task is bound to CPU 5 and 6.
> 
> /facepalm
> 
>> ==== Test Assertion Failure ====
>>   rseq_test.c:228: i > (NR_TASK_MIGRATIONS / 2)
>>   pid=10127 tid=10127 errno=4 - Interrupted system call
>>      1	0x00000000004018e5: main at rseq_test.c:227
>>      2	0x00007fcc8fc66bf6: ?? ??:0
>>      3	0x0000000000401959: _start at ??:?
>>   Only performed 4 KVM_RUNs, task stalled too much?
>>
>> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
>> ---
>>  tools/testing/selftests/kvm/rseq_test.c | 18 +++++++++++++++++-
>>  1 file changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/testing/selftests/kvm/rseq_test.c b/tools/testing/selftests/kvm/rseq_test.c
>> index c5e0dd664a7b..41df5173970c 100644
>> --- a/tools/testing/selftests/kvm/rseq_test.c
>> +++ b/tools/testing/selftests/kvm/rseq_test.c
>> @@ -10,6 +10,7 @@
>>  #include <signal.h>
>>  #include <syscall.h>
>>  #include <sys/ioctl.h>
>> +#include <sys/sysinfo.h>
>>  #include <asm/barrier.h>
>>  #include <linux/atomic.h>
>>  #include <linux/rseq.h>
>> @@ -43,6 +44,18 @@ static bool done;
>>  
>>  static atomic_t seq_cnt;
>>  
>> +static int get_max_cpu_idx(void)
>> +{
>> +	int nproc = get_nprocs_conf();
>> +	int i, max = -ENOENT;
>> +
>> +	for (i = 0; i < nproc; i++)
>> +		if (CPU_ISSET(i, &possible_mask))
>> +			max = i;
>> +
>> +	return max;
>> +}
>> +
>>  static void guest_code(void)
>>  {
>>  	for (;;)
>> @@ -61,10 +74,13 @@ static void *migration_worker(void *ign)
>>  {
>>  	cpu_set_t allowed_mask;
>>  	int r, i, nr_cpus, cpu;
>> +	int max_cpu_idx;
>>  
>>  	CPU_ZERO(&allowed_mask);
>>  
>> -	nr_cpus = CPU_COUNT(&possible_mask);
>> +	max_cpu_idx = get_max_cpu_idx();
>> +	TEST_ASSERT(max_cpu_idx >= 0, "Invalid possible_mask");
> 
> I feel like this should be a KSFT_SKIP condition, not an assert.
> 
>> +	nr_cpus = max_cpu_idx + 1;
>>  
>>  	for (i = 0; i < NR_TASK_MIGRATIONS; i++) {
>>  		cpu = i % nr_cpus;
> 
> This is still flawed, e.g. if the max CPU is 1023, but the task is pinned to _just_
> CPU 1023, then the assert at the end will likely still fail because the migration
> helper is effectively only running 1/1024 loops.
> 
> It probably also makes sense to grab the min CPU to reduce the pain if the task
> is affined to a small subset.
> 
> As an aside, _which_ CPUs the task is affined to seems to matter, e.g. some
> combinations of CPUs on my system don't fail, even with 100x iterations.  Don't
> think there's anything the test can do about that, just an interesting data point
> that suggests pinning while running tests may be a bad idea.

I agree sometimes the failure is expected and reasonable. E.g., I am able to
randomly reproduce the failure if I reduce NR_TASK_MIGRATIONS to 1000.

> 
> Anyways, something like this?
> 
> ---
>  tools/testing/selftests/kvm/rseq_test.c | 44 ++++++++++++++++++++-----
>  1 file changed, 36 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/rseq_test.c b/tools/testing/selftests/kvm/rseq_test.c
> index 060538bd405a..befd64c27152 100644
> --- a/tools/testing/selftests/kvm/rseq_test.c
> +++ b/tools/testing/selftests/kvm/rseq_test.c
> @@ -10,6 +10,7 @@
>  #include <signal.h>
>  #include <syscall.h>
>  #include <sys/ioctl.h>
> +#include <sys/sysinfo.h>
>  #include <asm/barrier.h>
>  #include <linux/atomic.h>
>  #include <linux/rseq.h>
> @@ -39,6 +40,7 @@ static __thread volatile struct rseq __rseq = {
> 
>  static pthread_t migration_thread;
>  static cpu_set_t possible_mask;
> +static int min_cpu, max_cpu;
>  static bool done;
> 
>  static atomic_t seq_cnt;
> @@ -60,16 +62,17 @@ static void sys_rseq(int flags)
>  static void *migration_worker(void *ign)
>  {
>  	cpu_set_t allowed_mask;
> -	int r, i, nr_cpus, cpu;
> +	int r, i, cpu;
> 
>  	CPU_ZERO(&allowed_mask);
> 
> -	nr_cpus = CPU_COUNT(&possible_mask);
> -
> -	for (i = 0; i < NR_TASK_MIGRATIONS; i++) {
> -		cpu = i % nr_cpus;
> -		if (!CPU_ISSET(cpu, &possible_mask))
> -			continue;
> +	for (i = 0, cpu = -1; i < NR_TASK_MIGRATIONS; i++) {
> +		do {
> +			if (cpu < min_cpu || cpu > max_cpu)
> +				cpu = min_cpu;
> +			else
> +				cpu++;
> +		} while (!CPU_ISSET(cpu, &possible_mask));
> 
>  		CPU_SET(cpu, &allowed_mask);

I see. The idea is to search within between min_cpu and max_cpu for each
NR_TASK_MIGRATION iteration to find the next cpu.

Perhaps a linked list is more suitable to here (when there are 1024 cpus and the
task is bound to both 1 and 1022) ... to pre-save the possible cpus in a list
and to only move to next cpu in the list for each iteration.

However, I think min_cpu/max_cpu is good enough for selttests case.

Would you please let me know if you would like to send above with my
Reported-by, or if you would like me to send with your Suggested-by.

Thank you very much!

Dongli Zhang