On 10.03.23 19:28, Stefan Roesch wrote:
> This adds the general_profit KSM sysfs knob and the process profit metric
> and process merge type knobs to ksm_stat.
>
> 1) split off pages_volatile function
>
> This splits off the pages_volatile function. The next patch will
> use this function.
>
> 2) expose general_profit metric
>
> The documentation mentions a general profit metric, however this
> metric is not calculated. In addition the formula depends on the size
> of internal structures, which makes it more difficult for an
> administrator to make the calculation. Adding the metric for a better
> user experience.
>
> 3) document general_profit sysfs knob
>
> 4) calculate ksm process profit metric
>
> The ksm documentation mentions the process profit metric and how to
> calculate it. This adds the calculation of the metric.
>
> 5) add ksm_merge_type() function
>
> This adds the ksm_merge_type function. The function returns the
> merge type for the process. For madvise it returns "madvise", for
> prctl it returns "process" and otherwise it returns "none".
>
> 6) mm: expose ksm process profit metric and merge type in ksm_stat
>
> This exposes the ksm process profit metric in /proc/<pid>/ksm_stat.
> The name of the value is ksm_merge_type. The documentation mentions
> the formula for the ksm process profit metric, however it does not
> calculate it. In addition the formula depends on the size of internal
> structures. So it makes sense to expose it.
>
> 7) document new procfs ksm knobs
>
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.
> Link: https://lkml.kernel.org/r/20230224044000.3084046-3-shr@devkernel.io
> Signed-off-by: Stefan Roesch <shr(a)devkernel.io>
> Reviewed-by: Bagas Sanjaya <bagasdotme(a)gmail.com>
> Cc: David Hildenbrand <david(a)redhat.com>
> Cc: Johannes Weiner <hannes(a)cmpxchg.org>
> Cc: Michal Hocko <mhocko(a)suse.com>
> Cc: Rik van Riel <riel(a)surriel.com>
> Signed-off-by: Andrew Morton <akpm(a)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.
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;
Or is the < 0 case relevant here?
> + general_profit = ksm_pages_sharing * PAGE_SIZE -
> + all_rmap_items * sizeof(struct ksm_rmap_item);
> +
> + return sysfs_emit(buf, "%ld\n", general_profit);
> +}
> +KSM_ATTR_RO(general_profit);
> +
> static ssize_t stable_node_dups_show(struct kobject *kobj,
> struct kobj_attribute *attr, char *buf)
> {
> @@ -3345,6 +3385,7 @@ static struct attribute *ksm_attrs[] = {
> &stable_node_dups_attr.attr,
> &stable_node_chains_prune_millisecs_attr.attr,
> &use_zero_pages_attr.attr,
> + &general_profit_attr.attr,
> NULL,
> };
>
The calculations (profit) don't include when KSM places the shared
zeropage I guess. Accounting that per MM (and eventually globally) is in
the works. [1]
[1]
https://lore.kernel.org/lkml/20230328153852.26c2577e4bd921c371c47a7e@linux-…
--
Thanks,
David / dhildenb