On Wed, 2014-02-26 at 22:47 -0800, Michel Lespinasse wrote:
> 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@xxxxxx> 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 ?
Sure.
> > +#define vmacache_flush(tsk) \
> > + do { \
> > + memset(tsk->vmacache, 0, sizeof(tsk->vmacache)); \
> > + } while (0)
>
> I think inline functions are preferred
Indeed, but I used a macro instead because we don't have access to
'struct task_struct' from vmacache.h, since we also include it in
sched.h.
>
> > 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 ?
You are absolutely right. I will update this.
>
> > +#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.
But that would require two vma checks as the vmacache_find() function
needs to verify the matching vmas before returning. I had also thought
of passing a pointer to a matching function where we do the default (the
addr within a range) or the exact lookup. Seemed more of an overkill so
I just went with just adding a vmacache_find_exact() for !CONFIG_MMU.
Thanks,
Davidlohr
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>