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. 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). 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. Hugh
