lkml.org 
[lkml]   [2014]   [Feb]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v3] mm: per-thread vma caching
From
Agree with Linus; this is starting to look pretty good.

I still have nits though :)

On Wed, Feb 26, 2014 at 4:07 PM, Davidlohr Bueso <davidlohr@hp.com> wrote:
> @@ -0,0 +1,45 @@
> +#ifndef __LINUX_VMACACHE_H
> +#define __LINUX_VMACACHE_H
> +
> +#include <linux/mm.h>
> +
> +#ifdef CONFIG_MMU
> +#define VMACACHE_BITS 2
> +#else
> +#define VMACACHE_BITS 0
> +#endif

I wouldn't even both with the #ifdef here - why not just always use 2 bits ?

> +#define vmacache_flush(tsk) \
> + do { \
> + memset(tsk->vmacache, 0, sizeof(tsk->vmacache)); \
> + } while (0)

I think inline functions are preferred

> diff --git a/mm/nommu.c b/mm/nommu.c
> index 8740213..9a5347b 100644
> --- a/mm/nommu.c
> +++ b/mm/nommu.c
> @@ -768,16 +768,19 @@ static void add_vma_to_mm(struct mm_struct *mm, struct vm_area_struct *vma)
> */
> static void delete_vma_from_mm(struct vm_area_struct *vma)
> {
> + int i;
> struct address_space *mapping;
> struct mm_struct *mm = vma->vm_mm;
> + struct task_struct *curr = current;
>
> kenter("%p", vma);
>
> protect_vma(vma, 0);
>
> mm->map_count--;
> - if (mm->mmap_cache == vma)
> - mm->mmap_cache = NULL;
> + for (i = 0; i < VMACACHE_SIZE; i++)
> + if (curr->vmacache[i] == vma)
> + curr->vmacache[i] = NULL;

Why is the invalidation done differently here ? shouldn't it be done
by bumping the mm's sequence number so that invalidation works accross
all threads sharing that mm ?

> +#ifndef CONFIG_MMU
> +struct vm_area_struct *vmacache_find_exact(struct mm_struct *mm,
> + unsigned long start,
> + unsigned long end)
> +{
> + int i;
> +
> + if (!vmacache_valid(mm))
> + return NULL;
> +
> + for (i = 0; i < VMACACHE_SIZE; i++) {
> + struct vm_area_struct *vma = current->vmacache[i];
> +
> + if (vma && vma->vm_start == start && vma->vm_end == end)
> + return vma;
> + }
> +
> + return NULL;
> +
> +}
> +#endif

I think the caller could do instead
vma = vmacache_find(mm, start)
if (vma && vma->vm_start == start && vma->vm_end == end) {
}

I.e. better deal with it at the call site than add a new vmacache
function for it.

These are nits, the code looks good already.

I would like to propose an LRU eviction scheme to replace your
VMACACHE_HASH mechanism; I will probably do that as a follow-up once
you have the code in andrew's tree.

Reviewed-by: Michel Lespinasse <walken@google.com>

--
Michel "Walken" Lespinasse
A program is never fully debugged until the last user dies.


\
 
 \ /
  Last update: 2014-02-27 08:41    [W:0.079 / U:0.020 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site