Re: More hpcboot memory/ELF loading errors?

["Valeriy E. Ushakov" <uwe%stderr.spb.ru@localhost>]

On Thu, Mar 06, 2008 at 08:52:02 -0500, Rafal Boni wrote:

> >>--- hpcboot/load_elf.cpp    5 Mar 2006 04:04:13 -0000       1.17
> >>+++ hpcboot/load_elf.cpp    6 Mar 2008 02:01:36 -0000
> >>@@ -123,10 +123,6 @@ ElfLoader::memorySize()
> >>                    sz += _mem->roundPage(filesz);
> >>                    // compensate for partial last tag
> >>                    extra += _mem->getTaggedPageSize();
> >>-                   if (filesz < ph->p_memsz)
> >>-                           // compensate for zero clear
> >>-                           extra += _mem->getTaggedPageSize();
> >>-
> >>            }
> >>    }
> >
> >Why have you removed this?  Zero-clear chunk occupies an extra tagged
> >page in the chain.
> 
> I thought this was the case too,  but if you follow the code / chain, 
> you'll note that they steal from the initial 'pvec' page allocated at 
> the beginning (they just kidnap sizeof(PageTag) since there's no data to 
> store).  So I adjusted for reality.

Well, my memory is hazy and the way page allocation is managed is a
bit contorted, but my reading of _load_segment is that we burn a whole
tagged page for zero-clear.  And that extra you've deleted was added
there exactly to force an extra tagged page to be allocated for that
zero-clear.  Cf. boot_func at the end of sh3/sh_arch.h.




> >>@@ -135,8 +131,9 @@ ElfLoader::memorySize()
> >>    if (symblk_sz) {
> >>            sz += symblk_sz;
> >>            DPRINTF((TEXT(" = 0x%x]"), symblk_sz));
> >>-           // XXX: compensate for partial tags after ELF header and 
> >>symtab
> >>-           extra += 2 * _mem->getTaggedPageSize();
> >>+           // XXX: compensate for partial tags after ELF header, symtab
> >>+           // and strtab
> >>+           extra += 3 * _mem->getTaggedPageSize();
> >>    }
> >
> >Does it work for you with this hunk alone?
> 
> I haven't tried just that one -- though even if it works, it seems a 
> little like a rubber-chickens and voodoo-sticks change ;).  But I'll 
> give it a try and report back in any case..
[...]
> >>-   // return total amount of symbol block
> >>-   return (_sym_blk.header_size + ROUND4(_sym_blk.shsym->sh_size) +
> >>-       _sym_blk.shstr->sh_size);
> >>+   // Round each of the three components to page_size since they're
> >>+   // loaded as 3 different segments.
> >>+   return (_mem->roundPage(_sym_blk.header_size) +
> >>+           _mem->roundPage(ROUND4(_sym_blk.shsym->sh_size)) +
> >>+           _mem->roundPage(_sym_blk.shstr->sh_size));
> >>+       
> >> }
> >
> >Is these roundPage necessary?  I really don't want to inflict on
> >myself the pain of reading and understanding this code again, but IIRC
> >extra pages added in the caller are supposed to take care of this.
> >Moreover, IIRC, doing roundPage doesn't actually help the "partial
> >tag" problem.
> 
> I think this is the crux of the problem, and yeah, I think it's 
> necessary.  If you look at what ElfLoader::load_symbol_block() does, it 
> loads (a) header, (b) symbol table, and (c) string table as individual 
> segments, which means each will start on a new page in the tag chain.
> 
> So if you have e.g. 1320 + 1290 + 1350 bytes in those 3, before we'd 
> return 1320 + ROUND4(1290) = 1292 + 1350 = 3962, which would look like 
> it would fit in a single page (even in a single tagged page), but would 
> actually require *3* tagged pages since each component would get it's 
> own link (== tagged page) in the tag chain.

But then the caller throws in two extra pages into the calculation to
accomdate the "gaps" induced by each of the above being allocated from
a fresh tagged page, so it's:

    1320 + tsz + 1292 + tsz + 1350

and that rounds to 3 pages (see estimateTaggedPageSize) as expected.
Even if each of the above is just 1 byte, the extra will still pad it
so that at least three tagged pages are allocated.

The rule of thumb is - the caller adds a page to the "extra" for every
potential partial tagged page:

        . last partial page of each segment
        . zero-clear page for each segment if necessary
        . elf header
        . symbol table

As string table is last, there's no gap and so no extra is added for
it.

I wonder if we don't account for some otehr partial tag elsewhere.


> As for the 'partial tag' issue, I'm also not sure off the top of my 
> head, but given the gyrations Architecture::allocateMemory goes through 
> to convert bytes-needed-in-memory to tagged-pages-needed, I'd think as 
> long as we rounded up all segments to the next unit of tagged-page-size 
> (since after all that's what matters, not actual HW page size), there 
> should be *no* page unaccounted for.  Rounding up to HW page size is 
> maybe a little sloppy, but should give more *conservative* estimates, 
> which from our POV is OK (after all, it's going to get rounded up to the 
> next chunk of 64k anyway when we allocate pages from WinCE).

Well the "extra" provides just that by conservatively forcing an extra
tagged page to be allocated for every place we might have a partial
tagged page caused by next segment &co starting from the new tagged
page.  This should have the same effect as rounding, except for some
cases where rounding will force two extra pages to be allocated
(e.g. last page is full except for the last 16 bytes (sizeof(PageTag))
or less - with rounding you will allocate two extra tagged pages to
"hold" the extra space instead of one).


> >I'd prefer we really understand were the overflow occurs before we
> >commit anything to this code that is already quite hairy.
> 
> I'm all for that, which is why I'm sending this to the list rather than 
> checking in... but I also want to minimize the amount of damage my 
> forehead takes from beating it against this too much, and in the end I 
> want something which works at least semi-reliably -- and this hasn't 
> been the case with the hpcboot built from unmodified sources.

Why don't you enable the debugging printfs (there an option to dump
page chains) and find where does the overflow occur?


SY, Uwe
-- 
uwe%stderr.spb.ru@localhost                       |       Zu Grunde kommen
http://snark.ptc.spbu.ru/~uwe/          |       Ist zu Grunde gehen