Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1241957 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH -next] mm/vmacache: inline vmacache_valid_mm() Davidlohr Bueso <dave@stgolabs.net> - 2015-10-08 06:20 +0200
    Re: [PATCH -next] mm/vmacache: inline vmacache_valid_mm() Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2015-10-08 08:30 +0200
      Re: [PATCH -next] mm/vmacache: inline vmacache_valid_mm() Davidlohr Bueso <dave@stgolabs.net> - 2015-10-08 15:30 +0200
        Re: [PATCH -next] mm/vmacache: inline vmacache_valid_mm() Sergey Senozhatsky <sergey.senozhatsky@gmail.com> - 2015-10-08 15:50 +0200
          Re: [PATCH -next] mm/vmacache: inline vmacache_valid_mm() Davidlohr Bueso <dave@stgolabs.net> - 2015-10-08 19:00 +0200
            Re: [PATCH -next] mm/vmacache: inline vmacache_valid_mm() Davidlohr Bueso <dave@stgolabs.net> - 2015-10-08 19:40 +0200
    Re: [PATCH -next] mm/vmacache: inline vmacache_valid_mm() Andrew Morton <akpm@linux-foundation.org> - 2015-10-09 00:20 +0200

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

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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1241989

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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1242341

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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1242369

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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1242602

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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1242631

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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1242816

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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web