Re: [Libevent-users] epoll erros

[Nicholas Marriott <nicholas.marriott@xxxxxxxxx>]

Hi

On Sun, Oct 24, 2010 at 01:03:25PM -0400, Nick Mathewson wrote:
> On Fri, Oct 22, 2010 at 4:25 PM, Nicholas Marriott
> <nicholas.marriott@xxxxxxxxx> wrote:
> > I think something needs to walk the file->f_ep_links list on close() and
> > remove any epitems where epi->ffd->fd is the fd being closed from the
> > tree in epi->ep.
> >
> > I don't have a Linux box to try this on though :-).
> 
> I think you're right.  I don't do kernel hacking myself though, so
> maybe somebody else should take this one on.

I'll add it to my list of things to try and do sometime but if someone
out there has a Linux environment already set up for kernel hacking (or
even knows who to report this to offhand) maybe they'll want to take a
look first.

> 
> In the meantime, this doesn't help people using any of the kernels
> currently in the wild.  Fortunately, part of the point of Libevent is
> to work around weird kernel behaviors.  Since the epoll tree _does_
> keep a separate struct epitem for every (file, fd) pair, I believe
> that we can work around this behavior by having epoll fall back to
> EPOLL_CTL_MOD when EPOLL_CTL_ADD fails with EEXIST.

I think this makes sense and from what I can see in the epoll code it
looks like it should work fine.

Cheers

> 
> Incidentally, there's another, more sinister bug hiding here.  When
> Libevent's changelist code coalesces a (DEL,ADD) combo, rather than
> making it a no-op, it makes it into an "ADD", since it's possible that
> the fd was closed in between the DEL and the ADD.  It calls these adds
> "precautionary".  When the current epoll code encounters such an add
> and the add fails with EEXIST, we currently ignore the failure with a
> debug message.  But if the user has been using dup() in the same way
> as Gilad's original code, this means that they'll think they have
> added the event, when in fact they haven't.
> 
> I've attached a series of two patches that try to get working behavior
> here.  See the commit messages for details.  Please test them if you
> can; they'll probably go into 2.0.9 if nobody finds them to be buggy.
> They are also branch epoll_dup in my github repo.
> 
> In the process, I also decided that maybe epoll_apply_changes should
> have fewer conditional branches, since they tend to be a poor choice
> on modern CPUs.  I've got a branch epoll_table in my github repo that
> replaces the huge pile of conditionals at the start of
> eopll_apply_changes with a table lookup.  It is *NOT* under
> consideration for 2.0, since it's a relatively large change and it
> doesn't fix a bug, but if anybody wants to have a look, that would be
> cool tool.
> 
> (My github repo is at git://github.com/nmathewson/Libevent.git )
> 
> yrs,
> -- 
> Nick



***********************************************************************
To unsubscribe, send an e-mail to majordomo@xxxxxxxxxxxxx with
unsubscribe libevent-users    in the body.