From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ferry Huberts Subject: Re: [PATCH] memory: transaction API Date: Thu, 21 Jul 2011 14:26:03 +0200 Message-ID: <4E281ADB.90708@hupie.com> References: <1311243679-18403-1-git-send-email-avi@redhat.com> <4E2807A1.6000001@hupie.com> <4E281684.5000203@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: Jan Kiszka , qemu-devel@nongnu.org, kvm@vger.kernel.org To: Avi Kivity Return-path: Received: from 82-197-206-98.dsl.cambrium.nl ([82.197.206.98]:61319 "EHLO mail.hupie.dyndns.org" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752132Ab1GUM0F (ORCPT ); Thu, 21 Jul 2011 08:26:05 -0400 In-Reply-To: <4E281684.5000203@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On 07/21/2011 02:07 PM, Avi Kivity wrote: > On 07/21/2011 02:04 PM, Ferry Huberts wrote: >> On 07/21/2011 12:21 PM, Avi Kivity wrote: >> > Allow changes to the memory hierarchy to be accumulated and >> > made visible all at once. This reduces computational effort, >> > especially when an accelerator (e.g. kvm) is involved. >> > >> > Useful when a single register update causes multiple changes >> > to an address space. >> > >> > >> > +void memory_region_transaction_begin(void) >> > +{ >> > + ++memory_region_transaction_depth; >> > +} >> > + >> >> wouldn't you rather keep it safe by doing either here >> >> if (!memory_region_transaction_depth) >> memory_region_transaction_depth++; >> > > Why? I want to allow nesting transactions (not that I anticipate such a > case). > >> > +void memory_region_transaction_commit(void) >> > +{ >> > + if (!memory_region_transaction_depth) { >> > + abort(); >> > + } >> >> >> > + --memory_region_transaction_depth; >> > + memory_region_update_topology(); >> > +} >> > + >> >> or by doing here >> >> while (!memory_region_transaction_depth) >> memory_region_transaction_depth--; doesn't memory_region_update_topology commit all accumulated changes? if it does then memory_region_transaction_depth is left non-zero in the nesting case while no more changes are actually present, resulting in superfluous calls to memory_region_update_topology. maybe I misunderstood memory_region_update_topology? >> >> >> with your setup nesting transactions will not work correctly I think. >> You seem to have designed it to not do nesting, so it's safer to make >> that explicit in your code imho > > Nesting should work just fine. > >> therefore I'd go for the change in _begin >> >> also, wouldn't you rather rename memory_region_transaction_depth to >> memory_region_transaction_pending? > > The existing name works for me, but if people want it changed, that's > fine too. > > -- Ferry Huberts