Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <f81f6f90-81ad-7b77-a74d-e9d5d6e3bc94@nvidia.com>
Date:   Sun, 4 Sep 2022 17:41:07 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.2.0
Subject: Re: [PATCH v4 4/4] selftests/hmm-tests: Add test for dirty bits
Content-Language: en-US
To:     Alistair Popple <apopple@nvidia.com>, linux-mm@kvack.org,
        akpm@linux-foundation.org
Cc:     Peter Xu <peterx@redhat.com>, Nadav Amit <nadav.amit@gmail.com>,
        huang ying <huang.ying.caritas@gmail.com>,
        LKML <linux-kernel@vger.kernel.org>,
        "Sierra Guiza, Alejandro (Alex)" <alex.sierra@amd.com>,
        Felix Kuehling <Felix.Kuehling@amd.com>,
        Jason Gunthorpe <jgg@nvidia.com>,
        David Hildenbrand <david@redhat.com>,
        Ralph Campbell <rcampbell@nvidia.com>,
        Matthew Wilcox <willy@infradead.org>,
        Karol Herbst <kherbst@redhat.com>,
        Lyude Paul <lyude@redhat.com>, Ben Skeggs <bskeggs@redhat.com>,
        Logan Gunthorpe <logang@deltatee.com>, paulus@ozlabs.org,
        linuxppc-dev@lists.ozlabs.org
References: <9f801e9d8d830408f2ca27821f606e09aa856899.1662078528.git-series.apopple@nvidia.com>
 <68cf1d70f3fb8ce4e3c1a4899c19df4f6c382a13.1662078528.git-series.apopple@nvidia.com>
