Re: [PATCH v7 7/7] mm/madvise: allow KSM hints for remote API

On Fri, Mar 06, 2020 at 05:08:18PM +0100, Vlastimil Babka wrote:

On 3/6/20 2:41 PM, Oleksandr Natalenko wrote:

quoted

On Fri, Mar 06, 2020 at 02:13:49PM +0100, Vlastimil Babka wrote:

quoted

On 3/2/20 8:36 PM, Minchan Kim wrote:

quoted

From: Oleksandr Natalenko <redacted>

It all began with the fact that KSM works only on memory that is marked
by madvise(). And the only way to get around that is to either:

  * use LD_PRELOAD; or
  * patch the kernel with something like UKSM or PKSM.

(i skip ptrace can of worms here intentionally)

To overcome this restriction, lets employ a new remote madvise API. This
can be used by some small userspace helper daemon that will do auto-KSM
job for us.

I think of two major consumers of remote KSM hints:

  * hosts, that run containers, especially similar ones and especially in
    a trusted environment, sharing the same runtime like Node.js;

Ah, I forgot to ask, given the discussion of races in patch 2 (Question 2),
where android can stop the tasks to apply the madvise hints in a race-free
manner, how does that work for remote KSM hints in your scenarios, especially
the one above?

We have cgroup.freeze for that.

quoted

quoted

quoted

  * heavy applications, that can be run in multiple instances, not
    limited to opensource ones like Firefox, but also those that cannot be
    modified since they are binary-only and, maybe, statically linked.

Speaking of statistics, more numbers can be found in the very first
submission, that is related to this one [1]. For my current setup with
two Firefox instances I get 100 to 200 MiB saved for the second instance
depending on the amount of tabs.

1 FF instance with 15 tabs:

   $ echo "$(cat /sys/kernel/mm/ksm/pages_sharing) * 4 / 1024" | bc
   410

2 FF instances, second one has 12 tabs (all the tabs are different):

   $ echo "$(cat /sys/kernel/mm/ksm/pages_sharing) * 4 / 1024" | bc
   592

At the very moment I do not have specific numbers for containerised
workload, but those should be comparable in case the containers share
similar/same runtime.

[1] https://lore.kernel.org/patchwork/patch/1012142/

Reviewed-by: SeongJae Park <redacted>
Signed-off-by: Oleksandr Natalenko <redacted>
Signed-off-by: Minchan Kim <minchan@kernel.org>

This will lead to one process calling unmerge_ksm_pages() of another. There's a
(signal_pending(current)) test there, should it check also the other task,
analogically to task 3?

Do we care about current there then? Shall we just pass mm into unmerge_ksm_pages and check the signals of the target task only, be it current or something else?

Dunno, it's nice to react to signals quickly, for any proces that gets them, no?

So, do you mean something like this?

===

diff --git a/mm/ksm.c b/mm/ksm.c
index 363ec8189561..b39c237cfcf4 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c

@@ -849,7 +849,8 @@ static int unmerge_ksm_pages(struct vm_area_struct *vma,
 	for (addr = start; addr < end && !err; addr += PAGE_SIZE) {
 		if (ksm_test_exit(vma->vm_mm))
 			break;
-		if (signal_pending(current))
+		if (signal_pending(current) ||
+		    signal_pending(rcu_dereference(vma->vm_mm->owner)))
 			err = -ERESTARTSYS;
 		else
 			err = break_ksm(vma, addr);

===

BTW, this won't work with !CONFIG_MEMCG, so probably task_struct should be
passed through instead. IIUC, this would also require amending struct
mm_slot in order to share the same code path with ksmd.

I'm not sure I've seen such a culprit anywhere else, so I'm in doubt
this would be a correct thing to do.

Ideas?

quoted

quoted

Then break_ksm() is fine as it is, as ksmd also calls it, right?

I think break_ksm() cares only about mmap_sem protection, so we should
be fine here.

quoted

quoted

---
 mm/madvise.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/mm/madvise.c b/mm/madvise.c
index e77c6c1fad34..f4fa962ee74d 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c

@@ -1005,6 +1005,10 @@ process_madvise_behavior_valid(int behavior)
 	switch (behavior) {
 	case MADV_COLD:
 	case MADV_PAGEOUT:
+#ifdef CONFIG_KSM
+	case MADV_MERGEABLE:
+	case MADV_UNMERGEABLE:
+#endif
 		return true;
 	default:
 		return false;

-- 
  Best regards,
    Oleksandr Natalenko (post-factum)
    Principal Software Maintenance Engineer