Re: [PATCH v4 1/3] mm/mprotect: Fix soft-dirty check in can_change_pte_writable()

[Muhammad Usama Anjum <usama.anjum@collabora.com>]

On 12/21/22 12:02 AM, Peter Xu wrote:
> On Tue, Dec 20, 2022 at 11:15:00PM +0500, Muhammad Usama Anjum wrote:
>> Hi Peter,
> 
> Hi, Muhammad,
> 
> [...]
> 
>> Nothing has changed for the userspace. But when the default soft-dirty
>> feature always updates the soft-dirty flag in the PTEs regardless of
>> VM_SOFTDIRTY is set or not
> 
> But it was not?  I don't see why the pte flags must be considered at all if
> VM_SOFTDIRTY is set in existing code base, or before this patch.
I completely agree that setting pte flags isn't needed if VM_SOFTDIRTY is
set. It is wasted effort. Before this patch, the effort was being wasted in
the default part of the feature and in the other related components as
well. After this patch, the effort is being wasted only in the default part
of the feature and other related components have stopped doing this wasted
effort which is a good thing. But now there is discrepancy that one part of
code keeps on tracking pte while other has moved on/improved.

> 
>> why does other components of the mm stop caring for soft-dirty flag in
>> the PTE when VM_SOFTDIRTY is set?
>>
>>>
>>> Your approach introduced PAGEMAP_NO_REUSED_REGIONS but that special
>>> information is not remembered in vma, IIUC that's why you find things
>>> messed up.  Fundamentally, it's because you're trying to reuse soft-dirty
>>> design but it's not completely soft-dirty anymore.
>> Correct, that's why I'm trying to find a way to correct the soft-dirty
>> support instead of using anything else. We should try and correct it. I've
>> sent a RFC to track the soft-dirty flags for sub regions in the VMA.
> 
> Note that I'm not against the change if that's servicing the purpose of the
> enhancement you're proposing.  But again I wouldn't use "correct" as the
> word here because I still didn't see anything wrong with the old code.
> 
> so IMHO the extra complexity on handling VM_SOFTDIRTY (even if to drop it
> and replace with other structures to maintain ranged soft-dirty) needs to
> be justified along with the feature introduced, not be justified as a fix.
The question of tracking re-used regions should be answered by the original
developers of the feature. I've been trying to get comments from them. But
I've not got any. Maybe some conference talk can work where the
maintainers/developers are present? Or I'll summarize the whole problem and
ask Andrew directly.

> 
> Thanks,
> 

-- 
BR,
Muhammad Usama Anjum