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@devkernel.io Signed-off-by: Stefan Roesch shr@devkernel.io Reviewed-by: Bagas Sanjaya bagasdotme@gmail.com Cc: David Hildenbrand david@redhat.com Cc: Johannes Weiner hannes@cmpxchg.org Cc: Michal Hocko mhocko@suse.com Cc: Rik van Riel riel@surriel.com Signed-off-by: Andrew Morton akpm@linux-foundation.org
[...]
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.