Message-ID: <e6523c46-df2e-6f73-1f69-c53e4f86ad50@collabora.com>
Date:   Tue, 22 Nov 2022 13:53:52 +0500
From:   Muhammad Usama Anjum <usama.anjum@...labora.com>
To:     David Hildenbrand <david@...hat.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Cyrill Gorcunov <gorcunov@...il.com>
Cc:     Muhammad Usama Anjum <usama.anjum@...labora.com>,
        Mel Gorman <mgorman@...e.de>, Peter Xu <peterx@...hat.com>,
        Andrei Vagin <avagin@...il.com>, kernel@...labora.com,
        stable@...r.kernel.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm: set the vma flags dirty before testing if it is
 mergeable

On 11/22/22 1:49 PM, Muhammad Usama Anjum wrote:
> On 11/22/22 1:36 PM, David Hildenbrand wrote:
>> On 22.11.22 09:24, Muhammad Usama Anjum wrote:
>>> The VM_SOFTDIRTY should be set in the vma flags to be tested if new
>>> allocation should be merged in previous vma or not. With this patch,
>>> the new allocations are merged in the previous VMAs.
>>>
>>> I've tested it by reverting the commit 34228d473efe ("mm: ignore
>>> VM_SOFTDIRTY on VMA merging") and after adding this following patch,
>>> I'm seeing that all the new allocations done through mmap() are merged
>>> in the previous VMAs. The number of VMAs doesn't increase drastically
>>> which had contributed to the crash of gimp. If I run the same test after
>>> reverting and not including this patch, the number of VMAs keep on
>>> increasing with every mmap() syscall which proves this patch.
>>>
>>> The commit 34228d473efe ("mm: ignore VM_SOFTDIRTY on VMA merging")
>>> seems like a workaround. But it lets the soft-dirty and non-soft-dirty
>>> VMA to get merged. It helps in avoiding the creation of too many VMAs.
>>> But it creates the problem while adding the feature of clearing the
>>> soft-dirty status of only a part of the memory region.
>>>
>>> Cc: <stable@...r.kernel.org>
>>> Fixes: d9104d1ca966 ("mm: track vma changes with VM_SOFTDIRTY bit")
>>> Signed-off-by: Muhammad Usama Anjum <usama.anjum@...labora.com>
>>> ---
>>> We need more testing of this patch.
>>>
>>> While implementing clear soft-dirty bit for a range of address space, I'm
>>> facing an issue. The non-soft dirty VMA gets merged sometimes with the soft
>>> dirty VMA. Thus the non-soft dirty VMA become dirty which is undesirable.
>>> When discussed with the some other developers they consider it the
>>> regression. Why the non-soft dirty page should appear as soft dirty when it
>>> isn't soft dirty in reality? I agree with them. Should we revert
>>> 34228d473efe or find a workaround in the IOCTL?
>>>
>>> * Revert may cause the VMAs to expand in uncontrollable situation where the
>>> soft dirty bit of a lot of memory regions or the whole address space is
>>> being cleared again and again. AFAIK normal process must either be only
>>> clearing a few memory regions. So the applications should be okay. There is
>>> still chance of regressions if some applications are already using the
>>> soft-dirty bit. I'm not sure how to test it.
>>>
>>> * Add a flag in the IOCTL to ignore the dirtiness of VMA. The user will
>>> surely lose the functionality to detect reused memory regions. But the
>>> extraneous soft-dirty pages would not appear. I'm trying to do this in the
>>> patch series [1]. Some discussion is going on that this fails with some
>>> mprotect use case [2]. I still need to have a look at the mprotect selftest
>>> to see how and why this fails. I think this can be implemented after some
>>> more work probably in mprotect side.
>>>
>>> [1]
>>> https://lore.kernel.org/all/20221109102303.851281-1-usama.anjum@collabora.com/
>>> [2]
>>> https://lore.kernel.org/all/bfcae708-db21-04b4-0bbe-712badd03071@redhat.com/
>>> ---
>>>   mm/mmap.c | 21 +++++++++++----------
>>>   1 file changed, 11 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/mm/mmap.c b/mm/mmap.c
>>> index f9b96b387a6f..6934b8f61fdc 100644
>>> --- a/mm/mmap.c
>>> +++ b/mm/mmap.c
>>> @@ -1708,6 +1708,15 @@ unsigned long mmap_region(struct file *file,
>>> unsigned long addr,
>>>           vm_flags |= VM_ACCOUNT;
>>>       }
>>>   +    /*
>>> +     * New (or expanded) vma always get soft dirty status.
>>> +     * Otherwise user-space soft-dirty page tracker won't
>>> +     * be able to distinguish situation when vma area unmapped,
>>> +     * then new mapped in-place (which must be aimed as
>>> +     * a completely new data area).
>>> +     */
>>> +    vm_flags |= VM_SOFTDIRTY;
>>> +
>>>       /*
>>>        * Can we just expand an old mapping?
>>>        */
>>> @@ -1823,15 +1832,6 @@ unsigned long mmap_region(struct file *file,
>>> unsigned long addr,
>>>       if (file)
>>>           uprobe_mmap(vma);
>>>   -    /*
>>> -     * New (or expanded) vma always get soft dirty status.
>>> -     * Otherwise user-space soft-dirty page tracker won't
>>> -     * be able to distinguish situation when vma area unmapped,
>>> -     * then new mapped in-place (which must be aimed as
>>> -     * a completely new data area).
>>> -     */
>>> -    vma->vm_flags |= VM_SOFTDIRTY;
>>> -
>>>       vma_set_page_prot(vma);
>>
>> vma_set_page_prot(vma) has to be called after adjusting vma->vm_flags.
>>
>> Did not look into the details here, but that jumped at me.
> vma_set_page_prot() also needs to be removed from here as it was being
> called after updating the vm_flags. I'll remove it. vma_set_page_prot() was
> added in a separate commit 64e455079e1b. I'll send a v2 in a while.
Just had another look. vm_flags is being modified just above
vma_set_page_prot(). So we don't need to remove it.

-- 
BR,
Muhammad Usama Anjum

Powered by blists - more mailing lists