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.
split off pages_volatile function
This splits off the pages_volatile function. The next patch will use this function.
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.
document general_profit sysfs knob
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.
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".
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.
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@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.
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-f...
David Hildenbrand david@redhat.com writes:
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.
- split off pages_volatile function This splits off the pages_volatile function. The next patch will use this function.
- 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.
- document general_profit sysfs knob
- 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.
- 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".
- 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.
- 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.
Originally these were individual patches. If I recall correctly Johannes Weiner wanted them as one patch. I can certainly split them again.
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.
Or is the < 0 case relevant here?
A negative profit is ok.
- 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-f...
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.
On Thu, Apr 06, 2023 at 03:23:11PM +0200, David Hildenbrand wrote:
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 ...
My concern was the initial splitting of patch 1. I found it easier to review with the new prctl() being one logical change, and fully implemented in one go. The changelog is still in the form of a list, but it essentially enumerates diff hunks that make up the interface.
No objection to splitting out the multiple sysfs knobs, though!
The strategy I usually follow is this:
1. Split out changes based on user-visible behavior (new prctl(), new sysfs knob)
2. Extract changes made along the way that have stand-alone value in existing code (bug fixes, simplifying/documenting tricky sections, cleanups).
3. Split out noisy prep work such as renames and refactors that make the user-visible functional changes more difficult to understand.
and then order the series into sections 2, 3, and 1.
On 06.04.23 16:16, Johannes Weiner wrote:
On Thu, Apr 06, 2023 at 03:23:11PM +0200, David Hildenbrand wrote:
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 ...
My concern was the initial splitting of patch 1. I found it easier to review with the new prctl() being one logical change, and fully implemented in one go. The changelog is still in the form of a list, but it essentially enumerates diff hunks that make up the interface.
No objection to splitting out the multiple sysfs knobs, though!
The strategy I usually follow is this:
Split out changes based on user-visible behavior (new prctl(), new sysfs knob)
Extract changes made along the way that have stand-alone value in existing code (bug fixes, simplifying/documenting tricky sections, cleanups).
Split out noisy prep work such as renames and refactors that make the user-visible functional changes more difficult to understand.
and then order the series into sections 2, 3, and 1.
I agree. The most important part is the "logical change" part. Once it's down to a list of 3 items or so for a single commit we can usually express it like (just an example that does no longer apply due to pages_volatile() not being required) the following when combining items 1+2+3 from the list:
" Let's expose the general KSM profit via sysfs and document that new toggle. [add details about that and especially why we are doing that]
As we need the number of volatile pages to calculate the general KSM profit, factor out existing functionality into a helper. "
Much easier to read.
linux-kselftest-mirror@lists.linaro.org
-
David Hildenbrand -
Johannes Weiner -
Stefan Roesch