From:   John Hubbard <jhubbard@nvidia.com>
In-Reply-To: <68cf1d70f3fb8ce4e3c1a4899c19df4f6c382a13.1662078528.git-series.apopple@nvidia.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?aUNsem96MWozSktQcVBJRnczZzZJeUR0NFNOcituQ1dYTWRYWk1EM1o4alRG?=
 =?utf-8?B?cnVFeEQzNTF3cmxsMTJQU2s2bjJERG5Wb21WdzIrWHZkNHBKWUhIcXVYRFJG?=
 =?utf-8?B?bnlWM3FCU3JYcUxMc0VTM3pCendhbWxzdis1QUJJOGZoc3YwWFVFNTAxSlpx?=
 =?utf-8?B?SWw5RC84R3piUXBTSktDSlgvZG5ZalRjc3NKS0tZRjJFSjl5WGNZTFFBNDJs?=
 =?utf-8?B?bUFFc2pPdkNnTDRGbzNFS283VVBMMVNpZzArUXpkdnIyMWJrMTJVNE9vb3RH?=
 =?utf-8?B?MXpvMVdTeno5QXVVcEIvSWxYc3dyMktCZWErK3ZVTVlXNmFPZXJhZmFqQnRw?=
 =?utf-8?B?SEI3Qkw5dXFvYVZKZlhKUHFEMndzMVBaNklRTUhXdklKQXJnVUxVNjdLU3RO?=
 =?utf-8?B?anQwYVJFZVdxazllTzJHcWxmb2dMeWszS0J3V0wwSmVSVkcrNm5TNTlmTXZ3?=
 =?utf-8?B?UE0weFRRcVdMSFN6Y1RyNkd2K01XY29CN0trYnVPS1RFTjBRSTVvcUZUbFdE?=
 =?utf-8?B?ODFQeDVHK09HakZnK2JkcGk1VFZsc1ZOU3cyWmRYcVBCdjRsZk5ITkZRemV3?=
 =?utf-8?B?MVZXb2REcGJ5cDFEM3QxV0ptOVMxTjlVV1h2dHZTME1ULzk5d2ZjdU4rbG1k?=
 =?utf-8?B?OWdsTTVhSkZmZElzNjNzTms2TWs0bHdabzZTNkJvbGYwNmtmRkdPTHZybEl6?=
 =?utf-8?B?QWJzS3p3Y3BibXFiZ3VuVDQ5SmV5UEhnK1FBSVNKT0tzNTZnbFRtSllmbklB?=
 =?utf-8?B?bDJLTzkvN08xQW1OK0d0ZEZ3SHVzNS9nMXIxT0pBV1h2SjdEUzhVaGtnUWk2?=
 =?utf-8?B?NHE1ajhLMDc4M29FMEU0bFpIUmFsOG1DZ0JGSU1KZFJ1Z0ZWSjY1c2VRYzVS?=
 =?utf-8?B?aEJyRzZDV2xKMWhsL0ZEZXZUOGh3Y1F3VGcvOHIrTitaMnpkK2VBcVpFVEhI?=
 =?utf-8?B?S0VSMXpsdm8vTDlOVy82Mkl1a1pVRElqVWg3bTNRQmY3K0dSbXNZcUlIOEho?=
 =?utf-8?B?UEpNNlZ1M1hJdFJnZ28rUXNPK0RaNWg4WHYxb3VhMm8xTDNKUEtFcC9zWXpC?=
 =?utf-8?B?dUpwNHFQeEtkSWlJVk1rRExBSjUrWmlWZW1wRS9uYkZ4ak5uYUVTOHFoRlpC?=
 =?utf-8?B?MG1QM2cyc0MxSHRtc2pLNXpGMWVCWlFua1FJbFl0VytWbVJUNmYwRXErUHJB?=
 =?utf-8?B?aEtqNC9FUFdTK2REZ214ZUx5L2lldlFGaVFBb0FTT3RIbHUvWlh5ZExReXFQ?=
 =?utf-8?B?c0lmZVlSdHczd2RuZkV5SVhwa2JRcnlnQXZQbXVQMml4N0xNR0xSdGl6NUdW?=
 =?utf-8?B?RGVxZVhtNVNvajkycDdVRlVtcWduWkFxd2hJazMxbHdobitQbW1mTjc4R3ky?=
 =?utf-8?B?dGd6NTh1aUZsckc4SUdjWGt1ZDlhcVQvQVlMcmt5U01yRUR4KzU0Tk0raUlm?=
 =?utf-8?B?OGNhRjlYTFFKbXZHTEswY3hFaTkxYXdtZnhobHk2Vkk0NWovZEIyeHNydWxO?=
 =?utf-8?B?cjJMWkJ2RS9RZXRmTk5jY004RDFIZHlqN0M5aHN4VzJzM3pjTE9rZkVLVVlm?=
 =?utf-8?B?WVRpSWVVT055OXRydzIwVTZhRVJlZDFmbnFwUWZhSGszSlZwbDhnTS9QZmg4?=
 =?utf-8?B?akZJeHI2WnFuUTFSWnVES1pkTGdUT2UrNUE2Q3ZzNm5xUEZ5alk2eEhmaktG?=
 =?utf-8?B?QUdtNnlrelg4b0NpbUI5TzJCaWhJcGFZL0xQaWpsbXRhYVkra29UaHEyQ0ND?=
 =?utf-8?B?WncrQjRMSGlZOTJleDhXc3ljcm1hUmJkcnFOdlZNOS9MellPWHBxMll0dFI5?=
 =?utf-8?B?Mm9XTGx4SHdwSExpZnRmRjNzL3BuLytrMFV1K0ViM3JpUWpDUjZiYzlaMEI1?=
 =?utf-8?B?SzhXTmtHWlRGUkhuUjFSbndQY3JwM1VVaERoL04wVU1CN05jRWxiMTkvMjRF?=
 =?utf-8?B?dkFMWWtVTCt4dlp0bG9uK1o2UTB6akJhb1JJSmY5MkdkVXdVRENFcjQwLzc4?=
 =?utf-8?B?bEJ5RWdoZ0NMR2FVRFoyMkd5LzU1ZWllNVM5bjE2RURBSVpoYkJLOEVmbUlp?=
 =?utf-8?B?dFE0Vm5RL3JReWhjQ3FZYUwrZVg1MGI4Z2tQN3FRbTRwaVFBSWtEemZnTGkx?=
 =?utf-8?Q?m7VtWOV+TyMuZFM0d8Gv1063B?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 9ea8ea1b-eef2-4adb-3729-08da8ed753e8
X-MS-Exchange-CrossTenant-AuthSource: BY5PR12MB4130.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Sep 2022 00:41:10.5373
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: u8bSeMJNMNAPnCXjjQLlj5l+lMFqQkGuW2v4SX82aTZ/sADA0UeoeY3nrqBZmNf9hmOjvvODbMlC3LYYWzzAOQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR12MB6968
X-Spam-Status: No, score=-2.3 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,
        SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 9/1/22 17:35, Alistair Popple wrote:
