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

[Vlastimil Babka <vbabka@suse.cz>]

On 3/6/20 2:41 PM, Oleksandr Natalenko wrote:
> On Fri, Mar 06, 2020 at 02:13:49PM +0100, Vlastimil Babka wrote:
>> On 3/2/20 8:36 PM, Minchan Kim wrote:
>> > From: Oleksandr Natalenko <oleksandr@redhat.com>
>> > 
>> > 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?

>> > 
>> >   * 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 <sjpark@amazon.de>
>> > Signed-off-by: Oleksandr Natalenko <oleksandr@redhat.com>
>> > 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?

>> 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.
> 
>> 
>> > ---
>> >  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;
>> > 
>> 
>