On 25.07.22 15:59, Peter Xu wrote:
> On Fri, Jul 22, 2022 at 10:21:00AM -0700, Nadav Amit wrote:
>> On Jul 22, 2022, at 12:08 AM, David Hildenbrand <david@xxxxxxxxxx> wrote:
>>
>>>> +static inline bool vma_soft_dirty_enabled(struct vm_area_struct *vma)
>>>> +{
>>>> + /*
>>>> + * NOTE: we must check this before VM_SOFTDIRTY on soft-dirty
>>>> + * enablements, because when without soft-dirty being compiled in,
>>>> + * VM_SOFTDIRTY is defined as 0x0, then !(vm_flags & VM_SOFTDIRTY)
>>>> + * will be constantly true.
>>>> + */
>>>> + if (!IS_ENABLED(CONFIG_MEM_SOFT_DIRTY))
>>>> + return false;
>>>> +
>>>> + /*
>>>> + * Soft-dirty is kind of special: its tracking is enabled when the
>>>> + * vma flags not set.
>>>> + */
>>>> + return !(vma->vm_flags & VM_SOFTDIRTY);
>>>> +}
>>>
>>> That will come in handy in other patches I'm cooking.
>>
>> clear_refs_write() also comes to mind as well (for consistency; I see no
>> correctness or performance issue).
>
> I explicitly didn't touch that because current code is better..
>
> mas_for_each(&mas, vma, ULONG_MAX) {
> if (!(vma->vm_flags & VM_SOFTDIRTY))
> continue;
> vma->vm_flags &= ~VM_SOFTDIRTY;
> vma_set_page_prot(vma);
> }
>
> It means when !CONFIG_MEM_SOFT_DIRTY the "if" will always be true and all
> vma will be jumped.
>
> If replaced with vma_soft_dirty_enabled() it'll be instead constantly false
> returned. We'll redo vma_set_page_prot() even if unnecessary.
>
> Here if we want to add the "CONFIG_MEM_SOFT_DIRTY" into equation it can be:
>
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index f8cd58846a28..ab6f2913b5a5 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -1290,6 +1290,8 @@ static ssize_t clear_refs_write(struct file *file, const char __user *buf,
> }
>
> if (type == CLEAR_REFS_SOFT_DIRTY) {
> + if (!IS_ENABLED(CONFIG_MEM_SOFT_DIRTY))
> + goto out_unlock;
> mas_for_each(&mas, vma, ULONG_MAX) {
> if (!(vma->vm_flags & VM_SOFTDIRTY))
> continue;
>
> Or even at the entrance to not take the mm sem. But it's not anything
> important IMHO, so if no one asking for that I'll just leave it be.
Yeah, I don't think we particularly care about that.
--
Thanks,
David / dhildenb
