Re: [Qemu-devel] [RFC v5 15/26] monitor: let suspend_cnt be thread safe

[Peter Xu <peterx@redhat.com>]

On Sat, Dec 16, 2017 at 09:11:08AM +0000, Stefan Hajnoczi wrote:
> On Sat, Dec 16, 2017 at 02:12:11PM +0800, Peter Xu wrote:
> > On Wed, Dec 13, 2017 at 06:43:58PM +0000, Stefan Hajnoczi wrote:
> > > On Tue, Dec 05, 2017 at 01:51:49PM +0800, Peter Xu wrote:
> > > > Monitor code now can be run in more than one thread.  Let it be thread
> > > > safe when accessing suspend_cnt counter.
> > > 
> > > Please add doc comments to the functions explaining which thread they
> > > may be called from (especially public functions).  Without this
> > > information other people will have a hard time keeping code thread-safe.
> > 
> > The two public functions would be monitor_resume() and
> > monitor_suspend().  IIUC as long as these functions are using atomic
> > operations correctly they should be able to be called anywhere?
> > 
> > Could you help point out what I missed?
> 
> I don't mean just the public functions.  How are other people supposed
> to know which monitor function executes in which thread without first
> studying all the monitor code?  This is where comments, naming schemes,
> or grouping functions helps.
> 
> Not every function needs to be documented in detail, but there needs to
> be some structure to help people understand what happens in the IOThread
> and what happens in the main loop.
> 
> Just adding threading to the existing code without leaving clues for
> people who come after you is likely to result in future bugs.

Yes, I fully agree with you that we need to do better commenting when
doing things like this.

I am just not sure what you would prefer me to add since AFAICT these
functions (after with the atomic protections) are really free to be
called anywhere, and also in any thread (even newly created threads
rather than main thread or monitor IO thread), and they are free of
locks too, like monitor_lock.  Here I only mean monitor_suspend() and
monitor_resume() for sure.  For the rest of functions, IMHO since they
are internal to monitor code so people who will call them do need to
know something about monitors.

I would be more appreciated if you can provide some examples here to
show what you mean.  Thanks,

-- 
Peter Xu