Hi,
The following code is causing some questions. Perhaps someone could
help explain it... (xen/common/page_alloc.c)
Why are the zones unsigned ints when they are used as indices ?
The ASSERTS do checks to keep you out of some amount of
trouble, but the loop appears to be able to get you quickly
into trouble by driving the index negative and with a check
that doesn''t appear to be effective.
static struct page_info *alloc_heap_pages(
unsigned int zone_lo, unsigned zone_hi,
unsigned int cpu, unsigned int order)
{
.
.
.
ASSERT(zone_lo <= zone_hi);
ASSERT(zone_hi < NR_ZONES);
.
.
.
for ( zone = zone_hi; zone >= zone_lo; --zone )
{
/* check if target node can support the allocation */
if ( avail[node] && (avail[node][zone] >= request) )
Has it just been a long day and I''m not reading this correctly ?
The older version of this code didn''t appear to operate in this
fashion. Is a simple change of unsigned -> signed sufficient here ?
(While it has been a long day, and it could be failing neurons,
this code has just crashed recently due to a bad index, so....)
I have to run, and can''t fix this right now, so I''ll settle
for this
query.
Thanks,
-b
--
------------------------------------------------------------------------
Ben Thomas Virtual Iron Software
bthomas@virtualiron.com Tower 1, Floor 2
978-849-1214 900 Chelmsford Street
Lowell, MA 01851
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
On 6/3/07 22:10, "Ben Thomas" <bthomas@virtualiron.com> wrote:> Why are the zones unsigned ints when they are used as indices ? > The ASSERTS do checks to keep you out of some amount of > trouble, but the loop appears to be able to get you quickly > into trouble by driving the index negative and with a check > that doesn''t appear to be effective.This is already fixed in the staging tree. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>>> Keir Fraser <Keir.Fraser@cl.cam.ac.uk> 07.03.07 09:27 >>> >On 6/3/07 22:10, "Ben Thomas" <bthomas@virtualiron.com> wrote: > >> Why are the zones unsigned ints when they are used as indices ? >> The ASSERTS do checks to keep you out of some amount of >> trouble, but the loop appears to be able to get you quickly >> into trouble by driving the index negative and with a check >> that doesn''t appear to be effective. > >This is already fixed in the staging tree.Hmm, looking at that fix I doubt it helps - since zone_lo and zone_hi remain unsigned, my understanding would be that the signed zone is converted to unsigned before the comparison, hence nothing changes. And I really think using signed variables for array indices is odd - it performs worse on x86-64/ia64 at least (because of the extra sign extension, whereas the zero extension is implied in most/some operations), and it certainly is contrary to how arrays work (they don''t normally have fields accessible with negative indices). Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>>> "Jan Beulich" <jbeulich@novell.com> 07.03.07 10:09 >>> >>>> Keir Fraser <Keir.Fraser@cl.cam.ac.uk> 07.03.07 09:27 >>> >>On 6/3/07 22:10, "Ben Thomas" <bthomas@virtualiron.com> wrote: >> >>> Why are the zones unsigned ints when they are used as indices ? >>> The ASSERTS do checks to keep you out of some amount of >>> trouble, but the loop appears to be able to get you quickly >>> into trouble by driving the index negative and with a check >>> that doesn''t appear to be effective. >> >>This is already fixed in the staging tree. > >Hmm, looking at that fix I doubt it helps - since zone_lo and zone_hi remain >unsigned, my understanding would be that the signed zone is converted to >unsigned before the comparison, hence nothing changes. > >And I really think using signed variables for array indices is odd - it performs >worse on x86-64/ia64 at least (because of the extra sign extension, whereas >the zero extension is implied in most/some operations), and it certainly is >contrary to how arrays work (they don''t normally have fields accessible with >negative indices).e.g. by instead doing for ( zone = zone_hi + 1; zone-- > zone_lo; ) Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 7/3/07 09:18, "Jan Beulich" <jbeulich@novell.com> wrote:>> And I really think using signed variables for array indices is odd - it >> performs >> worse on x86-64/ia64 at least (because of the extra sign extension, whereas >> the zero extension is implied in most/some operations), and it certainly is >> contrary to how arrays work (they don''t normally have fields accessible with >> negative indices). > > e.g. by instead doing > > for ( zone = zone_hi + 1; zone-- > zone_lo; ) > > JanHmmm... Yeah, okay. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel