[PATCH -next] mm/vmacache: inline vmacache_valid_mm()

[PATCH -next] mm/vmacache: inline vmacache_valid_mm()

[Davidlohr Bueso]

This function incurs in very hot paths and merely
does a few loads for validity check. Lets inline it,
such that we can save the function call overhead.

Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
---
 mm/vmacache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/vmacache.c b/mm/vmacache.c
index b6e3662..fd09dc9 100644
--- a/mm/vmacache.c
+++ b/mm/vmacache.c
@@ -52,7 +52,7 @@ void vmacache_flush_all(struct mm_struct *mm)
  * Also handle the case where a kernel thread has adopted this mm via use_mm().
  * That kernel thread's vmacache is not applicable to this mm.
  */
-static bool vmacache_valid_mm(struct mm_struct *mm)
+static inline bool vmacache_valid_mm(struct mm_struct *mm)
 {
 	return current->mm == mm && !(current->flags & PF_KTHREAD);
 }
-- 
2.1.4

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH -next] mm/vmacache: inline vmacache_valid_mm()

[Sergey Senozhatsky]

On (10/07/15 21:17), Davidlohr Bueso wrote:
> This function incurs in very hot paths and merely
> does a few loads for validity check. Lets inline it,
> such that we can save the function call overhead.
> 
> Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
> ---
>  mm/vmacache.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/vmacache.c b/mm/vmacache.c
> index b6e3662..fd09dc9 100644
> --- a/mm/vmacache.c
> +++ b/mm/vmacache.c
> @@ -52,7 +52,7 @@ void vmacache_flush_all(struct mm_struct *mm)
>   * Also handle the case where a kernel thread has adopted this mm via use_mm().
>   * That kernel thread's vmacache is not applicable to this mm.
>   */
> -static bool vmacache_valid_mm(struct mm_struct *mm)
> +static inline bool vmacache_valid_mm(struct mm_struct *mm)
>  {
>  	return current->mm == mm && !(current->flags & PF_KTHREAD);
>  }

Seems to be inlined anyway. do you want to inline vmacache_update()?
It looks simple enough (vmacache_valid_mm() is inlined):

void vmacache_update(unsigned long addr, struct vm_area_struct *newvma)
{
	if (vmacache_valid_mm(newvma->vm_mm))
		current->vmacache[VMACACHE_HASH(addr)] = newvma;
}


After moving vmacache_update() and vmacache_valid_mm() to include/linux/vmacache.h
(both `static inline')


./scripts/bloat-o-meter vmlinux.o.old vmlinux.o
add/remove: 0/1 grow/shrink: 1/0 up/down: 22/-54 (-32)
function                                     old     new   delta
find_vma                                      97     119     +22
vmacache_update                               54       -     -54


Something like this, perhaps?

---

 include/linux/vmacache.h | 21 ++++++++++++++++++++-
 mm/vmacache.c            | 20 --------------------
 2 files changed, 20 insertions(+), 21 deletions(-)

diff --git a/include/linux/vmacache.h b/include/linux/vmacache.h
index c3fa0fd4..0ec750b 100644
--- a/include/linux/vmacache.h
+++ b/include/linux/vmacache.h
@@ -15,8 +15,27 @@ static inline void vmacache_flush(struct task_struct *tsk)
 	memset(tsk->vmacache, 0, sizeof(tsk->vmacache));
 }
 
+/*
+ * This task may be accessing a foreign mm via (for example)
+ * get_user_pages()->find_vma().  The vmacache is task-local and this
+ * task's vmacache pertains to a different mm (ie, its own).  There is
+ * nothing we can do here.
+ *
+ * Also handle the case where a kernel thread has adopted this mm via use_mm().
+ * That kernel thread's vmacache is not applicable to this mm.
+ */
+static bool vmacache_valid_mm(struct mm_struct *mm)
+{
+	return current->mm == mm && !(current->flags & PF_KTHREAD);
+}
+
+static inline void vmacache_update(unsigned long addr, struct vm_area_struct *newvma)
+{
+	if (vmacache_valid_mm(newvma->vm_mm))
+		current->vmacache[VMACACHE_HASH(addr)] = newvma;
+}
+
 extern void vmacache_flush_all(struct mm_struct *mm);
-extern void vmacache_update(unsigned long addr, struct vm_area_struct *newvma);
 extern struct vm_area_struct *vmacache_find(struct mm_struct *mm,
 						    unsigned long addr);
 
diff --git a/mm/vmacache.c b/mm/vmacache.c
index b6e3662..14fec21 100644
--- a/mm/vmacache.c
+++ b/mm/vmacache.c
@@ -43,26 +43,6 @@ void vmacache_flush_all(struct mm_struct *mm)
 	rcu_read_unlock();
 }
 
-/*
- * This task may be accessing a foreign mm via (for example)
- * get_user_pages()->find_vma().  The vmacache is task-local and this
- * task's vmacache pertains to a different mm (ie, its own).  There is
- * nothing we can do here.
- *
- * Also handle the case where a kernel thread has adopted this mm via use_mm().
- * That kernel thread's vmacache is not applicable to this mm.
- */
-static bool vmacache_valid_mm(struct mm_struct *mm)
-{
-	return current->mm == mm && !(current->flags & PF_KTHREAD);
-}
-
-void vmacache_update(unsigned long addr, struct vm_area_struct *newvma)
-{
-	if (vmacache_valid_mm(newvma->vm_mm))
-		current->vmacache[VMACACHE_HASH(addr)] = newvma;
-}
-
 static bool vmacache_valid(struct mm_struct *mm)
 {
 	struct task_struct *curr;

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH -next] mm/vmacache: inline vmacache_valid_mm()

[Davidlohr Bueso]

On Thu, 08 Oct 2015, Sergey Senozhatsky wrote:

>After moving vmacache_update() and vmacache_valid_mm() to include/linux/vmacache.h
>(both `static inline')
>
>
>./scripts/bloat-o-meter vmlinux.o.old vmlinux.o
>add/remove: 0/1 grow/shrink: 1/0 up/down: 22/-54 (-32)
>function                                     old     new   delta
>find_vma                                      97     119     +22
>vmacache_update                               54       -     -54
>
>
>Something like this, perhaps?

iirc we actually had something like this in its original form, and akpm was forced
to move things around for all users to be happy and not break the build. But yeah,
that vmacache_update() could certainly be inlined if we can have it so. It's no
where near as hot a path as the mm validity check (we have a good hit rate), but still
seems reasonable.

>
>---
>
> include/linux/vmacache.h | 21 ++++++++++++++++++++-
> mm/vmacache.c            | 20 --------------------
> 2 files changed, 20 insertions(+), 21 deletions(-)
>
>diff --git a/include/linux/vmacache.h b/include/linux/vmacache.h
>index c3fa0fd4..0ec750b 100644
>--- a/include/linux/vmacache.h
>+++ b/include/linux/vmacache.h
>@@ -15,8 +15,27 @@ static inline void vmacache_flush(struct task_struct *tsk)
> 	memset(tsk->vmacache, 0, sizeof(tsk->vmacache));
> }
>
>+/*
>+ * This task may be accessing a foreign mm via (for example)
>+ * get_user_pages()->find_vma().  The vmacache is task-local and this
>+ * task's vmacache pertains to a different mm (ie, its own).  There is
>+ * nothing we can do here.
>+ *
>+ * Also handle the case where a kernel thread has adopted this mm via use_mm().
>+ * That kernel thread's vmacache is not applicable to this mm.
>+ */
>+static bool vmacache_valid_mm(struct mm_struct *mm)

This needs (explicit) inlined, no?

Thanks,
Davidlohr

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH -next] mm/vmacache: inline vmacache_valid_mm()

[Sergey Senozhatsky]

