At Thu, 16 Jan 2014 16:32:42 +0800, Liu Yuan wrote: > > On Thu, Jan 16, 2014 at 04:58:46PM +0900, Hitoshi Mitake wrote: > > At Thu, 16 Jan 2014 14:11:44 +0800, > > Liu Yuan wrote: > > > > > > On Mon, Jan 13, 2014 at 05:40:38PM +0900, Hitoshi Mitake wrote: > > > > This patch introduces deferred event register/unregister > > > > mechanism. Newly added APIs are: > > > > - deferred_register_event(): thread safe register_event() > > > > - deferred_register_event_prio(): thread safe register_event_prio() > > > > - deferred_unregister_event(): thread safe unregister_event() > > > > > > 'deferred' doesn't look a good name. 'wk' is better to indicate that it is used > > > by worker thread context. > > > > As you say, "deferred" is not a good prefix. I agree. But "wk" would > > be too short and hard to interpret. How about "worker" > > e.g. worker_register_event()? > > > > > > > > > > > > > These functions can be called by worker threads safely. They allocate > > > > data structure which represents registering/unregistering event add > > > > queue it to the list shared with the main thread. After queuing, > > > > the main thread registers and unregisters events in a safe way. > > > > > > > > Signed-off-by: Hitoshi Mitake <mitake.hitoshi at lab.ntt.co.jp> > > > > --- > > > > include/event.h | 5 +++ > > > > lib/event.c | 132 ++++++++++++++++++++++++++++++++++++++++++++++++++------ > > > > 2 files changed, 124 insertions(+), 13 deletions(-) > > > > > > > > diff --git a/include/event.h b/include/event.h > > > > index 8f8b21f..b64b06e 100644 > > > > --- a/include/event.h > > > > +++ b/include/event.h > > > > @@ -32,4 +32,9 @@ static inline int register_event(int fd, event_handler_t h, void *data) > > > > return register_event_prio(fd, h, data, EVENT_PRIO_DEFAULT); > > > > } > > > > > > > > +void deferred_register_event_prio(int fd, event_handler_t h, void *data, > > > > + int prio); > > > > +void deferred_register_event(int fd, event_handler_t h, void *data); > > > > +void deferred_unregister_event(int fd); > > > > + > > > > #endif > > > > diff --git a/lib/event.c b/lib/event.c > > > > index 88078f4..2549dcd 100644 > > > > --- a/lib/event.c > > > > +++ b/lib/event.c > > > > @@ -76,19 +76,6 @@ static int event_cmp(const struct event_info *e1, const struct event_info *e2) > > > > return intcmp(e1->fd, e2->fd); > > > > } > > > > > > > > -int init_event(int nr) > > > > -{ > > > > - nr_events = nr; > > > > - events = xcalloc(nr_events, sizeof(struct epoll_event)); > > > > - > > > > - efd = epoll_create(nr); > > > > - if (efd < 0) { > > > > - sd_err("failed to create epoll fd"); > > > > - return -1; > > > > - } > > > > - return 0; > > > > -} > > > > - > > > > static struct event_info *lookup_event(int fd) > > > > { > > > > struct event_info key = { .fd = fd }; > > > > @@ -224,3 +211,122 @@ void event_loop_prio(int timeout) > > > > { > > > > do_event_loop(timeout, true); > > > > } > > > > + > > > > +struct deferred_event_info { > > > > + bool is_register; /* true: register, false: unregister */ > > > > + > > > > + int fd; > > > > + event_handler_t h; > > > > + void *data; > > > > + int prio; > > > > + > > > > + struct list_node list; > > > > +}; > > > > + > > > > +static LIST_HEAD(deferred_event_list); > > > > +static struct sd_mutex deferred_event_mutex = SD_MUTEX_INITIALIZER; > > > > + > > > > +static int deferred_event_fd; > > > > + > > > > +static void add_deferred_event_info(struct deferred_event_info *info) > > > > +{ > > > > + sd_mutex_lock(&deferred_event_mutex); > > > > + list_add_tail(&info->list, &deferred_event_list); > > > > + sd_mutex_unlock(&deferred_event_mutex); > > > > + > > > > + eventfd_xwrite(deferred_event_fd, 1); > > > > + event_force_refresh(); > > > > +} > > > > + > > > > +void deferred_register_event_prio(int fd, event_handler_t h, void *data, > > > > + int prio) > > > > +{ > > > > + struct deferred_event_info *info = xzalloc(sizeof(*info)); > > > > + > > > > + info->is_register = true; > > > > + > > > > + info->fd = fd; > > > > + info->h = h; > > > > + info->data = data; > > > > + info->prio = prio; > > > > + > > > > + add_deferred_event_info(info); > > > > +} > > > > + > > > > +void deferred_register_event(int fd, event_handler_t h, void *data) > > > > +{ > > > > + deferred_register_event_prio(fd, h, data, EVENT_PRIO_DEFAULT); > > > > +} > > > > > > so why deferred_register_event() (which is used by async handler reg) has the > > > priority over register_event()? > > > > Registering an event with the default priority doesn't break > > the assumption of exec_local_req_async(). Because the registration > > itself is done in the highest priority. Quoting from modified > > init_event(): > > > > + ret = register_event_prio(deferred_event_fd, deferred_event_handler, > > + NULL, EVENT_PRIO_MAX); > > > > But I found a problem that sheep doesn't use event_loop_prio(). I'll > > modify the problem in the next version, thanks for your question. > > > > Since current code works well, do we really need to introduce prioritized event > loop which need some overhead of sorting events? It would be a timing problem. If epoll selects sys->local_req_efd first, my patchset will not work well. We shouldn't have any assumption about the order of events. # This is just a follow up. If Kazutaka-san can provide simpler # approach, of course I'll discard this patchset. Thanks, Hitoshi |