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

[Liu Yuan]

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?

For now with this patch set, the only benefit is that it looks more 'beautiful'
that we have complete reg/unreg for worker threads, which isn't must for making
code correct nor run faster.

The downside is obvious, that 1) reg/unreg in worker is worsen a bit 2) event loop
is affected negatively by sorting overhead. We don't get pratical benefits.

I want to get other developer's opinion.

Thanks
Yuan