From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Message-ID: <56B286E1.8090709@hpe.com> Date: Wed, 03 Feb 2016 18:01:53 -0500 From: Waiman Long MIME-Version: 1.0 To: Dave Chinner CC: Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , Alexander Viro , linux-fsdevel@vger.kernel.org, x86@kernel.org, linux-kernel@vger.kernel.org, Peter Zijlstra , Andi Kleen , Scott J Norton , Douglas Hatch Subject: Re: [PATCH v2 3/3] vfs: Enable list batching for the superblock's inode list References: <1454095846-19628-1-git-send-email-Waiman.Long@hpe.com> <1454095846-19628-4-git-send-email-Waiman.Long@hpe.com> <20160201000404.GP20456@dastard> In-Reply-To: <20160201000404.GP20456@dastard> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: 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 >> --- >> 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