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: <490e670d-40fe-6284-04d9-9be8fd15d49f@redhat.com>
Date:   Mon, 15 Aug 2022 07:06:25 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH 2/2] selftests/hmm-tests: Add test for dirty bits
Content-Language: en-US
From:   =?UTF-8?Q?Mika_Penttil=c3=a4?= <mpenttil@redhat.com>
To:     Alistair Popple <apopple@nvidia.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
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>
 <87h72ew4p6.fsf@nvdebian.thelocal>
 <2aa2013a-735d-a96a-2f35-0a44a06d85f0@redhat.com>
In-Reply-To: <2aa2013a-735d-a96a-2f35-0a44a06d85f0@redhat.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 15.8.2022 7.05, Mika Penttilä wrote:
> 
> 
> On 15.8.2022 6.21, Alistair Popple wrote:
>>
>> Mika Penttilä <mpenttil@redhat.com> writes:
>>
>>> On 15.8.2022 5.35, Alistair Popple wrote:
>>>> Mika Penttilä <mpenttil@redhat.com> writes:
>>>>
>>>>> Hi Alistair!
>>>>>
>>>>> On 12.8.2022 8.22, Alistair Popple wrote:
>>>> [...]
>>>>
>>>>>> +    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;
>>>>>> +
>>>>>
>>>>> The anon pages are quite likely in memory at this point, and dirty 
>>>>> in pte.
>>>> 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 
>>> migrate
>>> memory to device, which should fail because hopefully some pages are 
>>> backed by
>>> swap storage" indicates that hmm_migrate_sys_to_dev() would fail..and 
>>> there's
>>> that ASSERT_TRUE which means fail here.
>>>
>>> So I understand the data loss but where is the 
>>> hmm_migrate_sys_to_dev() failing,
>>> 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.
>>
> 
> Ok I forgot we skip also page cache pages, not just file pages...

Meant we skip swap cache pages also, not just file pages..


> 
> 
> 
> 
>>>> 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 = 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.
>>>>>>      */
>>>>>
>>>>>
>>>>> --Mika
>>>>
>>