Re: [PATCH mmotm] add the pagefault count into memcg stats: shmem fix

On Thu, May 19, 2011 at 9:28 AM, Hugh Dickins [off-list ref] wrote:

On Thu, 19 May 2011, Minchan Kim wrote:

quoted

On Wed, May 18, 2011 at 3:24 AM, Hugh Dickins [off-list ref] wrote:

quoted

mem_cgroup_count_vm_event() should update the PGMAJFAULT count for the
target mm, not for current mm (but of course they're usually the same).

We don't know the target mm in shmem_getpage(), so do it at the outer
level in shmem_fault(); and it's easier to follow if we move the
count_vm_event(PGMAJFAULT) there too.

Hah, it was using __count_vm_event() before, sneaking that update into
the unpreemptible section under info->lock: well, it comes to the same
on x86 at least, and I still think it's best to keep these together.

Signed-off-by: Hugh Dickins <hughd@google.com>

It's good to me but I have a nitpick.

You are changing behavior a bit.
Old behavior is to account FAULT although the operation got failed.
But new one is to not account it.
I think we have to account it regardless of whether it is successful or not.
That's because it is fact fault happens.

That's a good catch: something I didn't think of at all.

However, it looks as if the patch remains correct, and is fixing
a bug (or inconsistency) that we hadn't noticed before.

If you look through filemap_fault() or do_swap_page() (or even
ncp_file_mmap_fault(), though I don't take that one as canonical!),
they clearly do not count the major fault on error (except in the
case where VM_FAULT_MAJOR needs VM_FAULT_RETRY, then gets
VM_FAULT_ERROR on the retry).

So, shmem.c was the odd one out before.  If you feel very strongly
about it ("it is fact fault happens") you could submit a patch to
change them all - but I think just leave them as is.

Okay. I don't feel it strongly now.
Then, could you repost your patch with corrected description about
this behavior change which is a bug or inconsistency whatever. :)

Hugh

-- 
Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>