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

Groups > linux.kernel > #1322035 > unrolled thread

[PATCH v2 0/3] lib/list_batch: A simple list insertion/deletion batching facility

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/3] lib/list_batch: A simple list insertion/deletion batching facility Waiman Long <Waiman.Long@hpe.com> - 2016-01-29 20:40 +0100
    [PATCH v2 2/3] lib/list_batch, x86: Enable list insertion/deletion batching for x86 Waiman Long <Waiman.Long@hpe.com> - 2016-01-29 20:40 +0100
    [PATCH v2 3/3] vfs: Enable list batching for the superblock's inode list Waiman Long <Waiman.Long@hpe.com> - 2016-01-29 20:40 +0100
      Re: [PATCH v2 3/3] vfs: Enable list batching for the superblock's  inode list Ingo Molnar <mingo@kernel.org> - 2016-01-30 09:40 +0100
        Re: [PATCH v2 3/3] vfs: Enable list batching for the superblock's  inode list Andi Kleen <andi@firstfloor.org> - 2016-02-01 18:50 +0100
          Re: [PATCH v2 3/3] vfs: Enable list batching for the superblock's  inode list Waiman Long <waiman.long@hpe.com> - 2016-02-01 23:10 +0100
            Re: [PATCH v2 3/3] vfs: Enable list batching for the superblock's  inode list Waiman Long <waiman.long@hpe.com> - 2016-02-04 00:10 +0100
              Re: [PATCH v2 3/3] vfs: Enable list batching for the superblock's  inode list Dave Chinner <david@fromorbit.com> - 2016-02-07 01:00 +0100
        Re: [PATCH v2 3/3] vfs: Enable list batching for the superblock's  inode list Waiman Long <waiman.long@hpe.com> - 2016-02-01 22:50 +0100
      Re: [PATCH v2 3/3] vfs: Enable list batching for the superblock's  inode list Dave Chinner <david@fromorbit.com> - 2016-02-01 01:10 +0100
        Re: [PATCH v2 3/3] vfs: Enable list batching for the superblock's  inode list Waiman Long <waiman.long@hpe.com> - 2016-02-04 00:10 +0100

#1322035 — [PATCH v2 0/3] lib/list_batch: A simple list insertion/deletion batching facility

v1->v2:
  - Address feedbacks from PeterZ and Andi Kleen.

This patchset introduces a simple list insertion/deletion batching
facility to batch multiple list insertion and deletion operations
into a single one under one lock/unlock critical section.

Patch 1 introduces this new facility.

Patch 2 enables it for the x86 architecture.

Patch 3 makes the insertion and deletion of the VFS superblock's
inode list to use the new list batching functions.

On x86-64, the generated code (gcc v4.8.5) of the inode_sb_list_add()
function without the patch was:

   0x0000000000001030 <+0>:     callq  0x1035 <inode_sb_list_add+5>
   0x0000000000001035 <+5>:     push   %rbp
   0x0000000000001036 <+6>:     mov    %rsp,%rbp
   0x0000000000001039 <+9>:     push   %rbx
   0x000000000000103a <+10>:    mov    0x28(%rdi),%rax
   0x000000000000103e <+14>:    mov    %rdi,%rbx
   0x0000000000001041 <+17>:    lea    0x600(%rax),%rdi
   0x0000000000001048 <+24>:    callq  0x104d <inode_sb_list_add+29>
   0x000000000000104d <+29>:    mov    0x28(%rbx),%rsi
   0x0000000000001051 <+33>:    lea    0x120(%rbx),%rdi
   0x0000000000001058 <+40>:    mov    0x608(%rsi),%rdx
   0x000000000000105f <+47>:    add    $0x608,%rsi
   0x0000000000001066 <+54>:    callq  0x106b <inode_sb_list_add+59>
   0x000000000000106b <+59>:    mov    0x28(%rbx),%rdi
   0x000000000000106f <+63>:    add    $0x600,%rdi
   0x0000000000001076 <+70>:    callq  *0x0
   0x000000000000107d <+77>:    pop    %rbx
   0x000000000000107e <+78>:    pop    %rbp
   0x000000000000107f <+79>:    retq

