From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] tools/libxl: Fixes to stream v2 task joining logic Date: Mon, 27 Jul 2015 15:30:34 +0100 Message-ID: <55B6408A.2070400@citrix.com> References: <1437649778-16728-1-git-send-email-andrew.cooper3@citrix.com> <21938.9321.296666.298603@mariner.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <21938.9321.296666.298603@mariner.uk.xensource.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: Ian Jackson Cc: Wei Liu , Ian Campbell , Xen-devel List-Id: xen-devel@lists.xenproject.org On 24/07/15 12:41, Ian Jackson wrote: > 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. I have split them up. > >> 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. "Serves no specific purpose" in so far as what check_all_finished() would do in the success case. > > 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. After some more thought, I don't believe that my fix is necessarily correct. If a condition were to exist where the stream had recorded an error and abort()'ed the save helper, but the save helper was already exiting with a success condition, then the callback wouldn't be fired at all. I have a proposed alternate solution. > > In general each callback function should set up exactly one other > callback. If it does anything else then reentrancy hazards arise. The entire point of this logic is that there are multiple operations going on in parallel. It is not guaranteed that a save helper will ever be spawned on the read side (although this would be a very useless stream). The state of the libxl stream read/write object is deliberately separate from the save helper. > > Also, it is confusing and perhaps wrong that write_toolstack_record > calls stream_complete. What if there are other threads of control > outstanding ? This is exactly the problem which check_all_finished() is supposed to solve, but currently doesn't. > > >> 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. I have no idea why I wrote what I did. The code was correct but the description was wrong. I have fixed it in the split version of this patch. ~Andrew