Date:	Tue, 14 Aug 2012 15:13:55 -0700
From:	"Paton J. Lewis" <palewis@...be.com>
To:	Christof Meerwald <cmeerw@...erw.org>
CC:	Alexander Viro <viro@...iv.linux.org.uk>,
	Jason Baron <jbaron@...hat.com>,
	"linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Paul Holland <pholland@...be.com>,
	Davide Libenzi <davidel@...ilserver.org>
Subject: Re: [PATCH] epoll: Improved support for multi-threaded clients

At 8/14/2012 01:21 PM, Christof Meerwald wrote:
>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.

Good point.

> > 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.

Also true. However, I believe the context switch is always required 
by the custom message passing technique, but will not always happen 
when using the event disabling technique.


> > 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.

You are correct. Thanks for being patient and persistent here. I 
discovered this problem myself last week during testing, and I am 
planning to post a v2 patch proposal that includes this fix.

I am also working on an epoll self-test as Andrew Morton suggested. 
I'm going to finish that before re-submitting the EPOLL_CTL_DISABLE 
patch to help reduce the possibility that the v2 patch still contains bugs.

Pat


>Christof
>
>--
>
>http://cmeerw.org                              sip:cmeerw at cmeerw.org
>mailto:cmeerw at cmeerw.org                   xmpp:cmeerw at cmeerw.org

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists