`mmap` each ELF segment if `CONFIG_LIBPOSIXMMAP` is enabled #28

mogasergiu · 2023-09-22T16:38:53Z

Deprecate use of elf_load_do_fdread and uk_memalign in favor of
mmaping each ELF segment individually. This allows for massive
performance improvements in conjunction with on-demand paging.

The first address of the executable will be the first result of the
first time call to mmap on the first PT_LOAD and everything else
will be contiguously mapped by using the MAP_FIXED flag.

Depends on lib-libelf #3.

elf_load.c

The `e_rawfile` of `elf_prog` already contains the entire read or mmap'd ELF so use that instead for computing the interpretor path. Signed-off-by: Sergiu Moga <sergiu@unikraft.io>

i-Pear · 2023-10-02T07:31:46Z

A quick question: Looks like we didn't use p_align in the PHDRs, what will happen if p_align is larger than 4k? The manual says the segments should be aligned that way in memory.

[1] https://docs.oracle.com/cd/E19683-01/816-1386/chapter6-83432/index.html

elf_load.c

mogasergiu · 2023-10-02T09:39:07Z

A quick question: Looks like we didn't use p_align in the PHDRs, what will happen if p_align is larger than 4k? The manual says the segments should be aligned that way in memory.

I mmap according to the address that the linker already generated the PHDR with 🤔. Is it unsafe to assume that the linker already setup the address layout of the PHDRs according to the alignment the linker itself also used to describe the PHDR? 🤔

I just assume that the alignment is already taken care of implicitly by the address fields of the PHDR.

elf_load.c

i-Pear · 2023-10-02T12:59:10Z

Tested with a few applications, all working well, and looks good to me.

mogasergiu · 2023-10-02T20:08:14Z

I am going to try to modularize the code some more. elf_load_phdr ended up being massive (~170 lines).

mogasergiu · 2023-10-03T08:45:36Z

Ok, it should be easier to read the code now. Separated the logic between the first PT_LOAD and the other PT_LOAD's.

elf_load.c

mogasergiu · 2023-10-06T14:37:50Z

@i-Pear Thank you for the very thorough review! Here's another update. I hope I integrated all of the comments we reached consensus on. Let me know if you have any other comments!

i-Pear · 2023-10-06T15:07:38Z

The alignment of the first segment should be the maximum alignment of all PT_LOAD segments instead of the alignment of the first PT_LOAD segment? According to our discussion and your link [1].

[1] https://sourceware.org/bugzilla/show_bug.cgi?id=28676

i-Pear · 2023-10-12T12:01:17Z

True, this is also how the dynamic linker does it. But is it not safe to assume that only the first segment's alignment matters? Because otherwise, the other segments' alignment is ensured by the static linker when building the PHDR's and their start addresses right?

I don't think just considering the first alignment is safe enough, because you are performing a relocation. If the ELF is non-PIE, and you load it to the same address of the p_addr, that's safe enough. But if you relocate it, you have to ensure the diff of memory address is aligned to the maximum alignment?

Is it not safe to consider that if the alignment of the first PHDR is respected, then the first's PHDR alignment + the address offset from the PHDR with the biggest alignment to this first PHDR with a smaller alignment already ensures this alignment? E.g. (from NodeJS)
Program Headers:
  Type           Offset             VirtAddr           PhysAddr           FileSiz            MemSiz         Flags  Align
  LOAD           0x000000000064a000 0x000000000064a000 0x000000000064a000 0x0000000001229210 0x0000000001229210  R E    0x2000                                                                                                     
  LOAD           0x0000000001a00000 0x0000000001a00000 0x0000000001a00000 0x0000000000000231 0x0000000000000231  R E    0x200000
Does ensuring that this first segment is aligned is enough? Because then 0x0000000001a00000 would ensure for us that it will be aligned.

I still don't understand why it's safe. If the va_base is aligned to 0x200000, that will be safe.

But if we assume the va_base is 0x2000, in this case the first segment's p_align is respected. But the real vaddr of the second segment is 0x1a02000. The programmer may want the segment to use a HUGE_TLB mmap, but the unaligned va_base made it harder.

elf_load.c

mogasergiu · 2023-10-12T17:23:53Z

I decided to use the highest alignment after all. Since this is what glibc also does. You're right, let's keep it safe 💯.

i-Pear

Thanks, this will be the final round of review.

elf_load.c

mogasergiu · 2023-10-13T08:54:31Z

