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.hito...@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
--
sheepdog mailing list
sheepdog@lists.wpkg.org
http://lists.wpkg.org/mailman/listinfo/sheepdog
