[sheepdog] [PATCH 1/2] lib: prioritize events for epoll() and enhance event loop

[Hitoshi Mitake]

At Fri, 21 Dec 2012 13:20:20 +0800,
Liu Yuan wrote:
> 
> On 12/20/2012 05:41 PM, Hitoshi Mitake wrote:
> > From: Hitoshi Mitake <h.mitake at gmail.com>
> > 
> > This patch adds two new functionalities to event_loop():
> > 1. prioritize registered events
> > 2. more flexible control with return values from event handlers
> > 
> > I'd like to describe the objectives of two functionalities.
> > 
> > 1. Current event_loop() treats priorities of every registered event
> > equally. But in some case, the way of treating them unequally is helpful.
> > 
> > 2. Previous event handlers returned no values. This patch let them
> > return newly defined enum value: event_ret. event_ret has 3 values:
> > * EVENT_RET_DONE ... this value means no special actions are required.
> >   event loop should continue normally.
> > * EVENT_RET_REFRESH ... this value means event_loop() should call
> >   epoll_wait() again. If events which should be processed as soon as
> >   possible during the event handler, this value is returned. So the
> >   event can be processed before already rised events.
> 
> I don't get the REFRESH. does it means that the handler knows that some
> other events is already queued in the kernel and want to process them as
> soon as possible, then goto call epoll_wait() again to retrieve them?

Yes. I'm assuming events whose handlers are like
bs_thread_request_done(). The handler is called when eventfd_write()
is called on efd in work.c, and the write is done by other
handlers. If such an event should be processed before successor
events, this value is useful. REFRESH is not for processing events
like packet receiving, etc.

# sheepkeeper is a heavy user of REFRESH, mainly for processing
# leaving of sheeps. If you are wondering about the use case of it,
# I can send this patch as a part of sheepkeeper patchset. Which one
# do you prefer?

> 
> > * EVENT_RET_BREAK ... this value means event_loop() must break
> >   immediately.
> > 
> 
> Better rephrase as EVENT_LOOP_OKAY (event handle is done and continue to
> process next event if any), EVENT_LOOP_RETRIEVE(retrieve the new
> events), EVENT_LOOP_BREAK(break out the event loop and discard other
> events) and do as following:

How about EVENT_LOOP_NOP instead of EVENT_LOOP_OKAY? NOP means that it
doesn't require any actions like retrieving or breaking.

> 
> nr = epoll_wait(efd, polled_events, nr_events, TICK * 1000);
> if (nr < 0)
>     ...
> else if (nr) {
>     if (need_sort)
> sort:
>         sort events...
>     for (i = 0; i < nr; i++) {
>          ret = ei->handler(ei->fd, e, ei->data);
>          switch (ret) {
>          case EVENT_LOOP_OKAY:
>               break;
>          case EVENT_LOOP_RETRIEVE:
>               retrieve new events;
>               goto sort;
>          case EVENT_RET_BREAK:
>               goto out;
>     }
> out:
>     ...

Is this pseudo code different from the scheme of my patch?

> }
> > +static struct epoll_event *polled_events;
> > +static int nr_events;
> > +static struct epoll_event **sorted_events;
> > +
> 
> is it possible to use a single array?

OK, I'll try to reduce it.

Thanks,
Hitoshi