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

[Liu Yuan]

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.

How do we handle above problem if we use your approach?

If we take performance into account, we can't do it in your approch either.
Suppose we will issue 1000 requests in a async mode, we can't affort to be trapped
to main thread to register event for every request.
"
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.

Thanks
Yuan