Thanks for the very attentive review @i-Pear. If all is fine with you now, feel free to approve and give you Reviewed-by tag 🙏

i-Pear

Looks good to me, thanks.

Reviewed-by: Tianyi Liu i.pear@outlook.com

mogasergiu · 2023-10-18T11:29:23Z

Turns out that TODO: we placed as a comment came back at me for some very small apps. Thank you for pointing it out @skuenzer ! Alas, I ended up doing the first mmap of the first ELF segment having the length of what would supposedly be the entire file in memory. This way we ensure we have a contiguous big enough mapping ready to go.

mogasergiu · 2023-10-18T11:30:15Z

@i-Pear This is the difference basically. Let me know if this change is still fine with you. You also mentioned it sometime ago IIRC.

i-Pear

Looks better on my side, everything looks well except the following comments:

elf_load.c

We must ensure that the image base address is aligned according to the alignment of the highest PT_LOAD alignment. Thus, do two things: - define `align` field in `struct elf_prog` that we will use to hold this highest alignment - make `elf_load_parse` also compute this, since it makes sense judging by the function name and, besides, it does an initial iteration over PT_LOADs anyway. Signed-off-by: Sergiu Moga <sergiu@unikraft.io>

mogasergiu · 2023-10-19T14:18:02Z

I also realised I could set elf_prog->valen to elf_prog->upperl. But I think it's fine to still keep prog_upperl/prog_lowerl as they might prove useful in the future.

skuenzer

Thanks a lot for the update! Looks good to me.

Approved-by: Simon Kuenzer simon@unikraft.io

Add an additional way of loading an ELF depending on whether CONFIG_LIBPOSIX_MMAP is used or not. If it is enabled, then the ELF loader will `mmap` the file instead of `pread` it. Otherwise, the previous method of `pread`ing file contents is used. The first address of the executable will be the first result of the first time call to `mmap` on the first `PT_LOAD` and everything else will be contiguously mapped by using the `MAP_FIXED` flag. Some special things to be noted when it comes to misaligned end addresses of ELF segments. Suppose we have `vastart` and `vaend`, corresponding to the start address of the ELF segment and the end address respectively and consider memsz as the runtime memory size occupied by the loaded segment: if vaend > vastart + memsz if vaend == PAGE_ALIGN_UP(vastart + memsz) the remaining bytes from (vastart + memsz) to vaend have already been mapped, so simply memset with 0 else /* vaend > PAGE_ALIGN_UP(vastart + memsz) */ this segment contains a section with NOBITS, such as .bss, which can be fairly large. Therefore: /* memset what is already mapped */ memset(vastart + memsz, 0, PAGE_ALIGN_UP(vastart + memsz) - vastart); /* anonymously map what is left, as memsetting large * sections such as .bss is very costly. */ mmap(PAGE_ALIGN_UP(vastart + memsz), vaend - PAGE_ALIGN_UP(vastart + memsz), ..., MAP_ANONYMOUS, ...); NOTE: The first segment must be mmap'd with the length of the entire file in memory and then the surplus is to be `munmapped`. This way we ensure we will always have a contiguous mapping big enough ready. Although future `mmap`'s with `MAP_FIXED` should normally overwrite this surplus, ridding us of the need to explicitly `munmap` it, in this current state of Unikraft (0.14.1) there is a bug in `ukvmem` that may not handle overlapping VMA's correctly. Signed-off-by: Sergiu Moga <sergiu@unikraft.io>

mogasergiu · 2023-10-19T15:25:08Z

Realised I forgot to add the elf_unload_vaimg equivalent for mmap

The `e_rawfile` of `elf_prog` already contains the entire read or mmap'd ELF so use that instead for computing the interpretor path. Signed-off-by: Sergiu Moga <sergiu@unikraft.io> Reviewed-by: Tianyi Liu <i.pear@outlook.com> Approved-by: Simon Kuenzer <simon@unikraft.io> GitHub-Closes: #28

We must ensure that the image base address is aligned according to the alignment of the highest PT_LOAD alignment. Thus, do two things: - define `align` field in `struct elf_prog` that we will use to hold this highest alignment - make `elf_load_parse` also compute this, since it makes sense judging by the function name and, besides, it does an initial iteration over PT_LOADs anyway. Signed-off-by: Sergiu Moga <sergiu@unikraft.io> Reviewed-by: Tianyi Liu <i.pear@outlook.com> Approved-by: Simon Kuenzer <simon@unikraft.io> GitHub-Closes: #28

