From: Jeff Layton <jlayton@kernel.org> To: NeilBrown <neilb@suse.de>, Linus Torvalds <torvalds@linux-foundation.org> Cc: yangerkun <yangerkun@huawei.com>, kernel test robot <rong.a.chen@intel.com>, LKML <linux-kernel@vger.kernel.org>, lkp@lists.01.org, Bruce Fields <bfields@fieldses.org>, Al Viro <viro@zeniv.linux.org.uk> Subject: Re: [locks] 6d390e4b5d: will-it-scale.per_process_ops -96.6% regression Date: Tue, 17 Mar 2020 11:59:24 -0400 [thread overview] Message-ID: <46d2c16f48f1fd4ad28a85099c59ae95a9997740.camel@kernel.org> (raw) In-Reply-To: <87k13jsyum.fsf@notabene.neil.brown.name> On Tue, 2020-03-17 at 09:45 +1100, NeilBrown wrote: > > + > > + /* > > + * Tell the world we're done with it - see comment at top > > + * of this function > > This comment might be misleading. The world doesn't care. > Only this thread cares where ->fl_blocker is NULL. We need the release > semantics when some *other* thread sets fl_blocker to NULL, not when > this thread does. > I don't think we need to spell that out and I'm not against using > store_release here, but locks_delete_block cannot race with itself, so > referring to the comment at the top of this function is misleading. > > So: > Reviewed-by: NeilBrown <neilb@suse.de> > > but I'm not totally happy with the comments. > > Thanks Neil. We can clean up the comments before merge. How about this revision to the earlier patch? I took the liberty of poaching your your proposed verbiage: ------------------8<--------------------- From c9fbfae0ab615e20de0bdf1ae7b27591d602f577 Mon Sep 17 00:00:00 2001 From: Jeff Layton <jlayton@kernel.org> Date: Mon, 16 Mar 2020 18:57:47 -0400 Subject: [PATCH] SQUASH: update with Neil's comments Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/locks.c | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-) diff --git a/fs/locks.c b/fs/locks.c index eaf754ecdaa8..e74075b0e8ec 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -741,8 +741,9 @@ static void __locks_wake_up_blocks(struct file_lock *blocker) wake_up(&waiter->fl_wait); /* - * Tell the world we're done with it - see comment at - * top of locks_delete_block(). + * The setting of fl_blocker to NULL marks the official "done" + * point in deleting a block. Paired with acquire at the top + * of locks_delete_block(). */ smp_store_release(&waiter->fl_blocker, NULL); } @@ -761,11 +762,23 @@ int locks_delete_block(struct file_lock *waiter) /* * If fl_blocker is NULL, it won't be set again as this thread "owns" * the lock and is the only one that might try to claim the lock. - * Because fl_blocker is explicitly set last during a delete, it's - * safe to locklessly test to see if it's NULL. If it is, then we know - * that no new locks can be inserted into its fl_blocked_requests list, - * and we can therefore avoid doing anything further as long as that - * list is empty. + * + * We use acquire/release to manage fl_blocker so that we can + * optimize away taking the blocked_lock_lock in many cases. + * + * The smp_load_acquire guarantees two things: + * + * 1/ that fl_blocked_requests can be tested locklessly. If something + * was recently added to that list it must have been in a locked region + * *before* the locked region when fl_blocker was set to NULL. + * + * 2/ that no other thread is accessing 'waiter', so it is safe to free + * it. __locks_wake_up_blocks is careful not to touch waiter after + * fl_blocker is released. + * + * If a lockless check of fl_blocker shows it to be NULL, we know that + * no new locks can be inserted into its fl_blocked_requests list, and + * can avoid doing anything further if the list is empty. */ if (!smp_load_acquire(&waiter->fl_blocker) && list_empty(&waiter->fl_blocked_requests)) @@ -778,8 +791,8 @@ int locks_delete_block(struct file_lock *waiter) __locks_delete_block(waiter); /* - * Tell the world we're done with it - see comment at top - * of this function + * The setting of fl_blocker to NULL marks the official "done" point in + * deleting a block. Paired with acquire at the top of this function. */ smp_store_release(&waiter->fl_blocker, NULL); spin_unlock(&blocked_lock_lock); -- 2.24.1
WARNING: multiple messages have this Message-ID (diff)
From: Jeff Layton <jlayton@kernel.org> To: lkp@lists.01.org Subject: Re: [locks] 6d390e4b5d: will-it-scale.per_process_ops -96.6% regression Date: Tue, 17 Mar 2020 11:59:24 -0400 [thread overview] Message-ID: <46d2c16f48f1fd4ad28a85099c59ae95a9997740.camel@kernel.org> (raw) In-Reply-To: <87k13jsyum.fsf@notabene.neil.brown.name> [-- Attachment #1: Type: text/plain, Size: 3759 bytes --] On Tue, 2020-03-17 at 09:45 +1100, NeilBrown wrote: > > + > > + /* > > + * Tell the world we're done with it - see comment at top > > + * of this function > > This comment might be misleading. The world doesn't care. > Only this thread cares where ->fl_blocker is NULL. We need the release > semantics when some *other* thread sets fl_blocker to NULL, not when > this thread does. > I don't think we need to spell that out and I'm not against using > store_release here, but locks_delete_block cannot race with itself, so > referring to the comment at the top of this function is misleading. > > So: > Reviewed-by: NeilBrown <neilb@suse.de> > > but I'm not totally happy with the comments. > > Thanks Neil. We can clean up the comments before merge. How about this revision to the earlier patch? I took the liberty of poaching your your proposed verbiage: ------------------8<--------------------- >From c9fbfae0ab615e20de0bdf1ae7b27591d602f577 Mon Sep 17 00:00:00 2001 From: Jeff Layton <jlayton@kernel.org> Date: Mon, 16 Mar 2020 18:57:47 -0400 Subject: [PATCH] SQUASH: update with Neil's comments Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/locks.c | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-) diff --git a/fs/locks.c b/fs/locks.c index eaf754ecdaa8..e74075b0e8ec 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -741,8 +741,9 @@ static void __locks_wake_up_blocks(struct file_lock *blocker) wake_up(&waiter->fl_wait); /* - * Tell the world we're done with it - see comment at - * top of locks_delete_block(). + * The setting of fl_blocker to NULL marks the official "done" + * point in deleting a block. Paired with acquire at the top + * of locks_delete_block(). */ smp_store_release(&waiter->fl_blocker, NULL); } @@ -761,11 +762,23 @@ int locks_delete_block(struct file_lock *waiter) /* * If fl_blocker is NULL, it won't be set again as this thread "owns" * the lock and is the only one that might try to claim the lock. - * Because fl_blocker is explicitly set last during a delete, it's - * safe to locklessly test to see if it's NULL. If it is, then we know - * that no new locks can be inserted into its fl_blocked_requests list, - * and we can therefore avoid doing anything further as long as that - * list is empty. + * + * We use acquire/release to manage fl_blocker so that we can + * optimize away taking the blocked_lock_lock in many cases. + * + * The smp_load_acquire guarantees two things: + * + * 1/ that fl_blocked_requests can be tested locklessly. If something + * was recently added to that list it must have been in a locked region + * *before* the locked region when fl_blocker was set to NULL. + * + * 2/ that no other thread is accessing 'waiter', so it is safe to free + * it. __locks_wake_up_blocks is careful not to touch waiter after + * fl_blocker is released. + * + * If a lockless check of fl_blocker shows it to be NULL, we know that + * no new locks can be inserted into its fl_blocked_requests list, and + * can avoid doing anything further if the list is empty. */ if (!smp_load_acquire(&waiter->fl_blocker) && list_empty(&waiter->fl_blocked_requests)) @@ -778,8 +791,8 @@ int locks_delete_block(struct file_lock *waiter) __locks_delete_block(waiter); /* - * Tell the world we're done with it - see comment at top - * of this function + * The setting of fl_blocker to NULL marks the official "done" point in + * deleting a block. Paired with acquire at the top of this function. */ smp_store_release(&waiter->fl_blocker, NULL); spin_unlock(&blocked_lock_lock); -- 2.24.1
next prev parent reply other threads:[~2020-03-17 15:59 UTC|newest] Thread overview: 110+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-03-08 14:03 [locks] 6d390e4b5d: will-it-scale.per_process_ops -96.6% regression kernel test robot 2020-03-08 14:03 ` kernel test robot 2020-03-09 14:36 ` Jeff Layton 2020-03-09 14:36 ` Jeff Layton 2020-03-09 15:52 ` Linus Torvalds 2020-03-09 15:52 ` Linus Torvalds 2020-03-09 17:22 ` Jeff Layton 2020-03-09 17:22 ` Jeff Layton 2020-03-09 19:09 ` Jeff Layton 2020-03-09 19:09 ` Jeff Layton 2020-03-09 19:53 ` Jeff Layton 2020-03-09 19:53 ` Jeff Layton 2020-03-09 21:42 ` NeilBrown 2020-03-09 21:42 ` NeilBrown 2020-03-09 21:58 ` Jeff Layton 2020-03-09 21:58 ` Jeff Layton 2020-03-10 7:52 ` kernel test robot 2020-03-10 7:52 ` kernel test robot 2020-03-09 22:11 ` Jeff Layton 2020-03-09 22:11 ` Jeff Layton 2020-03-10 3:24 ` yangerkun 2020-03-10 3:24 ` yangerkun 2020-03-10 7:54 ` kernel test robot 2020-03-10 7:54 ` kernel test robot 2020-03-10 12:52 ` Jeff Layton 2020-03-10 12:52 ` Jeff Layton 2020-03-10 14:18 ` yangerkun 2020-03-10 14:18 ` yangerkun 2020-03-10 15:06 ` Jeff Layton 2020-03-10 15:06 ` Jeff Layton 2020-03-10 17:27 ` Jeff Layton 2020-03-10 17:27 ` Jeff Layton 2020-03-10 21:01 ` NeilBrown 2020-03-10 21:01 ` NeilBrown 2020-03-10 21:14 ` Jeff Layton 2020-03-10 21:14 ` Jeff Layton 2020-03-10 21:21 ` NeilBrown 2020-03-10 21:21 ` NeilBrown 2020-03-10 21:47 ` Linus Torvalds 2020-03-10 21:47 ` Linus Torvalds 2020-03-10 22:07 ` Jeff Layton 2020-03-10 22:07 ` Jeff Layton 2020-03-10 22:31 ` Linus Torvalds 2020-03-10 22:31 ` Linus Torvalds 2020-03-11 22:22 ` NeilBrown 2020-03-11 22:22 ` NeilBrown 2020-03-12 0:38 ` Linus Torvalds 2020-03-12 0:38 ` Linus Torvalds 2020-03-12 4:42 ` NeilBrown 2020-03-12 4:42 ` NeilBrown 2020-03-12 12:31 ` Jeff Layton 2020-03-12 12:31 ` Jeff Layton 2020-03-12 22:19 ` NeilBrown 2020-03-12 22:19 ` NeilBrown 2020-03-14 1:11 ` Jeff Layton 2020-03-14 1:11 ` Jeff Layton 2020-03-12 16:07 ` Linus Torvalds 2020-03-12 16:07 ` Linus Torvalds 2020-03-14 1:31 ` Jeff Layton 2020-03-14 1:31 ` Jeff Layton 2020-03-14 2:31 ` NeilBrown 2020-03-14 2:31 ` NeilBrown 2020-03-14 15:58 ` Linus Torvalds 2020-03-14 15:58 ` Linus Torvalds 2020-03-15 13:54 ` Jeff Layton 2020-03-15 13:54 ` Jeff Layton 2020-03-16 5:06 ` NeilBrown 2020-03-16 5:06 ` NeilBrown 2020-03-16 11:07 ` Jeff Layton 2020-03-16 11:07 ` Jeff Layton 2020-03-16 17:26 ` Linus Torvalds 2020-03-16 17:26 ` Linus Torvalds 2020-03-17 1:41 ` yangerkun 2020-03-17 1:41 ` yangerkun 2020-03-17 14:05 ` yangerkun 2020-03-17 14:05 ` yangerkun 2020-03-17 16:07 ` Jeff Layton 2020-03-17 16:07 ` Jeff Layton 2020-03-18 1:09 ` yangerkun 2020-03-18 1:09 ` yangerkun 2020-03-19 17:51 ` Jeff Layton 2020-03-19 17:51 ` Jeff Layton 2020-03-19 19:23 ` Linus Torvalds 2020-03-19 19:23 ` Linus Torvalds 2020-03-19 19:24 ` Jeff Layton 2020-03-19 19:24 ` Jeff Layton 2020-03-19 19:35 ` Linus Torvalds 2020-03-19 19:35 ` Linus Torvalds 2020-03-19 20:10 ` Jeff Layton 2020-03-19 20:10 ` Jeff Layton 2020-03-16 22:45 ` NeilBrown 2020-03-16 22:45 ` NeilBrown 2020-03-17 15:59 ` Jeff Layton [this message] 2020-03-17 15:59 ` Jeff Layton 2020-03-17 21:27 ` NeilBrown 2020-03-17 21:27 ` NeilBrown 2020-03-18 5:12 ` kernel test robot 2020-03-18 5:12 ` kernel test robot 2020-03-16 4:26 ` NeilBrown 2020-03-16 4:26 ` NeilBrown 2020-03-11 1:57 ` yangerkun 2020-03-11 1:57 ` yangerkun 2020-03-11 12:52 ` Jeff Layton 2020-03-11 12:52 ` Jeff Layton 2020-03-11 13:26 ` yangerkun 2020-03-11 13:26 ` yangerkun 2020-03-11 22:15 ` NeilBrown 2020-03-11 22:15 ` NeilBrown 2020-03-10 7:50 ` kernel test robot 2020-03-10 7:50 ` kernel test robot
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=46d2c16f48f1fd4ad28a85099c59ae95a9997740.camel@kernel.org \ --to=jlayton@kernel.org \ --cc=bfields@fieldses.org \ --cc=linux-kernel@vger.kernel.org \ --cc=lkp@lists.01.org \ --cc=neilb@suse.de \ --cc=rong.a.chen@intel.com \ --cc=torvalds@linux-foundation.org \ --cc=viro@zeniv.linux.org.uk \ --cc=yangerkun@huawei.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.