From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH 1 of 5] xentrace: fix t_info_pages calculation for the default case Date: Wed, 23 Mar 2011 14:56:36 +0000 Message-ID: <1300892196.2827.19.camel@silas> References: <20110323112040.GB28535@aepfle.de> <20110323124654.GA4696@aepfle.de> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20110323124654.GA4696@aepfle.de> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Olaf Hering Cc: "xen-devel@lists.xensource.com" , Keir Fraser , Jan Beulich List-Id: xen-devel@lists.xenproject.org Actually, I think the source of this bug was the re-definition of what the variable meant in the middle. I think the Right Thing, to avoid bugs like this in the future, would be to assign separate variables, thus: t_info_bytes= [calculation] t_info_pages = t_info_bytes / PAGE_SIZE if(t_info_bytes % PAGE_SIZE) t_info_pages++; The compiler should optimize away unused stuff, and even if not, a byte here is small cost to make the logic more readable. -George On Wed, 2011-03-23 at 12:46 +0000, Olaf Hering wrote: > On Wed, Mar 23, Keir Fraser wrote: > > > On 23/03/2011 11:20, "Olaf Hering" wrote: > > > > >>>> t_info_pages /= PAGE_SIZE; > > >>>> - if ( t_info_pages % PAGE_SIZE ) > > >>>> + if ( t_info_pages % PAGE_SIZE || t_info_pages == 0 ) > > >>> > > >>> While certainly not having a significant effect, to the unsuspecting > > >>> reader this looks like a bug - is it really meant to be a remainder > > >>> operation on the *result* of a division (rather than on the original > > >>> dividend)? Couldn't you just (ab)use PFN_UP() here? > > >> > > >> By which you mean to replace the division and subsequent if statement with > > >> t_info_pages = PFN_UP(t_info_pages). > > > > > > I did not know about PFN_UP() until now, using it would work as well. > > > > As opposed to the existing code (even including your latest patch) which > > doesn't work properly. You need to respin at least your patch 1/5. > > I will send the bugfix now. > > Olaf