Add an additional way of loading an ELF depending on whether CONFIG_LIBPOSIX_MMAP is used or not. If it is enabled, then the ELF loader will `mmap` the file instead of `pread` it. Otherwise, the previous method of `pread`ing file contents is used. The first address of the executable will be the first result of the first time call to `mmap` on the first `PT_LOAD` and everything else will be contiguously mapped by using the `MAP_FIXED` flag. Some special things to be noted when it comes to misaligned end addresses of ELF segments. Suppose we have `vastart` and `vaend`, corresponding to the start address of the ELF segment and the end address respectively and consider memsz as the runtime memory size occupied by the loaded segment: if vaend > vastart + memsz if vaend == PAGE_ALIGN_UP(vastart + memsz) the remaining bytes from (vastart + memsz) to vaend have already been mapped, so simply memset with 0 else /* vaend > PAGE_ALIGN_UP(vastart + memsz) */ this segment contains a section with NOBITS, such as .bss, which can be fairly large. Therefore: /* memset what is already mapped */ memset(vastart + memsz, 0, PAGE_ALIGN_UP(vastart + memsz) - vastart); /* anonymously map what is left, as memsetting large * sections such as .bss is very costly. */ mmap(PAGE_ALIGN_UP(vastart + memsz), vaend - PAGE_ALIGN_UP(vastart + memsz), ..., MAP_ANONYMOUS, ...); NOTE: The first segment must be mmap'd with the length of the entire file in memory and then the surplus is to be `munmapped`. This way we ensure we will always have a contiguous mapping big enough ready. Although future `mmap`'s with `MAP_FIXED` should normally overwrite this surplus, ridding us of the need to explicitly `munmap` it, in this current state of Unikraft (0.14.1) there is a bug in `ukvmem` that may not handle overlapping VMA's correctly. Signed-off-by: Sergiu Moga <sergiu@unikraft.io> Reviewed-by: Tianyi Liu <i.pear@outlook.com> Approved-by: Simon Kuenzer <simon@unikraft.io> GitHub-Closes: #28

mogasergiu mentioned this pull request Sep 22, 2023

Set ELFTC_HAVE_MMAP to 1 unikraft/lib-libelf#3

Merged

mogasergiu force-pushed the smoga/mmap branch from f9e69fd to 450411f Compare September 22, 2023 16:44

mogasergiu requested a review from skuenzer September 22, 2023 16:45

skuenzer self-assigned this Sep 30, 2023

skuenzer reviewed Sep 30, 2023

View reviewed changes

elf_load.c Show resolved Hide resolved

Read interpretor path from e_rawfile buffer instead of from file

362c070

The `e_rawfile` of `elf_prog` already contains the entire read or mmap'd ELF so use that instead for computing the interpretor path. Signed-off-by: Sergiu Moga <sergiu@unikraft.io>

mogasergiu force-pushed the smoga/mmap branch from 450411f to 336528a Compare September 30, 2023 11:00

razvand added the enhancement New feature or request label Oct 1, 2023

razvand requested review from skuenzer and i-Pear October 2, 2023 06:39

i-Pear reviewed Oct 2, 2023

View reviewed changes

elf_load.c Show resolved Hide resolved

i-Pear reviewed Oct 2, 2023

View reviewed changes

elf_load.c Outdated Show resolved Hide resolved

i-Pear reviewed Oct 2, 2023

View reviewed changes

elf_load.c Outdated Show resolved Hide resolved

mogasergiu force-pushed the smoga/mmap branch from 336528a to 281d992 Compare October 2, 2023 18:40

mogasergiu requested a review from i-Pear October 2, 2023 18:41

mogasergiu force-pushed the smoga/mmap branch 2 times, most recently from 9f4d14e to 82b5578 Compare October 2, 2023 19:02

mogasergiu force-pushed the smoga/mmap branch from 82b5578 to a0cc578 Compare October 3, 2023 08:44

mogasergiu force-pushed the smoga/mmap branch from a0cc578 to 2204f74 Compare October 3, 2023 08:57

i-Pear suggested changes Oct 5, 2023

View reviewed changes

i-Pear reviewed Oct 6, 2023

View reviewed changes

elf_load.c Show resolved Hide resolved

mogasergiu force-pushed the smoga/mmap branch from 2204f74 to fc178c4 Compare October 6, 2023 14:35

