From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Jackson Subject: Re: [PATCH] tools/libxl: Fixes to stream v2 task joining logic Date: Fri, 24 Jul 2015 12:41:29 +0100 Message-ID: <21938.9321.296666.298603@mariner.uk.xensource.com> References: <1437649778-16728-1-git-send-email-andrew.cooper3@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1437649778-16728-1-git-send-email-andrew.cooper3@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andrew Cooper Cc: Wei Liu , Ian Campbell , Xen-devel List-Id: xen-devel@lists.xenproject.org Andrew Cooper writes ("[PATCH] tools/libxl: Fixes to stream v2 task joining logic"): > During review of the libxl migration v2 series, I changes the task > joining logic, but clearly didn't think the result through > properly. This patch fixes several errors. This would have been much easier to review if it had been split into 3 patches. I have gone mostly by the commit message because it was hard to see hunk belonged to what. > 3) Avoid stacking of check_all_finished() via synchronous teardown of > tasks. If the _abort() functions call back synchronously, > stream->completion_callback() ends up getting called twice, as first and > last check_all_finished() frames observe each task being finished. I think this part of the patch is fine. > 1) Do not call check_all_finished() in the success cases of > libxl__xc_domain_{save,restore}_done(). It serves no specific purpose > as the save helper state will report itself as inactive by this point, > and avoids triggering a second stream->completion_callback() in the case > that write_toolstack_record()/stream_continue() record errors > synchronously themselves. "Serves no specific purpose" other than having a single exit path, which makes matters much less confusing. I think the problem may be that libxl__xc_domain_{save,restore}_done fail to "return" after "write_toolstack_record" and "stream_continue". That seems like simply a bug. I'm sorry that I didn't notice it in review. In general each callback function should set up exactly one other callback. If it does anything else then reentrancy hazards arise. Also, it is confusing and perhaps wrong that write_toolstack_record calls stream_complete. What if there are other threads of control outstanding ? > 2) Only ever set stream->rc in stream_complete(). The first version of > the migration v2 series had separate rc and joined_rc parameters, where > this logic worked. However when combining the two, the teardown path > fails to trigger if stream_done() records stream->rc itself. A side > effect of this is that stream_done() needs to take an rc parameter. "the teardown path fails to trigger if stream_done() records stream->rc itself" but in the code I am looking at neither of the functions stream_done assign to rc. Maybe I would understand this if I were looking at only this set of diff hunks. Ian.