With the patch, the function became:

   0x0000000000001030 <+0>:     callq  0x1035 <inode_sb_list_add+5>
   0x0000000000001035 <+5>:     push   %rbp
   0x0000000000001036 <+6>:     mov    %rsp,%rbp
   0x0000000000001039 <+9>:     push   %r13
   0x000000000000103b <+11>:    lea    0x120(%rdi),%r13
   0x0000000000001042 <+18>:    push   %r12
   0x0000000000001044 <+20>:    push   %rbx
   0x0000000000001045 <+21>:    mov    0x28(%rdi),%r12
   0x0000000000001049 <+25>:    lea    0x600(%r12),%rbx
   0x0000000000001051 <+33>:    mov    %rbx,%rdi
   0x0000000000001054 <+36>:    callq  0x1059 <inode_sb_list_add+41>
   0x0000000000001059 <+41>:    test   %eax,%eax
   0x000000000000105b <+43>:    je     0x1081 <inode_sb_list_add+81>
   0x000000000000105d <+45>:    mov    0x618(%r12),%rsi
   0x0000000000001065 <+53>:    mov    %r13,%rdi
   0x0000000000001068 <+56>:    mov    (%rsi),%rdx
   0x000000000000106b <+59>:    callq  0x1070 <inode_sb_list_add+64>
   0x0000000000001070 <+64>:    mov    %rbx,%rdi
   0x0000000000001073 <+67>:    callq  *0x0
   0x000000000000107a <+74>:    pop    %rbx
   0x000000000000107b <+75>:    pop    %r12
   0x000000000000107d <+77>:    pop    %r13
   0x000000000000107f <+79>:    pop    %rbp
   0x0000000000001080 <+80>:    retq
   0x0000000000001081 <+81>:    lea    0x618(%r12),%rdx
   0x0000000000001089 <+89>:    mov    %r13,%rcx
   0x000000000000108c <+92>:    mov    %rbx,%rsi
   0x000000000000108f <+95>:    xor    %edi,%edi
   0x0000000000001091 <+97>:    callq  0x1096 <inode_sb_list_add+102>
   0x0000000000001096 <+102>:   jmp    0x107a <inode_sb_list_add+74>

For the fastpath, the only overheads are the testing of the return value
of the spin_trylock function and the push/pop of 2 more registers. Other
than that, they look very similar.

Waiman Long (3):
  lib/list_batch: A simple list insertion/deletion batching facility
  lib/list_batch, x86: Enable list insertion/deletion batching for x86
  vfs: Enable list batching for the superblock's inode list

 arch/x86/Kconfig           |    1 +
 fs/inode.c                 |   13 ++---
 fs/super.c                 |    1 +
 include/linux/fs.h         |    2 +
 include/linux/list_batch.h |  133 ++++++++++++++++++++++++++++++++++++++++++++
 lib/Kconfig                |    7 ++
 lib/Makefile               |    1 +
 lib/list_batch.c           |  125 +++++++++++++++++++++++++++++++++++++++++
 8 files changed, 275 insertions(+), 8 deletions(-)
 create mode 100644 include/linux/list_batch.h
 create mode 100644 lib/list_batch.c

[toc] | [next] | [standalone]

#1322036 — [PATCH v2 2/3] lib/list_batch, x86: Enable list insertion/deletion batching for x86

System with a large number of CPUs will benefit from the list batching
facility when the list lock is contended. If the lock isn't contended,
the performance of the list batching function should be similar to the
"lock; listop; unlock;" sequence that it replaces.

This patch enables it for the x86 architecture.

Signed-off-by: Waiman Long <Waiman.Long@hpe.com>
---
 arch/x86/Kconfig |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 330e738..5b35be7 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -42,6 +42,7 @@ config X86
 	select ARCH_SUPPORTS_NUMA_BALANCING	if X86_64
 	select ARCH_USE_BUILTIN_BSWAP
 	select ARCH_USE_CMPXCHG_LOCKREF		if X86_64
+	select ARCH_USE_LIST_BATCHING
 	select ARCH_USE_QUEUED_RWLOCKS
 	select ARCH_USE_QUEUED_SPINLOCKS
 	select ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH if SMP
