From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christof Meerwald Subject: Re: [PATCH] epoll: Improved support for multi-threaded clients Date: Tue, 14 Aug 2012 22:21:25 +0200 Message-ID: <20120814202125.GH1407@edge.cmeerw.net> References: <20120616184707.GA22656@edge.cmeerw.net> <6.2.5.6.2.20120618161807.031eb6c8@adobe.com> <20120619181711.GE1281@edge.cmeerw.net> <6.2.5.6.2.20120629140909.04bb0a40@adobe.com> <6.2.5.6.2.20120802174226.04afdcd0@adobe.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Alexander Viro , Jason Baron , "linux-fsdevel@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Paul Holland , Davide Libenzi To: "Paton J. Lewis" Return-path: Content-Disposition: inline In-Reply-To: <6.2.5.6.2.20120802174226.04afdcd0@adobe.com> Sender: linux-kernel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org Hi Paton, On Thu, Aug 02, 2012 at 06:37:06PM -0700, Paton J. Lewis wrote: [...] > My first concern is about code clarity. Using a custom event to > delete an event type (either EPOLLIN or EPOLLOUT) from an epoll item > requires that functionality to be split across two areas of code: > the code that requests the deletion (via the call to epoll_ctl), and > the code that responds to it (via epoll_wait). But don't you have a similar problem in your proposal as well as you might get an EBUSY when trying to disabling the item - in which case you would have to do the deletion in the epoll_wait loop. > However, my main concern is about performance. Handling a custom > event means that each return from epoll_wait requires the responding > thread to check for possible custom events, which in the case of > deletion is going to be relatively rare. Thus code which was once > purely concerned with responding to I/O events must now spend a > fraction of its time testing for exceptional conditions. In > addition, handling deletion in this manner now requires a thread or > context switch. But in your initial proposal you also had the code checking for deletion in the epoll_wait loop. > Given the drawbacks listed above, and the kernel design philosophy > of only implementing what is actually needed, I would argue for > sticking with the original EPOLL_CTL_DISABLE proposal for now. I have finally had some chance to play around with your patch a bit and I really think that you don't want to check for ep_is_linked(&epi->rdllink) in ep_disable as I don't see that this would provide any useful semantics with respect to race-conditions. I.e. consider the point in the epoll_wait loop just after you have re-enabled to item - in this case ep_disable would (almost certainly) return EBUSY, but there is no guarantee that epoll_wait will be woken up on the next iteration. As I mentioned, I think it would be much more useful to check for "epi->event.events & ~EP_PRIVATE_BITS" instead which I believe would provide more useful semantics. Christof -- http://cmeerw.org sip:cmeerw at cmeerw.org mailto:cmeerw at cmeerw.org xmpp:cmeerw at cmeerw.org