On (10/08/15 06:23), Davidlohr Bueso wrote:
> >After moving vmacache_update() and vmacache_valid_mm() to include/linux/vmacache.h
> >(both `static inline')
> >
> >
> >./scripts/bloat-o-meter vmlinux.o.old vmlinux.o
> >add/remove: 0/1 grow/shrink: 1/0 up/down: 22/-54 (-32)
> >function                                     old     new   delta
> >find_vma                                      97     119     +22
> >vmacache_update                               54       -     -54
> >
> >
> >Something like this, perhaps?
> 
> iirc we actually had something like this in its original form, and akpm was forced
> to move things around for all users to be happy and not break the build. But yeah,
> that vmacache_update() could certainly be inlined if we can have it so. It's no
> where near as hot a path as the mm validity check (we have a good hit rate), but still
> seems reasonable.

Hello,

Andrew "was forced to move things around", hm, I need to google for it.

Davidlohr, care to send a V2? (this is just a minor improvement to your
patch).

> >
> >---
> >
> >include/linux/vmacache.h | 21 ++++++++++++++++++++-
> >mm/vmacache.c            | 20 --------------------
> >2 files changed, 20 insertions(+), 21 deletions(-)
> >
> >diff --git a/include/linux/vmacache.h b/include/linux/vmacache.h
> >index c3fa0fd4..0ec750b 100644
> >--- a/include/linux/vmacache.h
> >+++ b/include/linux/vmacache.h
> >@@ -15,8 +15,27 @@ static inline void vmacache_flush(struct task_struct *tsk)
> >	memset(tsk->vmacache, 0, sizeof(tsk->vmacache));
> >}
> >
> >+/*
> >+ * This task may be accessing a foreign mm via (for example)
> >+ * get_user_pages()->find_vma().  The vmacache is task-local and this
> >+ * task's vmacache pertains to a different mm (ie, its own).  There is
> >+ * nothing we can do here.
> >+ *
> >+ * Also handle the case where a kernel thread has adopted this mm via use_mm().
> >+ * That kernel thread's vmacache is not applicable to this mm.
> >+ */
> >+static bool vmacache_valid_mm(struct mm_struct *mm)
> 
> This needs (explicit) inlined, no?
> 

oh, yeah. Funny how I said "both `static inline'" and made 'inline' only
one of them.

	-ss

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH -next] mm/vmacache: inline vmacache_valid_mm()

[Davidlohr Bueso]

On Thu, 08 Oct 2015, Sergey Senozhatsky wrote:

>> >+/*
>> >+ * This task may be accessing a foreign mm via (for example)
>> >+ * get_user_pages()->find_vma().  The vmacache is task-local and this
>> >+ * task's vmacache pertains to a different mm (ie, its own).  There is
>> >+ * nothing we can do here.
>> >+ *
>> >+ * Also handle the case where a kernel thread has adopted this mm via use_mm().
>> >+ * That kernel thread's vmacache is not applicable to this mm.
>> >+ */
>> >+static bool vmacache_valid_mm(struct mm_struct *mm)
>>
>> This needs (explicit) inlined, no?
>>
>
>oh, yeah. Funny how I said "both `static inline'" and made 'inline' only
>one of them.

Thinking a bit more about it, we don't want to be making vmacache_valid_mm()
visible, as users should only stick to vmacache_valid() calls. I doubt that
this would infact ever occur, but it's a bad idea regardless.

So I'd rather keep my patch as is. Yes, the compiler can already inline it for
us, but making it explicit is certainly won't harm.

Thanks,
Davidlohr

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH -next] mm/vmacache: inline vmacache_valid_mm()

[Davidlohr Bueso]

On Thu, 08 Oct 2015, Bueso wrote:
>Thinking a bit more about it, we don't want to be making vmacache_valid_mm()
>visible, as users should only stick to vmacache_valid() calls.
                                         ^^ s/vmacache_valid/vmacache_update

(cache validity is always internal, obviously).

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH -next] mm/vmacache: inline vmacache_valid_mm()

[Andrew Morton]

On Wed,  7 Oct 2015 21:17:59 -0700 Davidlohr Bueso <dave@stgolabs.net> wrote:

> This function incurs in very hot paths and merely
> does a few loads for validity check. Lets inline it,
> such that we can save the function call overhead.
> 
> ...
>
> --- a/mm/vmacache.c
> +++ b/mm/vmacache.c
> @@ -52,7 +52,7 @@ void vmacache_flush_all(struct mm_struct *mm)
>   * Also handle the case where a kernel thread has adopted this mm via use_mm().
>   * That kernel thread's vmacache is not applicable to this mm.
>   */
> -static bool vmacache_valid_mm(struct mm_struct *mm)
> +static inline bool vmacache_valid_mm(struct mm_struct *mm)
>  {
>  	return current->mm == mm && !(current->flags & PF_KTHREAD);
>  }

Yeah, I'll ingest my headgear if there's any vaguely recent compiler
which isn't already inlining this.

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>