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 |