[sheepdog] [PATCH 2/3] event: add deferred event register/unregister mechanism

[Hitoshi Mitake]

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