-- 
1.7.1

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

#1322038 — [PATCH v2 3/3] vfs: Enable list batching for the superblock's inode list

The inode_sb_list_add() and inode_sb_list_del() functions in the vfs
layer just perform list addition and deletion under lock. So they can
use the new list batching facility to speed up the list operations
when many CPUs are trying to do it simultaneously.

In particular, the inode_sb_list_del() function can be a performance
bottleneck when large applications with many threads and associated
inodes exit. With an exit microbenchmark that creates a large number
of threads, attachs many inodes to them and then exits. The runtimes
of that microbenchmark with 1000 threads before and after the patch
on a 4-socket Intel E7-4820 v3 system (48 cores, 96 threads) were
as follows:

  Kernel        Elapsed Time    System Time
  ------        ------------    -----------
  Vanilla 4.4      65.29s         82m14s
  Patched 4.4      45.69s         49m44s

The elapsed time and the reported system time were reduced by 30%
and 40% respectively.

Signed-off-by: Waiman Long <Waiman.Long@hpe.com>
---
 fs/inode.c         |   13 +++++--------
 fs/super.c         |    1 +
 include/linux/fs.h |    2 ++
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/fs/inode.c b/fs/inode.c
index 9f62db3..870de8c 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -424,19 +424,16 @@ static void inode_lru_list_del(struct inode *inode)
  */
 void inode_sb_list_add(struct inode *inode)
 {
-	spin_lock(&inode->i_sb->s_inode_list_lock);
-	list_add(&inode->i_sb_list, &inode->i_sb->s_inodes);
-	spin_unlock(&inode->i_sb->s_inode_list_lock);
+	do_list_batch(&inode->i_sb->s_inode_list_lock, lb_cmd_add,
+			&inode->i_sb->s_list_batch, &inode->i_sb_list);
 }
 EXPORT_SYMBOL_GPL(inode_sb_list_add);
 
 static inline void inode_sb_list_del(struct inode *inode)
 {
-	if (!list_empty(&inode->i_sb_list)) {
-		spin_lock(&inode->i_sb->s_inode_list_lock);
-		list_del_init(&inode->i_sb_list);
-		spin_unlock(&inode->i_sb->s_inode_list_lock);
-	}
+	if (!list_empty(&inode->i_sb_list))
+		do_list_batch(&inode->i_sb->s_inode_list_lock, lb_cmd_del_init,
+				&inode->i_sb->s_list_batch, &inode->i_sb_list);
 }
 
 static unsigned long hash(struct super_block *sb, unsigned long hashval)
diff --git a/fs/super.c b/fs/super.c
index 1182af8..b0e8540 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -206,6 +206,7 @@ static struct super_block *alloc_super(struct file_system_type *type, int flags)
 	mutex_init(&s->s_sync_lock);
 	INIT_LIST_HEAD(&s->s_inodes);
 	spin_lock_init(&s->s_inode_list_lock);
+	list_batch_init(&s->s_list_batch, &s->s_inodes);
 
 	if (list_lru_init_memcg(&s->s_dentry_lru))
 		goto fail;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 1a20462..11d8b77 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -9,6 +9,7 @@
 #include <linux/stat.h>
 #include <linux/cache.h>
 #include <linux/list.h>
+#include <linux/list_batch.h>
 #include <linux/list_lru.h>
 #include <linux/llist.h>
 #include <linux/radix-tree.h>
@@ -1403,6 +1404,7 @@ struct super_block {
 	/* s_inode_list_lock protects s_inodes */
 	spinlock_t		s_inode_list_lock ____cacheline_aligned_in_smp;
 	struct list_head	s_inodes;	/* all inodes */
+	struct list_batch	s_list_batch;
 };
 
 extern struct timespec current_fs_time(struct super_block *sb);
-- 
1.7.1

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

#1322291 — Re: [PATCH v2 3/3] vfs: Enable list batching for the superblock's inode list

* Waiman Long <Waiman.Long@hpe.com> wrote:

