Re: [PATCH] tools: adding support for idle page tracking to tool

["Christian Hansen (chansen3)" <chansen3@cisco.com>]

Yes, I wasn't sure which one went in first.  I can make this one dependent on the other.

On 2018-06-19, 6:39 PM, "Andrew Morton" <akpm@linux-foundation.org> wrote:

    On Tue, 12 Jun 2018 11:32:23 -0400 Christian Hansen <chansen3@cisco.com> wrote:
    
    > Adding a flag which will use the kernels's idle
    > page tracking to mark pages idle.  As the tool already
    > prints the idle flag if set, subsequent runs will show
    > which pages have been accessed since last run.
    
    That sounds useful.
    
    This patch seems to have been prepared against the mainline kernel, so
    it conflicts with your "tools: modifying page-types to include shared
    map counts" patch.  Awkward, but I seem to have got it fixed up.
    
    > ...
    >
    > @@ -566,6 +570,30 @@ static int unpoison_page(unsigned long offset)
    >  	return 0;
    >  }
    >  
    > +static int mark_page_idle(unsigned long offset)
    > +{
    > +	static unsigned long off;
    > +	static uint64_t buf;
    > +	int len;
    > +
    > +	if ((offset / 64 == off / 64) || buf == 0) {
    > +		buf |= 1UL << (offset % 64);
    > +		off = offset;
    > +		return 0;
    > +	}
    > +
    > +	len = pwrite(page_idle_fd, &buf, 8, 8 * (off / 64));
    > +	if (len < 0) {
    > +		perror("mark page idle");
    > +		return len;
    > +	}
    > +
    > +	buf = 1UL << (offset % 64);
    > +	off = offset;
    > +
    > +	return 0;
    > +}
    
    This is a bit cumbersome.  Why not this way:
    
    static int mark_page_idle(unsigned long offset)
    {
    	static unsigned long off;
    	static uint64_t buf;
    	int len;
    
    	if ((offset / 64 != off / 64) && buf != 0) {
    		len = pwrite(page_idle_fd, &buf, 8, 8 * (off / 64));
    		if (len < 0) {
    			perror("mark page idle");
    			return len;
    		}
    	}
    	buf = 1UL << (offset % 64);
    	off = offset;
    	return 0;
    }
    
    Also, it's not very clear what's going on here - the handling of
    offset, off and buf.  Some well-crafted comments would help.
    
    >
    > ...
    >