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
Yuan
|
|