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

[Liu Yuan]

On 12/21/2012 03:27 PM, Hitoshi Mitake wrote:
> 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?
> 

No, you'd better push prepare patches into the code first as much as
possible for better review.

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

Looks okay to me.

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

Yes, pseudo code. your previous code structure looks unnecessary complex
to me.

Thanks,
Yuan