> The inode_sb_list_add() and inode_sb_list_del() functions in the vfs
> layer just perform list addition and deletion under lock. So they can
> use the new list batching facility to speed up the list operations
> when many CPUs are trying to do it simultaneously.
> 
> In particular, the inode_sb_list_del() function can be a performance
> bottleneck when large applications with many threads and associated
> inodes exit. With an exit microbenchmark that creates a large number
> of threads, attachs many inodes to them and then exits. The runtimes
> of that microbenchmark with 1000 threads before and after the patch
> on a 4-socket Intel E7-4820 v3 system (48 cores, 96 threads) were
> as follows:
> 
>   Kernel        Elapsed Time    System Time
>   ------        ------------    -----------
>   Vanilla 4.4      65.29s         82m14s
>   Patched 4.4      45.69s         49m44s
> 
> The elapsed time and the reported system time were reduced by 30%
> and 40% respectively.

That's pretty impressive!

I'm wondering, why are inode_sb_list_add()/del() even called for a presumably 
reasonably well cached benchmark running on a system with enough RAM? Are these 
perhaps thousands of temporary files, already deleted, and released when all the 
file descriptors are closed as part of sys_exit()?

If that's the case then I suspect an even bigger win would be not just to batch 
the (sb-)global list fiddling, but to potentially turn the sb list into a 
percpu_alloc() managed set of per CPU lists? It's a bigger change, but it could 
speed up a lot of other temporary file intensive usecases as well, not just 
batched delete.

Thanks,

	Ingo

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

#1323361 — Re: [PATCH v2 3/3] vfs: Enable list batching for the superblock's inode list

> I'm wondering, why are inode_sb_list_add()/del() even called for a presumably 
> reasonably well cached benchmark running on a system with enough RAM? Are these 
> perhaps thousands of temporary files, already deleted, and released when all the 
> file descriptors are closed as part of sys_exit()?
> 
> If that's the case then I suspect an even bigger win would be not just to batch 
> the (sb-)global list fiddling, but to potentially turn the sb list into a 
> percpu_alloc() managed set of per CPU lists? It's a bigger change, but it could 

We had such a patch in the lock elision patchkit (It avoided a lot
of cache line bouncing leading to aborts)

https://git.kernel.org/cgit/linux/kernel/git/ak/linux-misc.git/commit/?h=hle315/combined&id=f1cf9e715a40f44086662ae3b29f123cf059cbf4

-Andi

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

#1323556 — Re: [PATCH v2 3/3] vfs: Enable list batching for the superblock's inode list

On 02/01/2016 12:45 PM, Andi Kleen wrote:
>> I'm wondering, why are inode_sb_list_add()/del() even called for a presumably
>> reasonably well cached benchmark running on a system with enough RAM? Are these
>> perhaps thousands of temporary files, already deleted, and released when all the
>> file descriptors are closed as part of sys_exit()?
>>
>> If that's the case then I suspect an even bigger win would be not just to batch
>> the (sb-)global list fiddling, but to potentially turn the sb list into a
>> percpu_alloc() managed set of per CPU lists? It's a bigger change, but it could
> We had such a patch in the lock elision patchkit (It avoided a lot
> of cache line bouncing leading to aborts)
>
> https://git.kernel.org/cgit/linux/kernel/git/ak/linux-misc.git/commit/?h=hle315/combined&id=f1cf9e715a40f44086662ae3b29f123cf059cbf4
>
> -Andi
>
>

I like your patch though it cannot be applied cleanly for the current 
upstream kernel. I will port it to the current kernel and run my 
microbenchmark to see what performance gain I can get.

Cheers,
Longman

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

#1326134 — Re: [PATCH v2 3/3] vfs: Enable list batching for the superblock's inode list

