[sheepdog] [PATCH 0/3] introduce a thread safe mechanism for register/unregister event

[Hitoshi Mitake]

At Thu, 16 Jan 2014 13:43:28 +0800,
Liu Yuan wrote:
> 
> On Wed, Jan 15, 2014 at 06:38:41PM +0900, Hitoshi Mitake wrote:
> > At Wed, 15 Jan 2014 17:15:57 +0800,
> > Liu Yuan wrote:
> > > 
> > > On Wed, Jan 15, 2014 at 06:11:16PM +0900, Hitoshi Mitake wrote:
> > > > At Wed, 15 Jan 2014 11:27:36 +0800,
> > > > Liu Yuan wrote:
> > > > > 
> > > > > On Mon, Jan 13, 2014 at 07:15:47PM +0900, Hitoshi Mitake wrote:
> > > > > > At Mon, 13 Jan 2014 17:15:24 +0800,
> > > > > > Liu Yuan wrote:
> > > > > > > 
> > > > > > > On Mon, Jan 13, 2014 at 05:13:09PM +0800, Liu Yuan wrote:
> > > > > > > > On Mon, Jan 13, 2014 at 05:40:36PM +0900, Hitoshi Mitake wrote:
> > > > > > > > > The commit 6601e90cf2c5 (sheep: allow {register,unregister}_event to be  called
> > > > > > > > > in worker thread) introduced dangerous change of
> > > > > > > > > {register,unregister}_event(). The changed functions are halfway thread safe. It
> > > > > > > > > will be bug prone stuff and shoud be fixed in a correct manner. This patch adds
> > > > > > > > > real thread safe version functions for registering and unregistering events.
> > > > > > > > 
> > > > > > > > No, as we previously discussed, we need a high performance register/unregister
> > > > > > > > mechanism for worker threads. Currently, exec_local_req_async is only user of it
> > > > > > > > and current code serve it well.
> > > > > > > > 
> > > > > > > > Queuing reg/unreg in the main thread is too slow.
> > > > > > > 
> > > > > > > "2. make sure registeration is done before some other events"
> > > > > > > 
> > > > > > > can main thread reg/unreg satisfy above requirements? I don't think so.
> > > > > > 
> > > > > > The main thread reg/unreg satisfy the above requirement. The
> > > > > > requirement is satisfied by the event priority mechanism.
> > > > > > 
> > > > > > I think you are talking about that the main thread reg/unreg can lose
> > > > > > notifications to req->local_req_efd. But it cannot be
> > > > > > lost. Registering is always done before local_req_handler() because
> > > > > > the register/unregister is set as an event with highest priority.
> > > > > 
> > > > > This is my previous email about why your approah would fail.  It still holds
> > > > > true for your patch set. It is a waste of my time to search the mail box and
> > > > > paste it for you because this mail previously sent to you.
> > > > > 
> > > > > "
> > > > > I think this approach will not work for async request handling, e.g,
> > > > > 
> > > > > +       uatomic_inc(&iocb->count);
> > > > > +       req->iocb = iocb;
> > > > > +       register_event(req->local_req_efd, local_req_async_handler, req);
> > > > > +       submit_local_request(req);
> > > > > 
> > > > > I have to register the event before sumit_local_request().
> > > > > 
> > > > > If I don't and put registeration in a delegated thread, suppose that is called
> > > > > after request is already handled. So I'll miss the notification that the request
> > > > > is done and local_req_async_handler will never be called.
> > > > 
> > > > My approach can solve the above problem. The request handling is never
> > > > done before completion of deferred register event. Because the request
> > > > handling is invoked by writing to sys->local_req_efd. The writing
> > > > invokes calling of local_req_handler() in the main event loop. But
> > > > before calling of local_req_handler(), deferred_event_handler() is
> > > > called because defered_register_event() writes data to
> > > > deferred_event_fd and it is registered to the main event loop with
> > > > highest priority.
> > > > 
> > > > > And you are defensive of your patch set and so it is your responsibility to
> > > > > provide performance number. I think, trapping to main thread in a sequential
> > > > > manner is obviously much worse performance-wise than current scheme.
> > > > > 
> > > > 
> > > > OK, I'll compare the performance. Could you give me a benchmarking
> > > > script? There are some aspect of performance: latency, throughput,
> > > > IOPS, etc. I don't know which one is important thing for you so I need
> > > > a measurement method.
> > > 
> > > Since http is current only use of async handler, I'll do a simple test later
> > > with it and report back test script and performance number when I have time.
> > 
> > OK, thanks. I'm looking forward to seeing it.
> 
> I did a simple test and found no performance degration because local request must
> be trapped to main thread firstly to queue the request. So looks okay for me to
> trap to main thread to reg/unreg worker event.

Thanks a lot for your evaluation. It is a good news for me.

Thanks,
Hitoshi