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;
References: <a9daea363991c023d0364be22a762405b6c6f5c4.1660281458.git-series.apopple@nvidia.com>
 <8f19b172d32be2e889b837f88b1ba070bf2c97ee.1660281458.git-series.apopple@nvidia.com>
 <1ec090fa-f93b-c197-e5b3-ff2b0d5862ef@redhat.com>
 <87lerqw72n.fsf@nvdebian.thelocal>
 <58be2b37-0c3a-06d8-35f5-50bf4b765fb2@redhat.com>
User-agent: mu4e 1.6.9; emacs 27.1
From:   Alistair Popple <apopple@nvidia.com>
To:     Mika =?utf-8?Q?Penttil=C3=A4?= <mpenttil@redhat.com>
Cc:     linux-mm@kvack.org, akpm@linux-foundation.org,
        linux-kernel@vger.kernel.org,
        "Sierra Guiza, Alejandro (Alex)" <alex.sierra@amd.com>,
        Felix Kuehling <Felix.Kuehling@amd.com>,
        Jason Gunthorpe <jgg@nvidia.com>,
        John Hubbard <jhubbard@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>, linuxram@us.ibm.com,
        paulus@ozlabs.org
Subject: Re: [PATCH 2/2] selftests/hmm-tests: Add test for dirty bits
Date:   Mon, 15 Aug 2022 13:21:56 +1000
In-reply-to: <58be2b37-0c3a-06d8-35f5-50bf4b765fb2@redhat.com>
Message-ID: <87h72ew4p6.fsf@nvdebian.thelocal>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?dERmbS92RUwxZDA1R0QrMDRhZ2wyR1ZYcXZVNVduRDRBdldqSFZtc1ZlYXBJ?=
 =?utf-8?B?YVdacjgyOXlOYnNvMFNLWlh5Wlg1UndYMXRNVnZOcTQ5MjBXbUhJNjFPVXF5?=
 =?utf-8?B?L0REV1pNV3NTZFdHaWd5YkdvRmNLMFMzUUN1aUkveWJtZVJsSlN3UDFLR2Vy?=
 =?utf-8?B?MFN1M2FpSk41bnJVK0pRYW5za1ZGcitleEg4NmgzWFpOMTl5MWdmZ2wxK2lK?=
 =?utf-8?B?NEZFWGttNjB0NFpFd09WMGN2d0hjeFFGNTRkRXhvTWFpdHY5Sis3ZnQ5Q3hj?=
 =?utf-8?B?WlBXM25kNGlTQ3hHVXZlanEyWEd4ZTNQTVBPeTkzeUdqN2t2VUhXMWhvY0RM?=
 =?utf-8?B?MzVIVHhPVEFYa2s3RWtUanlhY1VrZGF2MCthZGJNNmdTaTEwZUZabHlTUjdk?=
 =?utf-8?B?d0FNajFTR1p6ZTBRWUs4K1F2SDVwRCtYRXg5UEtPS3RwWkRGTjNVSzVhcmJl?=
 =?utf-8?B?VE9vaFJtR3BYTHdGRG4wV2VtUjNub1NKR0hycVVkdjQ5SkRocmlKMU5tbnl3?=
 =?utf-8?B?NnU3MjBTdEVEVEkyUGQxZkpZR1psbFhXc3k3ZGVvL1ZOVHM1NitidEM1T295?=
 =?utf-8?B?TURuWGpacHlMcDFvcElPYU5zOWhIandLdnlsdUI1RENHeTlTMnRPQUhpOTJm?=
 =?utf-8?B?NFAyMzZBL24wOE43czlGbmN2cUM4ZnZGLzN4WHU0a3R2OGFKbjBWT3dkMmlq?=
 =?utf-8?B?ODFoSWpidkZxN25NNzlCcDQ4Z2hXaW5KNGN3eTdQL1c0ZHVkbWIvNXg4aTli?=
 =?utf-8?B?Q3Y0dnRyWkNyYjRXUERqL2FBUHI4NFhMWUdidmRRQk5pZlB0M2JrNlVlSVBV?=
 =?utf-8?B?R3lNV3k1TlNCcE9ndXBSeWdsc1JtSFg2cFg1and1WU85YjNJSk1ZQ2h2bGxm?=
 =?utf-8?B?L1RodVpSRElibllwTEVJZm9zTzBRamVRVmVEd3BzWWxTcEh5Sy8wdm9YTVVU?=
 =?utf-8?B?UVYxbzFSM2JGQkQrcnY5ZWpGdTd6WCtzWVZJcHd6VmkrRlVXb20rNURWcnZr?=
 =?utf-8?B?YnZJRUJrdE95RGtkK2s3UjF6bTRCeWpFMVl1T2JiaEJuSktBRmRncWFkNU9V?=
 =?utf-8?B?eURkVUtYQ0YwWXltR3NwZ3YyZmFyS1lBbDZTN1ZVejk2M29MbFZVODg5TDQ3?=
 =?utf-8?B?MWUvNGJ2OFlPK3VFaWFFY015RGRGT0FVdVM1ZW9XUnJqZmMzaDNSQVRJVVlY?=
 =?utf-8?B?Kzd3UXJzZVVZMkM4cm1sWEcwY3Bod0xSRHlwb2c2TGlFWGJ1VTg0R3docWJi?=
 =?utf-8?B?dFJOYWFyNWN1dHBQZVdnbVJjQTh5M3dka2IrbTNOL1RMRERDSDVuenAxcnQ0?=
 =?utf-8?B?d0ZEUytqZi9KK3ZRM3JNc3BzTnNib1R1NHMwd1JybEhYbUM3Yk9zU2tYaFRB?=
 =?utf-8?B?YWt4aVIzV0VHSmlWZXhMU3UwRVdUMkJ3RlhldzF3K2Z1OFNIcFB5M0UyTDQw?=
 =?utf-8?B?amxGYkZNamQ4WGtQb2VSMWVKdXdxRnVFNmFHcjZxVC9hVzZBZ2dRYUpDWTFK?=
 =?utf-8?B?ais0QTZIU0JaS0drbVFEaVFKWGl4ZWxaazNBQWtJVFRrbnFiUkhwOFg4WU5v?=
 =?utf-8?B?Vm5rTU9aV1NRRlVlUllZdDBSVmpYQnhqVVYzd2UwdWhtQ0ZYV2xzZzFCWnpR?=
 =?utf-8?B?MFB0bmMwSWNtNnoyeGgxTXNjcnZCZmRrcitTZ1FZWVU2R1pYcmVudmRCNTF4?=
 =?utf-8?B?U1VuRE51dGpFUVNaZFJBWjczWHhaNkpqcUxYUHpHcGhJdUJPeHBqK29OZ3pV?=
 =?utf-8?B?WlAveDIyWUJ6aDgxY1ZMRFNERWVNSGpSLzdmYzZCdzZJR01jUUc1eEl5NEw2?=
 =?utf-8?B?N3dNY0JnSVh1ZG1ST1hIU3c2WmJrRmwwNmphR3lZdEYzcnF1ejg1bloycW1O?=
 =?utf-8?B?MXdnZ0JlK0xkZHdseU01am9vTG1sVkw2Y1doVzdKeFVDd2pueWUrSW9NaDRV?=
 =?utf-8?B?eG9aN1lPU05jdS81SFZac1hoVkhVYzl4TlZsTjNGQVFrVGNGclhZejVGL2pM?=
 =?utf-8?B?aUVUR3N0L3hRMFV1WUxsNXRJaVFmV29aYTQwZnRwQnR0ZVBraFo4ckFjTHVK?=
 =?utf-8?B?S2FoWVJ0UWo1dmp0cnZyS3VJdXpCQTIxMWNkU3lDRmhQeXI0WE51SWJ6NGl4?=
 =?utf-8?Q?JA6yP9Jl4YmmwhslwS3IuID5p?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 00f99bdb-8fba-4469-3c63-08da7e70852e
