From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f67.google.com ([74.125.82.67]:42970 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752134AbdLGAtT (ORCPT ); Wed, 6 Dec 2017 19:49:19 -0500 Date: Thu, 7 Dec 2017 01:49:15 +0100 From: Ingo Molnar To: Christoph Hellwig , Peter Zijlstra Cc: Ingo Molnar , Peter Zijlstra , Andrew Morton , Al Viro , Jason Baron , linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/2] epoll: use the waitqueue lock to protect ep->wq Message-ID: <20171207004914.lxu2dz6cmxgnrou3@gmail.com> References: <20171206235230.19425-1-hch@lst.de> <20171206235230.19425-2-hch@lst.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171206235230.19425-2-hch@lst.de> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: * Christoph Hellwig wrote: > The eoll code currently always uses the unlocked waitqueue helpers for s/eoll /epoll > ep->wq, but instead of holding the lock inside the waitqueue around these > calls, as expected by the epoll code uses its own lock. Hm, that reads a bit weirdly. How about: The epoll code currently uses the unlocked waitqueue helpers for managing ep->wq, but instead of holding the waitqueue lock around these calls, it uses its own ep->lock spinlock. > Given that the > waitqueue is not exposed to the rest of the kernel this actually works > ok at the moment, but prevents the epoll locking rules from being > enforced using lockdep. Remove ep->lock and use the waitqueue lock > to not only reduce the size of struct eventpoll but also make sure we > can assert locking invariations in the waitqueue code. s/but also make sure but also to make sure s/invariations /invariants > Signed-off-by: Christoph Hellwig > --- > fs/eventpoll.c | 46 ++++++++++++++++++++++------------------------ > 1 file changed, 22 insertions(+), 24 deletions(-) > > diff --git a/fs/eventpoll.c b/fs/eventpoll.c > index afd548ebc328..2b2c5ac80e26 100644 > --- a/fs/eventpoll.c > +++ b/fs/eventpoll.c > @@ -182,11 +182,10 @@ struct epitem { > * This structure is stored inside the "private_data" member of the file > * structure and represents the main data structure for the eventpoll > * interface. > + * > + * Access to it is protected by the lock inside wq. > */ > struct eventpoll { > - /* Protect the access to this structure */ > - spinlock_t lock; > - > /* > * This mutex is used to ensure that files are not removed > * while epoll is using them. This is held during the event > @@ -210,7 +209,7 @@ struct eventpoll { > /* > * This is a single linked list that chains all the "struct epitem" that > * happened while transferring ready events to userspace w/out > - * holding ->lock. > + * holding ->wq.lock. > */ Neat trick! This exposes some waitqueue internals, but AFAICS the FUSE code already does a similar trick with fiq->waitq.lock so there's precedent. Peter, what do you think? Thanks, Ingo