At Tue, 17 Dec 2013 16:43:08 +0900,
Hitoshi Mitake wrote:
>
> At Tue, 17 Dec 2013 15:36:45 +0800,
> Liu Yuan wrote:
> >
> > On Tue, Dec 17, 2013 at 04:26:09PM +0900, Hitoshi Mitake wrote:
> > > At Tue, 17 Dec 2013 15:11:30 +0800,
> > > Liu Yuan wrote:
> > > >
> > > > On Tue, Dec 17, 2013 at 03:58:07PM +0900, Hitoshi Mitake wrote:
> > > > > At Tue, 17 Dec 2013 14:50:19 +0800,
> > > > > Liu Yuan wrote:
> > > > > >
> > > > > > On Tue, Dec 17, 2013 at 03:42:29PM +0900, Hitoshi Mitake wrote:
> > > > > > > At Tue, 17 Dec 2013 14:31:56 +0800,
> > > > > > > Liu Yuan wrote:
> > > > > > > >
> > > > > > > > This allow us to call even handling functions in worker thread
> > > > > > > >
> > > > > > > > Signed-off-by: Liu Yuan <namei.u...@gmail.com>
> > > > > > > > ---
> > > > > > >
> > > > > > > I think this change is dangerous. This permits worker threads to
> > > > > > > unregister events even if these events are processed in the main
> > > > > > > thread. Making a new work queue and delegate it to
> > > > > > > register/unregister
> > > > > > > events would be safer.
> > > > > >
> > > > > > This scheme is not pratical for async request.
> > > > > >
> > > > > > This is just internal API, which are supported to be called by
> > > > > > programmers
> > > > > > and check it is correct.
> > > > >
> > > > > The checking will cost us lots of time. The problem is a race
> > > > > condition caused by multiple threads.
> > > >
> > > > Why we would face this kind of problem? we can make sure at any time,
> > > > there will
> > > > be a single thread manipulate it, be it worker or main thread.
> > > >
> > > > I think we are talking about different issues, you are supposed that
> > > > events
> > > > handling will be generically thread-safe for multiple threads. But this
> > > > wouldn't
> > > > happen I think. Instead, assumption 'one event will be only be
> > > > manipulated by
> > > > single entity (thus no multiple threads case)' will hold true in the
> > > > long run.
> > >
> > > As you say, your patch doesn't violate the above condition (one event
> > > will be
> > > only be manipulated by single entity). But we cannot express that
> > > register_event() and unregister_event() are thread safe in the above
> > > special
> > > condition. So users (we programmers) would use in an invalid way in the
> > > future
> > > and debugging will be hard.
> >
> > How about following commit log:
> >
> > sheep: allow {register,unregister}_event to be called in worker thread
> >
> > For now we can only call them in the main thread, which is designed for
> > long
> > running or infrequent events. This would be inefficient if we want to
> > deal with
> > short running and frequent events that register/unregister the events
> > in the
> > worker thread
> >
> > 1. avoid to be trapped to main thread for performance
> > 2. make sure registeration is done before some other events.
> >
> > This doesn't mean we can manipulate the same event with multiple threads
> > simultaneously, instead we still adhere to the assumpioin that
> >
> > - one event will only be manipluated by a single entity.
>
> We will never read all commit messages before using some functions. The
> description should be comments in source tree.
Let's wrap it up. If you add a summary of the above message to the source tree
as a comment, I'll ack the patch set.
Thanks,
Hitoshi
--
sheepdog mailing list
sheepdog@lists.wpkg.org
http://lists.wpkg.org/mailman/listinfo/sheepdog
