Date: Fri, 30 Aug 2002 17:30:52 +1000 (EST) From: Bruce Evans <bde@zeta.org.au> To: Peter Wemm <peter@wemm.org> Cc: cvs-committers@FreeBSD.org, <cvs-all@FreeBSD.org> Subject: Re: cvs commit: src/sys/sys param.h Message-ID: <20020830165607.F2668-100000@gamplex.bde.org> In-Reply-To: <20020830004539.C123C2A7D6@canning.wemm.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, 29 Aug 2002, Peter Wemm wrote: > Peter Wemm wrote: > > Modified files: > > sys/sys param.h > > Log: > > AARGH! btoc() is used in the MI buffer sizing routines to calculate > > the minimum of either physmem or KVA. But.. btoc() casts the address > > to (unsigned int). This is NOT GOOD on 64 bit machines and on alpha and > > ia64, this results in a buffer limit of around 500K (not megs). This > > causes extreme disk access problems on alpha and ia64. Since this cast > > is simply to ensure that it is unsigned, use 'vm_offset_t' instead. This > > is available because it is already defined in types.h. > > > > Alpha has been suffering from this for ages. It always felt like the > > caching wasn't working, and unfortunately it turned out that way. :-( > > Pointy hat to: phk > > sys/param.h > revision 1.127 > date: 2002/05/14 20:35:29; author: phk; state: Exp; lines: +68 -0 > Move MI stuff out of MD param.h files. > +#define btoc(x) (((unsigned)(x)+PAGE_MASK)>>PAGE_SHIFT) > > > alpha/include/param.h: > revision 1.28 > date: 2002/05/14 20:35:25; author: phk; state: Exp; lines: +0 -38 > Move MI stuff out of MD param.h files. > -#define btoc(x) (((x) + PAGE_MASK) >> PAGE_SHIFT) > > ie: he used the x86 version on all platforms without accounting for the > differences. ctob() is still broken, but was already broken on alphas. The old alpha version of btoc() has some advantages over your fixed version: - it is missing a dubious cast. vm_offset_t is not quite the right thing to cast to. vm_size_t would be less incorrect -- the fact that the broken ctob() didn't cause problems immediately shows that it is mostly used on sizes and not on addresses. ctob() should probably only be used on cluster counts anyway (and it should be spelled ptob() since clusters went away long ago). There are confusingly differently named macros for converting pages to addresses and pages to bytes. (x) needs to be positive so that the right shift works right but that it the caller's responsibilty -- there would only be a problem with very broken callers that handle very large byte counts by storing them in int or long variables as negative values. - it is missing some style bugs (it is not missing spaces around binary operators). The old alpha version of ctob() is missing the same style bugs as the old alpha version of btoc(). Unfortunately, it is also missing a cast, so it overflows at much the same places as ctob() if the type of (x) is int or u_int. I guess the overflow doesn't happen often in practice since the type is usually vm_offset_t or vm_size_t, but a type of [u_]int32 would be quite reasonable for storing page counts since it would work up to 8K * 4GB of physycal memory. The following untested patches restore the old alpha versions and add a cast to ctob(). I used a cast to vm_size_t for the reasons discussed above. This is not quite right, since vm_size_t might be for virtual sizes (it is not documented, so who knows what it is for :-) and physical sizes might be larger. %%% Index: param.h =================================================================== RCS file: /home/ncvs/src/sys/sys/param.h,v retrieving revision 1.133 diff -u -2 -r1.133 param.h --- param.h 30 Aug 2002 00:29:52 -0000 1.133 +++ param.h 30 Aug 2002 06:57:42 -0000 @@ -148,10 +144,10 @@ /* clicks to bytes */ #ifndef ctob -#define ctob(x) ((x)<<PAGE_SHIFT) +#define ctob(x) ((vm_size_t)(x) << PAGE_SHIFT) #endif /* bytes to clicks */ #ifndef btoc -#define btoc(x) (((vm_offset_t)(x)+PAGE_MASK)>>PAGE_SHIFT) +#define btoc(x) (((x) + PAGE_MASK) >> PAGE_SHIFT) #endif %%% Bruce To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe cvs-all" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20020830165607.F2668-100000>