Often, when you have to start making a list of things that a patch does, it
might make sense to split some of the items into separate patches such that you
can avoid lists and just explain in list-free text how the pieces in the patch
fit together.
I'd suggest splitting this patch into logical pieces. For example, separating
the general profit calculation/exposure from the per-mm profit and the per-mm
ksm type indication.
Originally these were individual patches. If I recall correctly Johannes
Weiner wanted them as one patch. I can certainly split them again.
That's why I remember that v1 contained more patches :)
Again, just my opinion on patches that require a description in form of
a list ...
Link: https://lkml.kernel.org/r/20230224044000.3084046-3-shr@xxxxxxxxxxxx
Signed-off-by: Stefan Roesch <shr@xxxxxxxxxxxx>
Reviewed-by: Bagas Sanjaya <bagasdotme@xxxxxxxxx>
Cc: David Hildenbrand <david@xxxxxxxxxx>
Cc: Johannes Weiner <hannes@xxxxxxxxxxx>
Cc: Michal Hocko <mhocko@xxxxxxxx>
Cc: Rik van Riel <riel@xxxxxxxxxxx>
Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
---
[...]
KSM_ATTR_RO(pages_volatile);
@@ -3280,6 +3305,21 @@ static ssize_t zero_pages_sharing_show(struct kobject
*kobj,
}
KSM_ATTR_RO(zero_pages_sharing);
+static ssize_t general_profit_show(struct kobject *kobj,
+ struct kobj_attribute *attr, char *buf)
+{
+ long general_profit;
+ long all_rmap_items;
+
+ all_rmap_items = ksm_max_page_sharing + ksm_pages_shared +
+ ksm_pages_unshared + pages_volatile();
Are you sure you want to count a config knob (ksm_max_page_sharing) into that
formula? I yet have to digest what this calculation implies, but it does feel
odd.
This was a mistake. I wanted ksm_pages_sharing instead of
ksm_max_page_sharing.
Further, maybe just avoid pages_volatile(). Expanding the formula (excluding
ksm_max_page_sharing for now):
all_rmap = ksm_pages_shared + ksm_pages_unshared + pages_volatile();
-> expand pages_volatile() (ignoring the < 0 case)
all_rmap = ksm_pages_shared + ksm_pages_unshared + ksm_rmap_items -
ksm_pages_shared - ksm_pages_sharing - ksm_pages_unshared;
-> simplify
all_rmap = ksm_rmap_items + ksm_pages_sharing;
I'll simplify it.
Cool.
--
Thanks,
David / dhildenb
