page_alloc query

page_alloc query

[Ben Thomas]

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

Re: page_alloc query

[Keir Fraser]

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

Re: page_alloc query

[Jan Beulich]

>>> 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

Re: page_alloc query

[Jan Beulich]

>>> "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

Re: page_alloc query

[Keir Fraser]

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; )
> 
> Jan

Hmmm... Yeah, okay.

 -- Keir