[systemd-devel] [PATCH] sd-event: we do not support EPOLLONESHOT correctly
If this event is not the highest priority, then it will not be dispatched when the epoll triggers, and since we will not get any more wakeups (due to the way EPOLLONESHOT works) will never be dispatched. Since we only handle one event per epoll_wait() wakeup, and we dequeue a ONE_SHOT event when we dispatch it, we do not have the problem described in epoll(7) of "multiple events can be generated upon receipt of multiple chunks of data". --- src/libsystemd-bus/sd-event.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/libsystemd-bus/sd-event.c b/src/libsystemd-bus/sd-event.c index 462dd41..1cf661e 100644 --- a/src/libsystemd-bus/sd-event.c +++ b/src/libsystemd-bus/sd-event.c @@ -447,9 +447,6 @@ static int source_io_register( ev.events = events; ev.data.ptr = s; -if (enabled == SD_EVENT_ONESHOT) -ev.events |= EPOLLONESHOT; - if (s->io.registered) r = epoll_ctl(s->event->epoll_fd, EPOLL_CTL_MOD, s->io.fd, &ev); else -- 1.8.5.1 ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] sd-event: we do not support EPOLLONESHOT correctly
On Fri, 13.12.13 13:08, Shawn Landden (sh...@churchofgit.com) wrote: > If this event is not the highest priority, then it will not be > dispatched when the epoll triggers, and since we will not get any more > wakeups (due to the way EPOLLONESHOT works) will never be dispatched. Hmm, so the way this is supposed to work is that as soon as epoll told us about some event once, we will set the event source to "pending", and it stays "pending" until we have dispatched it, regardless if we ever get the event from epoll again or not. However, we currently will overwrite the "revents" field should we ever get it again with the new one. It probably makes more sense to OR it in, so that we don't forget readability if writability happens at a later point or vice versa, if you follow what I mean. I'll prep a fix for that. With that fixed is there something else to still fix? > Since we only handle one event per epoll_wait() wakeup, > and we dequeue a ONE_SHOT event when we dispatch it, we > do not have the problem described in epoll(7) of > "multiple events can be generated upon receipt of multiple > chunks of data". Not sure I follow here? That part appears to be about EPOLLET? Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] sd-event: we do not support EPOLLONESHOT correctly
On Fri, Dec 13, 2013 at 7:17 PM, Lennart Poettering wrote: > On Fri, 13.12.13 13:08, Shawn Landden (sh...@churchofgit.com) wrote: > >> If this event is not the highest priority, then it will not be >> dispatched when the epoll triggers, and since we will not get any more >> wakeups (due to the way EPOLLONESHOT works) will never be dispatched. > > Hmm, so the way this is supposed to work is that as soon as epoll told > us about some event once, we will set the event source to "pending", and > it stays "pending" until we have dispatched it, regardless if we ever > get the event from epoll again or not. However, we currently will > overwrite the "revents" field should we ever get it again with the new > one. It probably makes more sense to OR it in, so that we don't forget > readability if writability happens at a later point or vice versa, if > you follow what I mean. I'll prep a fix for that. good > > With that fixed is there something else to still fix? Oh I missed how the pending code worked. Because of that I think I can use EPOLLET with sd-event, and will send a Distribute= patch that uses it. (already have it, just wasn't sure if it was only working cause my test setup has near-0 load) > >> Since we only handle one event per epoll_wait() wakeup, >> and we dequeue a ONE_SHOT event when we dispatch it, we >> do not have the problem described in epoll(7) of >> "multiple events can be generated upon receipt of multiple >> chunks of data". > > Not sure I follow here? That part appears to be about EPOLLET? That part explains why EPOLLET cannot be used to implement EPOLLONESHOT. The reason is that even with EPOLLET, epoll_wait can generate multiple events for the same watch, if more than one connection came in. > > Lennart > > -- > Lennart Poettering, Red Hat > ___ > systemd-devel mailing list > systemd-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/systemd-devel ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] sd-event: we do not support EPOLLONESHOT correctly
On Fri, Dec 13, 2013 at 8:04 PM, Shawn Landden wrote: > On Fri, Dec 13, 2013 at 7:17 PM, Lennart Poettering > wrote: >> On Fri, 13.12.13 13:08, Shawn Landden (sh...@churchofgit.com) wrote: >> >>> If this event is not the highest priority, then it will not be >>> dispatched when the epoll triggers, and since we will not get any more >>> wakeups (due to the way EPOLLONESHOT works) will never be dispatched. >> >> Hmm, so the way this is supposed to work is that as soon as epoll told >> us about some event once, we will set the event source to "pending", and >> it stays "pending" until we have dispatched it, regardless if we ever >> get the event from epoll again or not. However, we currently will >> overwrite the "revents" field should we ever get it again with the new >> one. It probably makes more sense to OR it in, so that we don't forget >> readability if writability happens at a later point or vice versa, if >> you follow what I mean. I'll prep a fix for that. > good >> >> With that fixed is there something else to still fix? > Oh I missed how the pending code worked. Because of that I think I can > use EPOLLET > with sd-event, and will send a Distribute= patch that uses it. (already have > it, > just wasn't sure if it was only working cause my test setup has near-0 load) With either of these, can't we end up in a situation where we have events pending but are still blocking on epoll_wait()? shouldn't we do something like p = have_pending() epoll_wait(foo, foo, foo, p ? timeout : 0); do prioq stuff() mark_pending() dispatch_highest_prio() to avoid this issue? >> >>> Since we only handle one event per epoll_wait() wakeup, >>> and we dequeue a ONE_SHOT event when we dispatch it, we >>> do not have the problem described in epoll(7) of >>> "multiple events can be generated upon receipt of multiple >>> chunks of data". >> >> Not sure I follow here? That part appears to be about EPOLLET? > That part explains why EPOLLET cannot be used to implement EPOLLONESHOT. > The reason is that even with EPOLLET, epoll_wait can generate multiple > events for the > same watch, if more than one connection came in. >> >> Lennart >> >> -- >> Lennart Poettering, Red Hat >> ___ >> systemd-devel mailing list >> systemd-devel@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/systemd-devel ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] sd-event: we do not support EPOLLONESHOT correctly
On Fri, Dec 13, 2013 at 8:39 PM, Shawn Landden wrote: > On Fri, Dec 13, 2013 at 8:04 PM, Shawn Landden wrote: >> On Fri, Dec 13, 2013 at 7:17 PM, Lennart Poettering >> wrote: >>> On Fri, 13.12.13 13:08, Shawn Landden (sh...@churchofgit.com) wrote: >>> If this event is not the highest priority, then it will not be dispatched when the epoll triggers, and since we will not get any more wakeups (due to the way EPOLLONESHOT works) will never be dispatched. >>> >>> Hmm, so the way this is supposed to work is that as soon as epoll told >>> us about some event once, we will set the event source to "pending", and >>> it stays "pending" until we have dispatched it, regardless if we ever >>> get the event from epoll again or not. However, we currently will >>> overwrite the "revents" field should we ever get it again with the new >>> one. It probably makes more sense to OR it in, so that we don't forget >>> readability if writability happens at a later point or vice versa, if >>> you follow what I mean. I'll prep a fix for that. >> good >>> >>> With that fixed is there something else to still fix? >> Oh I missed how the pending code worked. Because of that I think I can >> use EPOLLET >> with sd-event, and will send a Distribute= patch that uses it. (already have >> it, >> just wasn't sure if it was only working cause my test setup has near-0 load) > With either of these, can't we end up in a situation where we have > events pending > but are still blocking on epoll_wait()? > > shouldn't we do something like > > p = have_pending() > > epoll_wait(foo, foo, foo, p ? timeout : 0); > > do prioq stuff() > mark_pending() > dispatch_highest_prio() > > to avoid this issue? NVM, i need to read the code better >>> Since we only handle one event per epoll_wait() wakeup, and we dequeue a ONE_SHOT event when we dispatch it, we do not have the problem described in epoll(7) of "multiple events can be generated upon receipt of multiple chunks of data". >>> >>> Not sure I follow here? That part appears to be about EPOLLET? >> That part explains why EPOLLET cannot be used to implement EPOLLONESHOT. >> The reason is that even with EPOLLET, epoll_wait can generate multiple >> events for the >> same watch, if more than one connection came in. >>> >>> Lennart >>> >>> -- >>> Lennart Poettering, Red Hat >>> ___ >>> systemd-devel mailing list >>> systemd-devel@lists.freedesktop.org >>> http://lists.freedesktop.org/mailman/listinfo/systemd-devel ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