On 02/01/2016 05:03 PM, Waiman Long wrote:
> On 02/01/2016 12:45 PM, Andi Kleen wrote:
>>> I'm wondering, why are inode_sb_list_add()/del() even called for a 
>>> presumably
>>> reasonably well cached benchmark running on a system with enough 
>>> RAM? Are these
>>> perhaps thousands of temporary files, already deleted, and released 
>>> when all the
>>> file descriptors are closed as part of sys_exit()?
>>>
>>> If that's the case then I suspect an even bigger win would be not 
>>> just to batch
>>> the (sb-)global list fiddling, but to potentially turn the sb list 
>>> into a
>>> percpu_alloc() managed set of per CPU lists? It's a bigger change, 
>>> but it could
>> We had such a patch in the lock elision patchkit (It avoided a lot
>> of cache line bouncing leading to aborts)
>>
>> https://git.kernel.org/cgit/linux/kernel/git/ak/linux-misc.git/commit/?h=hle315/combined&id=f1cf9e715a40f44086662ae3b29f123cf059cbf4 
>>
>>
>> -Andi
>>
>>
>
> I like your patch though it cannot be applied cleanly for the current 
> upstream kernel. I will port it to the current kernel and run my 
> microbenchmark to see what performance gain I can get.
>

Unfortunately, using per-cpu list didn't have the performance benefit 
that I expected. I saw maybe 1 or 2% of performance increase, but 
nothing significant. I guess the bulk of the performance improvement in 
my patch is in the elimination of most of the cacheline transfer 
latencies when the lock ownership is passed from one CPU to another. 
Those latencies are still there even if we use the per-cpu list.

Cheers,
Longman

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

#1328390 — Re: [PATCH v2 3/3] vfs: Enable list batching for the superblock's inode list

On Wed, Feb 03, 2016 at 05:59:17PM -0500, Waiman Long wrote:
> On 02/01/2016 05:03 PM, Waiman Long wrote:
> >On 02/01/2016 12:45 PM, Andi Kleen wrote:
> >>>I'm wondering, why are inode_sb_list_add()/del() even called
> >>>for a presumably
> >>>reasonably well cached benchmark running on a system with
> >>>enough RAM? Are these
> >>>perhaps thousands of temporary files, already deleted, and
> >>>released when all the
> >>>file descriptors are closed as part of sys_exit()?
> >>>
> >>>If that's the case then I suspect an even bigger win would be
> >>>not just to batch
> >>>the (sb-)global list fiddling, but to potentially turn the sb
> >>>list into a
> >>>percpu_alloc() managed set of per CPU lists? It's a bigger
> >>>change, but it could
> >>We had such a patch in the lock elision patchkit (It avoided a lot
> >>of cache line bouncing leading to aborts)
> >>
> >>https://git.kernel.org/cgit/linux/kernel/git/ak/linux-misc.git/commit/?h=hle315/combined&id=f1cf9e715a40f44086662ae3b29f123cf059cbf4
> >>
> >>
> >>-Andi
> >>
> >>
> >
> >I like your patch though it cannot be applied cleanly for the
> >current upstream kernel. I will port it to the current kernel and
> >run my microbenchmark to see what performance gain I can get.
> >
> 
> Unfortunately, using per-cpu list didn't have the performance
> benefit that I expected. I saw maybe 1 or 2% of performance
> increase, but nothing significant. I guess the bulk of the
> performance improvement in my patch is in the elimination of most of
> the cacheline transfer latencies when the lock ownership is passed
> from one CPU to another. Those latencies are still there even if we
> use the per-cpu list.

Now that I look at the above patch, it doesn't get rid of the global
list lock. hence it won't change any of the existing global lock
cacheline contention. The list structure change to per-cpu is
completely irrelevant because it doesn't address the problem being
seen.

A proper per-cpu list implementation will provide either per-cpu
locks or some other mechanism to protect each list and eliminate a
large amount of global cacheline bouncing. Given this, I would not
use the above patch and results as a reason for saying this approach
will not work, or be a better solution....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

#1323537 — Re: [PATCH v2 3/3] vfs: Enable list batching for the superblock's inode list

[Multipart message — attachments visible in raw view] — view raw

On 01/30/2016 03:35 AM, Ingo Molnar wrote:
> * Waiman Long<Waiman.Long@hpe.com>  wrote:
>
>> The inode_sb_list_add() and inode_sb_list_del() functions in the vfs
>> layer just perform list addition and deletion under lock. So they can
>> use the new list batching facility to speed up the list operations
>> when many CPUs are trying to do it simultaneously.
>>
>> In particular, the inode_sb_list_del() function can be a performance
>> bottleneck when large applications with many threads and associated
>> inodes exit. With an exit microbenchmark that creates a large number
>> of threads, attachs many inodes to them and then exits. The runtimes
>> of that microbenchmark with 1000 threads before and after the patch
>> on a 4-socket Intel E7-4820 v3 system (48 cores, 96 threads) were
>> as follows:
>>
>>    Kernel        Elapsed Time    System Time
>>    ------        ------------    -----------
>>    Vanilla 4.4      65.29s         82m14s
>>    Patched 4.4      45.69s         49m44s
>>
>> The elapsed time and the reported system time were reduced by 30%
>> and 40% respectively.
> That's pretty impressive!
>
> I'm wondering, why are inode_sb_list_add()/del() even called for a presumably
> reasonably well cached benchmark running on a system with enough RAM? Are these
> perhaps thousands of temporary files, already deleted, and released when all the
> file descriptors are closed as part of sys_exit()?

The inodes that need to be deleted were actually procfs files which have 
to go away when the processes/threads exit. I encountered this problem 
when running the SPECjbb2013 benchmark on large machine where sometimes 
it might seems to hang for 30 mins or so after the benchmark complete. I 
wrote a simple microbenchmark to simulate this situation which is in the 
attachment.


> If that's the case then I suspect an even bigger win would be not just to batch
> the (sb-)global list fiddling, but to potentially turn the sb list into a
> percpu_alloc() managed set of per CPU lists? It's a bigger change, but it could
> speed up a lot of other temporary file intensive usecases as well, not just
> batched delete.
>
> Thanks,
>
> 	Ingo

Yes, that can be another possible. I will investigate further on that 
one. Thanks for the suggestion.

Cheers,
Longman

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

#1322760 — Re: [PATCH v2 3/3] vfs: Enable list batching for the superblock's inode list

On Fri, Jan 29, 2016 at 02:30:46PM -0500, Waiman Long wrote:
> The inode_sb_list_add() and inode_sb_list_del() functions in the vfs
> layer just perform list addition and deletion under lock. So they can
> use the new list batching facility to speed up the list operations
> when many CPUs are trying to do it simultaneously.
> 
> In particular, the inode_sb_list_del() function can be a performance
> bottleneck when large applications with many threads and associated
> inodes exit. With an exit microbenchmark that creates a large number
> of threads, attachs many inodes to them and then exits. The runtimes
> of that microbenchmark with 1000 threads before and after the patch

I've never seen sb inode list contention in typical workloads in
exit processing. Can you post the test script you are using?

The inode sb list contention I usually often than not, it's
workloads that turn over the inode cache quickly (i.e. instantiating
lots of inodes through concurrent directory traversal or create
workloads). These are often latency sensitive, so I'm wondering what
the effect of spinning waiting for batch processing on every
contended add is going to do to lookup performance...

> on a 4-socket Intel E7-4820 v3 system (48 cores, 96 threads) were
> as follows:
> 
>   Kernel        Elapsed Time    System Time
>   ------        ------------    -----------
>   Vanilla 4.4      65.29s         82m14s
>   Patched 4.4      45.69s         49m44s

I wonder if you'd get the same results on such a benchmark simply by
making the spin lock a mutex, thereby reducing the number of CPUs
spinning on a single lock cacheline at any one point in time.
Certainly the system time will plummet....

