linux-kernel - Re: [PATCH] delayacct: track delays from ksm cow

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 17 Mar 2022 02:03:57 +0000
From:   CGEL <cgel.zte@...il.com>
To:     David Hildenbrand <david@...hat.com>
Cc:     bsingharora@...il.com, akpm@...ux-foundation.org,
        yang.yang29@....com.cn, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org
Subject: Re: [PATCH] delayacct: track delays from ksm cow

On Wed, Mar 16, 2022 at 03:56:23PM +0100, David Hildenbrand wrote:
> On 16.03.22 14:34, cgel.zte@...il.com wrote:
> > From: Yang Yang <yang.yang29@....com.cn>
> > 
> > Delay accounting does not track the delay of ksm cow.  When tasks
> > have many ksm pages, it may spend a amount of time waiting for ksm
> > cow.
> > 
> > To get the impact of tasks in ksm cow, measure the delay when ksm
> > cow happens. This could help users to decide whether to user ksm
> > or not.
> > 
> > Also update tools/accounting/getdelays.c:
> > 
> >     / # ./getdelays -dl -p 231
> >     print delayacct stats ON
> >     listen forever
> >     PID     231
> > 
> >     CPU             count     real total  virtual total    delay total  delay average
> >                      6247     1859000000     2154070021     1674255063          0.268ms
> >     IO              count    delay total  delay average
> >                         0              0              0ms
> >     SWAP            count    delay total  delay average
> >                         0              0              0ms
> >     RECLAIM         count    delay total  delay average
> >                         0              0              0ms
> >     THRASHING       count    delay total  delay average
> >                         0              0              0ms
> >     KSM             count    delay total  delay average
> >                      3635      271567604              0ms
> > 
> 
> TBH I'm not sure how particularly helpful this is and if we want this.
>
Thanks for replying.

Users may use ksm by calling madvise(, , MADV_MERGEABLE) when they want
save memory, it's a tradeoff by suffering delay on ksm cow. Users can
get to know how much memory ksm saved by reading
/sys/kernel/mm/ksm/pages_sharing, but they don't know what the costs of
ksm cow delay, and this is important of some delay sensitive tasks. If
users know both saved memory and ksm cow delay, they could better use
madvise(, , MADV_MERGEABLE).

> [...]
> 
> >  	struct vm_area_struct *vma = vmf->vma;
> > +	vm_fault_t ret = 0;
> > +	bool delayacct = false;
> >  
> >  	if (userfaultfd_pte_wp(vma, *vmf->pte)) {
> >  		pte_unmap_unlock(vmf->pte, vmf->ptl);
> > @@ -3294,7 +3296,11 @@ static vm_fault_t do_wp_page(struct vm_fault *vmf)
> >  		 *
> >  		 * PageKsm() doesn't necessarily raise the page refcount.
> >  		 */
> > -		if (PageKsm(page) || page_count(page) > 3)
> > +		if (PageKsm(page)) {
> > +			delayacct = true;
> > +			goto copy;
> > +		}
> > +		if (page_count(page) > 3)
> >  			goto copy;
> >  		if (!PageLRU(page))
> >  			/*
> > @@ -3308,7 +3314,12 @@ static vm_fault_t do_wp_page(struct vm_fault *vmf)
> >  			goto copy;
> >  		if (PageSwapCache(page))
> >  			try_to_free_swap(page);
> > -		if (PageKsm(page) || page_count(page) != 1) {
> > +		if (PageKsm(page)) {
> > +			delayacct = true;
> > +			unlock_page(page);
> > +			goto copy;
> > +		}
> > +		if (page_count(page) != 1) {
> >  			unlock_page(page);
> >  			goto copy;
> >  		}
> > @@ -3328,10 +3339,18 @@ static vm_fault_t do_wp_page(struct vm_fault *vmf)
> >  	/*
> >  	 * Ok, we need to copy. Oh, well..
> >  	 */
> 
> Why not simply check for PageKsm() here? I dislike the added complexity
> above.
> 
The original code check PageKsm() twice, I just try to keep the original
semantics.

If you think this patch is reasonable, I will try to find a better way
to realize this.
> 
> -- 
> Thanks,
> 
> David / dhildenb