X-MS-Exchange-CrossTenant-AuthSource: BYAPR12MB3176.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Aug 2022 03:44:56.4370
 (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: xpuZqtbdHQoa9vGij/RfyNuxhLwI4uz9T6EHJHKjWULIe9flwrWV886cGK3BnaZ5i1yhIBz87huBOu1XAA+P5g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR12MB5510
X-Spam-Status: No, score=-2.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        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


Mika Penttil=C3=A4 <mpenttil@redhat.com> writes:

> On 15.8.2022 5.35, Alistair Popple wrote:
>> Mika Penttil=C3=A4 <mpenttil@redhat.com> writes:
>>
>>> Hi Alistair!
>>>
>>> On 12.8.2022 8.22, Alistair Popple wrote:
>> [...]
>>
>>>> +	buffer->ptr =3D 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 =3D 0, ptr =3D buffer->ptr; i < size / sizeof(*ptr); ++i)
>>>> +		ptr[i] =3D 0;
>>>> +
>>>> +	ASSERT_FALSE(write_cgroup_param(cgroup, "memory.reclaim", 1UL<<30));
>>>> +
>>>> +	/* Fault pages back in from swap as clean pages */
>>>> +	for (i =3D 0, ptr =3D buffer->ptr; i < size / sizeof(*ptr); ++i)
>>>> +		tmp +=3D ptr[i];
>>>> +
>>>> +	/* Dirty the pte */
>>>> +	for (i =3D 0, ptr =3D buffer->ptr; i < size / sizeof(*ptr); ++i)
>>>> +		ptr[i] =3D i;
>>>> +
>>>
>>> The anon pages are quite likely in memory at this point, and dirty in p=
te.
>> Why would the pte be dirty? I just confirmed using some modified pagemap
>> code that on my system at least this isn't the case.
>>
>>>> +	/*
>>>> +	 * 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));
>>>
>>> And pages marked dirty also now. But could you elaborate how and where =
the above
>>> fails in more detail, couldn't immediately see it...
>> Not if you don't have patch 1 of this series applied. If the
>> trylock_page() in migrate_vma_collect_pmd() succeeds (which it almost
>> always does) it will have cleared the pte without setting PageDirty.
>>
>
> Ah yes but I meant with the patch 1 applied, the comment "Attempt to migr=
ate
> memory to device, which should fail because hopefully some pages are back=
ed by
> swap storage" indicates that hmm_migrate_sys_to_dev() would fail..and the=
re's
> that ASSERT_TRUE which means fail here.
>
> So I understand the data loss but where is the hmm_migrate_sys_to_dev() f=
ailing,
> with or wihtout patch 1 applied?

Oh right. hmm_migrate_sys_to_dev() will fail because the page is in the
swap cache, and migrate_vma_*() doesn't currently support migrating
pages with a mapping.

>> So now we have a dirty page without PageDirty set and without a dirty
>> pte. If this page gets swapped back to disk and is still in the swap
>> cache data will be lost because reclaim will see a clean page and won't
>> write it out again.
>> At least that's my understanding - please let me know if you see
>> something that doesn't make sense.
>>
>>>> +
>>>> +	ASSERT_FALSE(write_cgroup_param(cgroup, "memory.reclaim", 1UL<<30));
>>>> +
>>>> +	/* Check we still see the updated data after restoring from swap. */
>>>> +	for (i =3D 0, ptr =3D buffer->ptr; i < size / sizeof(*ptr); ++i)
>>>> +		ASSERT_EQ(ptr[i], i);
>>>> +
>>>> +	hmm_buffer_free(buffer);
>>>> +	destroy_cgroup();
>>>> +}
>>>> +
>>>>    /*
>>>>     * Read anonymous memory multiple times.
>>>>     */
>>>
>>>
>>> --Mika
>>