> The elapsed time and the reported system time were reduced by 30%
> and 40% respectively.
> 
> Signed-off-by: Waiman Long <Waiman.Long@hpe.com>
> ---
>  fs/inode.c         |   13 +++++--------
>  fs/super.c         |    1 +
>  include/linux/fs.h |    2 ++
>  3 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index 9f62db3..870de8c 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -424,19 +424,16 @@ static void inode_lru_list_del(struct inode *inode)
>   */
>  void inode_sb_list_add(struct inode *inode)
>  {
> -	spin_lock(&inode->i_sb->s_inode_list_lock);
> -	list_add(&inode->i_sb_list, &inode->i_sb->s_inodes);
> -	spin_unlock(&inode->i_sb->s_inode_list_lock);
> +	do_list_batch(&inode->i_sb->s_inode_list_lock, lb_cmd_add,
> +			&inode->i_sb->s_list_batch, &inode->i_sb_list);

I don't like the API. This should simply be:

void inode_sb_list_add(struct inode *inode)
{
	list_batch_add(&inode->i_sb_list, &inode->i_sb->s_inodes);
}

void inode_sb_list_del(struct inode *inode)
{
	list_batch_del(&inode->i_sb_list, &inode->i_sb->s_inodes);
}

And all the locks, lists and batch commands are internal to the
struct list_batch and the API implementation.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

#1326116 — Re: [PATCH v2 3/3] vfs: Enable list batching for the superblock's inode list

On 01/31/2016 07:04 PM, Dave Chinner wrote:
> On Fri, Jan 29, 2016 at 02:30:46PM -0500, Waiman Long wrote:
>> The inode_sb_list_add() and inode_sb_list_del() functions in the vfs
>> layer just perform list addition and deletion under lock. So they can
>> use the new list batching facility to speed up the list operations
>> when many CPUs are trying to do it simultaneously.
>>
>> In particular, the inode_sb_list_del() function can be a performance
>> bottleneck when large applications with many threads and associated
>> inodes exit. With an exit microbenchmark that creates a large number
>> of threads, attachs many inodes to them and then exits. The runtimes
>> of that microbenchmark with 1000 threads before and after the patch
> I've never seen sb inode list contention in typical workloads in
> exit processing. Can you post the test script you are using?

I have posted it in one of my earlier email.


> The inode sb list contention I usually often than not, it's
> workloads that turn over the inode cache quickly (i.e. instantiating
> lots of inodes through concurrent directory traversal or create
> workloads). These are often latency sensitive, so I'm wondering what
> the effect of spinning waiting for batch processing on every
> contended add is going to do to lookup performance...

I think the batch processor will get higher latency, but the other will 
see a shorter one. If each CPU has a more or less chance to become the 
batch processor, the overall impact to system performance should not be 
that significatn.

>> on a 4-socket Intel E7-4820 v3 system (48 cores, 96 threads) were
>> as follows:
>>
>>    Kernel        Elapsed Time    System Time
>>    ------        ------------    -----------
>>    Vanilla 4.4      65.29s         82m14s
>>    Patched 4.4      45.69s         49m44s
> I wonder if you'd get the same results on such a benchmark simply by
> making the spin lock a mutex, thereby reducing the number of CPUs
> spinning on a single lock cacheline at any one point in time.
> Certainly the system time will plummet....

I don't think it is a good idea to use mutex as we can't sleep.

>> The elapsed time and the reported system time were reduced by 30%
>> and 40% respectively.
>>
>> Signed-off-by: Waiman Long<Waiman.Long@hpe.com>
>> ---
>>   fs/inode.c         |   13 +++++--------
>>   fs/super.c         |    1 +
>>   include/linux/fs.h |    2 ++
>>   3 files changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/inode.c b/fs/inode.c
>> index 9f62db3..870de8c 100644
>> --- a/fs/inode.c
>> +++ b/fs/inode.c
>> @@ -424,19 +424,16 @@ static void inode_lru_list_del(struct inode *inode)
>>    */
>>   void inode_sb_list_add(struct inode *inode)
>>   {
>> -	spin_lock(&inode->i_sb->s_inode_list_lock);
>> -	list_add(&inode->i_sb_list,&inode->i_sb->s_inodes);
>> -	spin_unlock(&inode->i_sb->s_inode_list_lock);
>> +	do_list_batch(&inode->i_sb->s_inode_list_lock, lb_cmd_add,
>> +			&inode->i_sb->s_list_batch,&inode->i_sb_list);
> I don't like the API. This should simply be:
>
> void inode_sb_list_add(struct inode *inode)
> {
> 	list_batch_add(&inode->i_sb_list,&inode->i_sb->s_inodes);
> }
>
> void inode_sb_list_del(struct inode *inode)
> {
> 	list_batch_del(&inode->i_sb_list,&inode->i_sb->s_inodes);
> }
>
> And all the locks, lists and batch commands are internal to the
> struct list_batch and the API implementation.
>

Points taken. I will update the patch to do that. Thanks for the review.

Cheers,
Longman

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web