mogasergiu requested a review from i-Pear October 6, 2023 14:38

i-Pear reviewed Oct 12, 2023

View reviewed changes

elf_load.c Show resolved Hide resolved

mogasergiu force-pushed the smoga/mmap branch from fc178c4 to 87471cf Compare October 12, 2023 17:22

mogasergiu requested a review from i-Pear October 12, 2023 17:23

i-Pear suggested changes Oct 13, 2023

View reviewed changes

elf_load.c Outdated Show resolved Hide resolved

elf_load.c Outdated Show resolved Hide resolved

mogasergiu force-pushed the smoga/mmap branch 3 times, most recently from 3673311 to 78b2a70 Compare October 13, 2023 08:43

mogasergiu requested a review from i-Pear October 13, 2023 08:53

i-Pear approved these changes Oct 13, 2023

View reviewed changes

mogasergiu force-pushed the smoga/mmap branch from 78b2a70 to 46bc6c9 Compare October 18, 2023 11:26

i-Pear reviewed Oct 18, 2023

View reviewed changes

elf_load.c Outdated Show resolved Hide resolved

elf_load.c Show resolved Hide resolved

mogasergiu force-pushed the smoga/mmap branch from 46bc6c9 to d5ab1d2 Compare October 18, 2023 18:19

skuenzer reviewed Oct 19, 2023

View reviewed changes

elf_load.c Outdated Show resolved Hide resolved

elf_load.c Outdated Show resolved Hide resolved

mogasergiu force-pushed the smoga/mmap branch 2 times, most recently from 135d9f2 to 04b559a Compare October 19, 2023 14:16

skuenzer approved these changes Oct 19, 2023

View reviewed changes

mogasergiu force-pushed the smoga/mmap branch from 04b559a to 92e0c08 Compare October 19, 2023 15:24

razvand merged commit fd7f282 into unikraft:staging Oct 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`mmap` each ELF segment if `CONFIG_LIBPOSIXMMAP` is enabled #28

`mmap` each ELF segment if `CONFIG_LIBPOSIXMMAP` is enabled #28

mogasergiu commented Sep 22, 2023 •

edited

Loading

i-Pear commented Oct 2, 2023

mogasergiu commented Oct 2, 2023

i-Pear commented Oct 2, 2023

mogasergiu commented Oct 2, 2023

mogasergiu commented Oct 3, 2023

mogasergiu commented Oct 6, 2023

i-Pear commented Oct 6, 2023

i-Pear commented Oct 12, 2023 •

edited

Loading

mogasergiu commented Oct 12, 2023

i-Pear left a comment

mogasergiu commented Oct 13, 2023

i-Pear left a comment

mogasergiu commented Oct 18, 2023

mogasergiu commented Oct 18, 2023

i-Pear left a comment

mogasergiu commented Oct 19, 2023

skuenzer left a comment

mogasergiu commented Oct 19, 2023

mmap each ELF segment if CONFIG_LIBPOSIXMMAP is enabled #28

mmap each ELF segment if CONFIG_LIBPOSIXMMAP is enabled #28

Conversation

mogasergiu commented Sep 22, 2023 • edited Loading

i-Pear commented Oct 2, 2023

mogasergiu commented Oct 2, 2023

i-Pear commented Oct 2, 2023

mogasergiu commented Oct 2, 2023

mogasergiu commented Oct 3, 2023

mogasergiu commented Oct 6, 2023

i-Pear commented Oct 6, 2023

i-Pear commented Oct 12, 2023 • edited Loading

mogasergiu commented Oct 12, 2023

i-Pear left a comment

Choose a reason for hiding this comment

mogasergiu commented Oct 13, 2023

i-Pear left a comment

Choose a reason for hiding this comment

mogasergiu commented Oct 18, 2023

mogasergiu commented Oct 18, 2023

i-Pear left a comment

Choose a reason for hiding this comment

mogasergiu commented Oct 19, 2023

skuenzer left a comment

Choose a reason for hiding this comment

mogasergiu commented Oct 19, 2023

`mmap` each ELF segment if `CONFIG_LIBPOSIXMMAP` is enabled #28

`mmap` each ELF segment if `CONFIG_LIBPOSIXMMAP` is enabled #28

mogasergiu commented Sep 22, 2023 •

edited

Loading

i-Pear commented Oct 12, 2023 •

edited

Loading