From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de ([195.135.220.15]:57838 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755168AbcIWIOs (ORCPT ); Fri, 23 Sep 2016 04:14:48 -0400 Date: Fri, 23 Sep 2016 10:14:45 +0200 From: Jan Kara To: Jeff Layton Cc: Jan Kara , Andrew Morton , linux-fsdevel@vger.kernel.org, Heiner Kallweit Subject: Re: [PATCH] fsnotify: Cleanup spinlock assertions Message-ID: <20160923081445.GA32043@quack2.suse.cz> References: <1474537439-18919-1-git-send-email-jack@suse.cz> <1474566328.11527.4.camel@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1474566328.11527.4.camel@redhat.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Thu 22-09-16 13:45:28, Jeff Layton wrote: > On Thu, 2016-09-22 at 11:43 +0200, Jan Kara wrote: > > Use assert_spin_locked() macro instead of hand-made BUG_ON statements. > > > > Suggested-by: Heiner Kallweit > > Signed-off-by: Jan Kara > > --- > > fs/notify/fanotify/fanotify_user.c | 3 +-- > > fs/notify/notification.c | 9 +++------ > > 2 files changed, 4 insertions(+), 8 deletions(-) > > > > Andrew, can you please add this cleanup to the fanotify patches you carry? > > Thanks! > > > > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c > > index 189fab3ac4e6..7ebfca6a1427 100644 > > --- a/fs/notify/fanotify/fanotify_user.c > > +++ b/fs/notify/fanotify/fanotify_user.c > > @@ -54,8 +54,7 @@ struct kmem_cache *fanotify_perm_event_cachep __read_mostly; > > static struct fsnotify_event *get_one_event(struct fsnotify_group *group, > > size_t count) > > { > > - BUG_ON(IS_ENABLED(CONFIG_SMP) && > > - !spin_is_locked(&group->notification_lock)); > > + assert_spin_locked(&group->notification_lock); > > > > pr_debug("%s: group=%p count=%zd\n", __func__, group, count); > > > > diff --git a/fs/notify/notification.c b/fs/notify/notification.c > > index 1a8010e7a2a0..66f85c651c52 100644 > > --- a/fs/notify/notification.c > > +++ b/fs/notify/notification.c > > @@ -63,8 +63,7 @@ EXPORT_SYMBOL_GPL(fsnotify_get_cookie); > > /* return true if the notify queue is empty, false otherwise */ > > bool fsnotify_notify_queue_is_empty(struct fsnotify_group *group) > > { > > - BUG_ON(IS_ENABLED(CONFIG_SMP) && > > - !spin_is_locked(&group->notification_lock)); > > + assert_spin_locked(&group->notification_lock); > > return list_empty(&group->notification_list) ? true : false; > > } > > > > @@ -149,8 +148,7 @@ struct fsnotify_event *fsnotify_remove_first_event(struct fsnotify_group *group) > > { > > struct fsnotify_event *event; > > > > - BUG_ON(IS_ENABLED(CONFIG_SMP) && > > - !spin_is_locked(&group->notification_lock)); > > + assert_spin_locked(&group->notification_lock); > > > > pr_debug("%s: group=%p\n", __func__, group); > > > > @@ -172,8 +170,7 @@ struct fsnotify_event *fsnotify_remove_first_event(struct fsnotify_group *group) > > */ > > struct fsnotify_event *fsnotify_peek_first_event(struct fsnotify_group *group) > > { > > - BUG_ON(IS_ENABLED(CONFIG_SMP) && > > - !spin_is_locked(&group->notification_lock)); > > + assert_spin_locked(&group->notification_lock); > > > > return list_first_entry(&group->notification_list, > > struct fsnotify_event, list); > > Much cleaner. > > That said, I have a personal preference for lockdep_assert_held() in > these situations, which not only tells you whether the lock is locked, > but (I believe) whether it was locked by the current task as well. Yes, it does. > Theoretically you could have a different task take this spinlock, and > then call into here without holding it and not get the assertion since > it was locked at the time. Of course, that does require lockdep... Yeah, I personally don't have a strong preference. Both have advantages and disadvantages - as you said, lockdep_assert_held() is reliable when lockdep is enabled but there's much less testing happening with lockdep enabled and also lockdep changes the timing enough that some cases just need not trigger... > In any case, this is still an improvement: > > Reviewed-by: Jeff Layton Thanks! Honza -- Jan Kara SUSE Labs, CR