Re: [PATCH v8 1/3] ksm: add linear_page_index into ksm_rmap_item

[xu.xin16]

>> +/*
>> + * break_cow: actively break COW, replacing the KSM page by a fresh anonymous
>> + * page. This is called when rmap_item has not yet become stable, but page
>> + * has been merged.
>> + */
>>  static void break_cow(struct ksm_rmap_item *rmap_item)
>>  {
>>  	struct mm_struct *mm = rmap_item->mm;
>> @@ -787,6 +798,11 @@ static void break_cow(struct ksm_rmap_item *rmap_item)
>>  	 * to undo, we also need to drop a reference to the anon_vma.
>>  	 */
>>  	put_anon_vma(rmap_item->anon_vma);
>> +	/*
>> +	 * Reset linear_page_index that might overlay age-related
>> +	 * information. (it's still unstable node)
>> +	 */
>> +	rmap_item->linear_page_index = 0;
>
>Sashiko comments that, on 32bit, it is not overlaying age-related information.
>So setting it to 0 won't clear the age.
>
>Which is what we document with the "might".
>
>On 32bit, it simply behaves the way it was before (no reset of age-related
>information here).
>
>We could move oldchecksum below remaining_skips to clear age-related information
>consistently. Not sure whether that is really worth it.

Yes, On 32-bit systems, although we do not clean the age, we also did not overwrite it.
So the behavior is the same as it used to be.

>> @@ -1598,8 +1618,16 @@ static int try_to_merge_with_ksm_page(struct ksm_rmap_item *rmap_item,
>>  	/* Unstable nid is in union with stable anon_vma: remove first */
>>  	remove_rmap_item_from_tree(rmap_item);
>> 
>> -	/* Must get reference to anon_vma while still holding mmap_lock */
>> +	/*
>> +	 * Must get reference to anon_vma while still holding mmap_lock.
>
>I think this sentence should go, and instead ...
>
>> +	 * Must can only reference the VMA while still holding the mmap
>
>This one should become:
>
>"We can consider the VMA only while still holding the mmap lock, so ...

Oh, my bad. I'll correct it right away.

> +	 * lock, so reference the anon_vma and calculate the linear page
> +	 * index early, before stable_tree_append(). If anything goes
> +	 * wrong that prevents the rmap_item from being added to the
> +	 * stable_tree, break_cow() will clean it up.
> +	 */
>  	rmap_item->anon_vma = vma->anon_vma;
> +	rmap_item->linear_page_index = linear_page_index(vma, rmap_item->address);
>  	get_anon_vma(vma->anon_vma);
>  out:
>  	mmap_read_unlock(mm);
> @@ -2458,6 +2486,13 @@ static bool should_skip_rmap_item(struct folio *folio,
>  	if (folio_test_ksm(folio))
>  		return false;

> Thinking about the overlay once more, I'm trying to assess what it means when we
> clear rmap_item->checksum. We'd do that now in:
> 
> (a) remove_node_from_stable_tree(): We had a stable node -> KSM page, but
> something changed.
> 
> (b) remove_rmap_item_from_tree(): Same as (a)
> 
> (c) break_cow(): we have to unshare, either because insertion into the stable
>     tree failed, or because we would have only a single PTE mapping the KSM
>     folio after a failed merge.
> 
> For (c), I guess if we'd have to, we could remember the checksum while
> processing the rmap_item.
> 
> Clearing rmap_item->checksum implies that cmp_and_merge_page() would refuse to
> merge one round.
> 
> Having rmap_item->checksum cleared is just like allocating a fresh rmap_item. So
> it will fix itself up during the next scan.
> 
> So my best guess is that this is alright.

You've thought this through carefully. For case c, clearing rmap_item->checksum will
indeed cause cmp_and_merge_page to reject the first merge. The original intent of
rejecting the merge is because the page changes too frequently (somewhat similar to
LRU), and this rationale still hold. Making this page scanned in your case (c) one
more time is acceptable.