Hi Hugh,
On Wed, May 18, 2011 at 3:24 AM, Hugh Dickins <hughd@xxxxxxxxxx> wrote:
> 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@xxxxxxxxxx>
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.
> ---
>
> Âmm/shmem.c | Â 13 ++++++-------
> Â1 file changed, 6 insertions(+), 7 deletions(-)
>
> --- mmotm/mm/shmem.c  Â2011-05-13 14:57:45.367884578 -0700
> +++ linux/mm/shmem.c  Â2011-05-17 10:27:19.901934756 -0700
> @@ -1293,14 +1293,10 @@ repeat:
> Â Â Â Â Â Â Â Âswappage = lookup_swap_cache(swap);
> Â Â Â Â Â Â Â Âif (!swappage) {
> Â Â Â Â Â Â Â Â Â Â Â Âshmem_swp_unmap(entry);
> + Â Â Â Â Â Â Â Â Â Â Â spin_unlock(&info->lock);
> Â Â Â Â Â Â Â Â Â Â Â Â/* here we actually do the io */
> - Â Â Â Â Â Â Â Â Â Â Â if (type && !(*type & VM_FAULT_MAJOR)) {
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â __count_vm_event(PGMAJFAULT);
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â mem_cgroup_count_vm_event(current->mm,
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â PGMAJFAULT);
> + Â Â Â Â Â Â Â Â Â Â Â if (type)
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â*type |= VM_FAULT_MAJOR;
> - Â Â Â Â Â Â Â Â Â Â Â }
> - Â Â Â Â Â Â Â Â Â Â Â spin_unlock(&info->lock);
> Â Â Â Â Â Â Â Â Â Â Â Âswappage = shmem_swapin(swap, gfp, info, idx);
> Â Â Â Â Â Â Â Â Â Â Â Âif (!swappage) {
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âspin_lock(&info->lock);
> @@ -1539,7 +1535,10 @@ static int shmem_fault(struct vm_area_st
> Â Â Â Âerror = shmem_getpage(inode, vmf->pgoff, &vmf->page, SGP_CACHE, &ret);
> Â Â Â Âif (error)
> Â Â Â Â Â Â Â Âreturn ((error == -ENOMEM) ? VM_FAULT_OOM : VM_FAULT_SIGBUS);
> -
> + Â Â Â if (ret & VM_FAULT_MAJOR) {
> + Â Â Â Â Â Â Â count_vm_event(PGMAJFAULT);
> + Â Â Â Â Â Â Â mem_cgroup_count_vm_event(vma->vm_mm, PGMAJFAULT);
> + Â Â Â }
> Â Â Â Âreturn ret | VM_FAULT_LOCKED;
> Â}
>
>
--
Kind regards,
Minchan Kim
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxxx 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