> We were not correctly copying PTE dirty bits to pages during
> migrate_vma_setup() calls. This could potentially lead to data loss, so
> add a test for this.
> 
> Signed-off-by: Alistair Popple <apopple@nvidia.com>
> ---
>  tools/testing/selftests/vm/hmm-tests.c | 124 ++++++++++++++++++++++++++-
>  1 file changed, 124 insertions(+)
> 
> diff --git a/tools/testing/selftests/vm/hmm-tests.c b/tools/testing/selftests/vm/hmm-tests.c
> index 529f53b..70fdb49 100644
> --- a/tools/testing/selftests/vm/hmm-tests.c
> +++ b/tools/testing/selftests/vm/hmm-tests.c
> @@ -1200,6 +1200,130 @@ TEST_F(hmm, migrate_multiple)
>  	}
>  }
>  
> +static char cgroup[] = "/sys/fs/cgroup/hmm-test-XXXXXX";
> +static int write_cgroup_param(char *cgroup_path, char *param, long value)
> +{
> +	int ret;
> +	FILE *f;
> +	char *filename;
> +
> +	if (asprintf(&filename, "%s/%s", cgroup_path, param) < 0)
> +		return -1;
> +
> +	f = fopen(filename, "w");
> +	if (!f) {
> +		ret = -1;
> +		goto out;
> +	}
> +
> +	ret = fprintf(f, "%ld\n", value);
> +	if (ret < 0)
> +		goto out1;
> +
> +	ret = 0;
> +
> +out1:
> +	fclose(f);
> +out:
> +	free(filename);
> +
> +	return ret;
> +}
> +
> +static int setup_cgroup(void)
> +{
> +	pid_t pid = getpid();
> +	int ret;
> +
> +	if (!mkdtemp(cgroup))
> +		return -1;
> +
> +	ret = write_cgroup_param(cgroup, "cgroup.procs", pid);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int destroy_cgroup(void)
> +{
> +	pid_t pid = getpid();
> +	int ret;
> +
> +	ret = write_cgroup_param("/sys/fs/cgroup/cgroup.procs",
> +				"cgroup.proc", pid);
> +	if (ret)
> +		return ret;
> +
> +	if (rmdir(cgroup))
> +		return -1;
> +
> +	return 0;
> +}
> +
> +/*
> + * Try and migrate a dirty page that has previously been swapped to disk. This
> + * checks that we don't loose dirty bits.

s/loose/lose/

> + */
> +TEST_F(hmm, migrate_dirty_page)
> +{
> +	struct hmm_buffer *buffer;
> +	unsigned long npages;
> +	unsigned long size;
> +	unsigned long i;
> +	int *ptr;
> +	int tmp = 0;
> +
> +	npages = ALIGN(HMM_BUFFER_SIZE, self->page_size) >> self->page_shift;
> +	ASSERT_NE(npages, 0);
> +	size = npages << self->page_shift;
> +
> +	buffer = malloc(sizeof(*buffer));
> +	ASSERT_NE(buffer, NULL);
> +
> +	buffer->fd = -1;
> +	buffer->size = size;
> +	buffer->mirror = malloc(size);
> +	ASSERT_NE(buffer->mirror, NULL);
> +
> +	ASSERT_EQ(setup_cgroup(), 0);
> +
> +	buffer->ptr = mmap(NULL, size,
> +			   PROT_READ | PROT_WRITE,
> +			   MAP_PRIVATE | MAP_ANONYMOUS,
> +			   buffer->fd, 0);
> +	ASSERT_NE(buffer->ptr, MAP_FAILED);
> +
> +	/* Initialize buffer in system memory. */
> +	for (i = 0, ptr = buffer->ptr; i < size / sizeof(*ptr); ++i)
> +		ptr[i] = 0;
> +
> +	ASSERT_FALSE(write_cgroup_param(cgroup, "memory.reclaim", 1UL<<30));
> +
> +	/* Fault pages back in from swap as clean pages */
> +	for (i = 0, ptr = buffer->ptr; i < size / sizeof(*ptr); ++i)
> +		tmp += ptr[i];
> +
> +	/* Dirty the pte */
> +	for (i = 0, ptr = buffer->ptr; i < size / sizeof(*ptr); ++i)
> +		ptr[i] = i;
> +
> +	/*
> +	 * Attempt to migrate memory to device, which should fail because
> +	 * hopefully some pages are backed by swap storage.
> +	 */
> +	ASSERT_TRUE(hmm_migrate_sys_to_dev(self->fd, buffer, npages));

Are you really sure that you want to assert on that? Because doing so
guarantees a test failure if and when we every upgrade the kernel to
be able to migrate swap-backed pages. And I seem to recall that this
current inability to migrate swap-backed pages is considered a flaw
to be fixed, right?


> +
> +	ASSERT_FALSE(write_cgroup_param(cgroup, "memory.reclaim", 1UL<<30));
> +
> +	/* Check we still see the updated data after restoring from swap. */
> +	for (i = 0, ptr = buffer->ptr; i < size / sizeof(*ptr); ++i)
> +		ASSERT_EQ(ptr[i], i);
> +
> +	hmm_buffer_free(buffer);
> +	destroy_cgroup();
> +}
> +
>  /*
>   * Read anonymous memory multiple times.
>   */

thanks,

-- 
John Hubbard
NVIDIA