Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <6204b63f.1c69fb81.6011.2908@mx.google.com>
Date:   Thu, 10 Feb 2022 06:52:44 +0000
From:   CGEL <cgel.zte@gmail.com>
To:     Hugh Dickins <hughd@google.com>
Cc:     Johannes Weiner <hannes@cmpxchg.org>, akpm@linux-foundation.org,
        sfr@canb.auug.org.au, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, Yang Yang <yang.yang29@zte.com.cn>,
        ran.xiaokai@zte.com.cn, wang.yong12@zte.com.cn
Subject: Re: [PATCH] psi: Treat ksm swapping in copy as memstall
References: <20220116152150.859520-1-yang.yang29@zte.com.cn>
 <YeVdvVVBvrXH5U0L@cmpxchg.org>
 <61e7ac25.1c69fb81.e8938.bc67@mx.google.com>
 <YegK7+oKFG8EPRp9@cmpxchg.org>
 <61ea820f.1c69fb81.e79d5.09c9@mx.google.com>
 <YfNG5KAog6fI0kYu@cmpxchg.org>
 <61f35591.1c69fb81.48dad.3244@mx.google.com>
 <72532675-d898-9f30-1ba4-318fbd61786@google.com>
 <62026b89.1c69fb81.b9102.7042@mx.google.com>
 <11c8fcd3-58fc-77f4-addb-18d3a52b51a7@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <11c8fcd3-58fc-77f4-addb-18d3a52b51a7@google.com>
Precedence: bulk

On Tue, Feb 08, 2022 at 09:55:48PM -0800, Hugh Dickins wrote:
> On Tue, 8 Feb 2022, CGEL wrote:
> > On Mon, Feb 07, 2022 at 07:22:22PM -0800, Hugh Dickins wrote:
> > > On Fri, 28 Jan 2022, CGEL wrote:
> > > > On Thu, Jan 27, 2022 at 08:29:08PM -0500, Johannes Weiner wrote:
> > > > > On Fri, Jan 21, 2022 at 09:51:08AM +0000, CGEL wrote:
> > > > > >  Wed, Jan 19, 2022 at 07:58:23AM -0500, Johannes Weiner wrote:
> > > > > > > On Wed, Jan 19, 2022 at 06:13:54AM +0000, CGEL wrote:
> > > > > > > > I did a test, when we use zram, it takes longer time for ksm copying than
> > > > > > > > swap_readpage(). Ksm copying average takes 147263ns, swap_readpage()
> > > > > > > > average takes 55639ns. So I think this patch is reasonable.
> > > > > > > 
> > > > > > > Ok, that sounds reasonable to me as well. Please add the
> > > > > > > PageWorkingset() check and resubmit the patch. Thanks!
> > > > > > I am a litte confused about adding PageWorkingset(), since I
> > > > > > think ksm_might_need_to_copy() memstall is like swap_readpage()
> > > > > > memstall and swap_readpage() doesn't add PageWorkingset().
> > > > > 
> > > > > That's actually a bug! It should do that.
> > > > I recently found that too. Please CC to me your new patch, thanks!
> > > > And I will send V2 of this patch "psi: Treat ksm swapping in copy
> > > > as memstall" with PageWorkingset().
> > > 
> > > I'm entirely PSI-ignorant, and reluctant to disagree with Johannes,
> > > but I don't see how your patch to ksm_might_need_to_copy() could be
> > > correct - maybe the "swapping" in your subject is confusing.
> > > 
> > > There is no PSI enter and exit around the page allocation and copying
> > > in wp_page_copy(), so why in the analogous ksm_might_need_to_copy()?
> > >
> > I think it's two questions, first why PSI didn't treat wp_page_copy() as
> > memstall, second why should PSI treat ksm_might_need_to_copy() as memstall.
> > 
> > The first question is unrelated with this patch. I think the reason is PSI
> > focous on memory contending(see Documentation/accounting/psi.rst), and
> > wp_page_copy() is not caused by memory contending. Actually wp_page_copy()
> > will still be called if memory is not contending.
> 
> Agreed.
> 
> > 
> > For the second question, ksm_might_need_to_copy() is called only becaused
> > of swapping, and swap is caused by memory contending, so PSI better treat
> > it as memstall.
> 
> But there I'm not at all convinced.
> 
>  * psi_memstall_enter - mark the beginning of a memory stall section
>  * @flags: flags to handle nested sections
>  *
>  * Marks the calling task as being stalled due to a lack of memory,
>  * such as waiting for a refault or performing reclaim.
> 
> psi_memstall_enter() will have been called if do_swap_page() had to
> read back from swap or wait on page lock; and psi_memstall_enter()
> will be called if ksm_might_need_to_copy()'s alloc_page_vma() or
> mem_cgroup_charge() goes into page reclaim.  Being stalled due to
> a lack of memory is fully covered there.
PSI counts "read back from swap time" as memstall time, because the
reason why we had to "read back from swap" is memory contending had
happened before and caused swap out. ksm_might_need_to_copy() is just
like it, we had to do this copy because of swap out happened before,
not because ksm_might_need_to_copy() may goes into page reclaim. And
if page reclaim happens, doesn't matter, psi_memstall_enter() will
handle nested sections.

> Your argument is that copy_user_highpage() is a significant overhead
> (but not a memstall), and it might have been avoided if there was
> no KSM or no swapping - though doing the copy there often(?) saves
> doing the wp_page_copy() when do_swap_page() goes on to do_wp_page()
> (and that one you're not asking to count).
"Doing the copy" doesn't always means "saves doing the wp_page_copy()".
For example, if there is no FAULT_FLAG_WRITE flag. What about we only
count this case as memstall: ksm_might_need_to_copy() do the copy and
not "saves doing the wp_page_copy()"? As this copy takes 2 times longer
than swap_readpage() when use zram(see early mail on this maillist),
we'd better not just leave it.

> I can understand you wanting to keep track of page copying overhead;
> and I've grown uneasy recently about the way CONFIG_KSM=y can add a
> ksm_might_need_to_copy() overhead for pages which never went near KSM;
> but I still don't see how psi_memstall is appropriate here.
For "ksm_might_need_to_copy() overhead for pages which never went near KSM",
is there a plan to optimize it? Or should I work on this?

Thanks.

> Hugh