Re: sendfile and page usage statistics

On Wed, Oct 10, 2007 at 10:25:41AM +0400, Igor Sysoev wrote:
> On Wed, Oct 10, 2007 at 04:14:44PM +1000, Antony Mawer wrote:
> 
> > On 10/10/2007 4:06 PM, Igor Sysoev wrote:
> > >On Wed, Oct 10, 2007 at 09:57:48AM +0400, Igor Sysoev wrote:
> > >
> > >>On Tue, Oct 09, 2007 at 04:50:01PM +0400, Igor Sysoev wrote:
> > >>
> > >>>As I understand if sendfile() with hardware TX chsum or TSO are used,
> > >>>then CPU does not touch file pages at all. So pagedaemon never set
> > >>>PG_REFERENCED to vnode object pages while scanning them.
> > >>>Does VM set PG_REFERENCED by another way ? Otherwise, often used files
> > >>>that accessed by sendfile() only have small chances to be cached by VM.
> > >>I have looked kern_sendfile() in CURRENT and it seems to me that
> > >>PG_REFERENCED is never set when sendfile() gets cached vm_page.
> > >>I have looked the code path:
> > >>
> > >>vm_page_grab()
> > >>    vm_page_lookup()
> > >>    vm_page_wire()
> > >>sf_buf_alloc() on i386 and amd64
> > >>
> > >>and have not seen PG_REFERENCED.
> > >
> > >As I understand the following patch against uipc_syscalls.c
> > >should fix the bug to some extent:
> > >
> > >                         if (pg->valid && vm_page_is_valid(pg, pgoff, 
> > >                         xfsize))
> > >+                                vm_page_flag_set(pg, PG_REFERENCED);
> > >                                 VM_OBJECT_UNLOCK(obj);
> > >                         else if (m != NULL)
> > 
> > You are missing opening/closing braces on your if() statement with this 
> > change...
> 
> Oh, thank you! You have saved me from coredump.
> This is why in my coding style I always use braces for single statement 
> inside if().

Also, as a minimum you need to brace the vm_page_flag_set() with
vm_page_lock_queues()/vm_page_unlock_queues().

But I think that the actual patch is wrong. The PG_REFERENCED bit shall
be set when hardware access bit is set on pte. You patch would set it in
advance.