Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide option to CMake for Page Size #664

Merged
merged 4 commits into from
Sep 25, 2024
Merged

Conversation

mjp41
Copy link
Member

@mjp41 mjp41 commented Jun 17, 2024

This uses the PAGESIZE constant from the unistd.h on POSIX. This should make the code more resilient to being compiled on platforms with different page sizes.

@mjp41
Copy link
Member Author

mjp41 commented Jun 17, 2024

Hopefully addresses #663

@mjp41
Copy link
Member Author

mjp41 commented Jun 17, 2024

@SchrodingerZhu would you be able to see if this fixes your problem.

@SchrodingerZhu
Copy link
Contributor

will try this after work

@SchrodingerZhu
Copy link
Contributor

SchrodingerZhu commented Jun 18, 2024

Sorry for the noise. git seems to have strange behaviour when checking out pr/head so my local change was not overwritten, which makes me think the issue was fixed.

The fix is not working. Such macro is undefined on platforms without a fixed page size.

#ifdef PAGESIZE
    #warning "defined"
    static constexpr size_t page_size = max(Aal::smallest_page_size, PAGESIZE);
#else
    #warning "undefined"
    static constexpr size_t page_size = Aal::smallest_page_size;
#endif

Gives undefined.

Actually, the corresponding header says

/* We do not provide fixed values for

   ARG_MAX      Maximum length of argument to the `exec' function
                including environment data.

   ATEXIT_MAX   Maximum number of functions that may be registered
                with `atexit'.

   CHILD_MAX    Maximum number of simultaneous processes per real
                user ID.

   OPEN_MAX     Maximum number of files that one process can have open
                at anyone time.

   PAGESIZE
   PAGE_SIZE    Size of bytes of a page.

   PASS_MAX     Maximum number of significant bytes in a password.

   We only provide a fixed limit for

   IOV_MAX      Maximum number of `iovec' structures that one process has
                available for use with `readv' or writev'.

   if this is indeed fixed by the underlying system.
*/

If this is not the on the perf-critical path, I suggest use _SC_PAGESIZE approach instead. It is the most portable way. In libc on linux, this value is usually populated via aux vector and cached by libc. There is no syscall involved. If you are concern that sysconf has a super large jumptable, you can also cache it inside snmalloc.

@SchrodingerZhu
Copy link
Contributor

SchrodingerZhu commented Jun 22, 2024

Inspected the code a little, it seems that a dynamic page size demands too many changes and may impact negatively on the performance. Is it possible to allow cmake to run sysconf or let user override page size during configuration phase? All these can be set as additional options.

@mjp41
Copy link
Member Author

mjp41 commented Jun 22, 2024

Inspected the code a little, it seems that a dynamic page size demands too many changes and may impact negatively on the performance. Is it possible to allow cmake to run sysconf or let user override page size during configuration phase? All these can be set as additional options.

I would prefer the second. The first might have issues with cross compiling, so I wouldn't want to always run sysconf at configure time.

I think we could make the dynamic page size work, but it is more effort than I have time for, and would be pretty subtle to review the changes.

This uses the PAGESIZE constant from the unistd.h on POSIX.
This should make the code more resilient to being compiled on platforms with
different page sizes.
Co-authored-by: Nathaniel Filardo <105816689+nwf-msr@users.noreply.github.com>
@mjp41
Copy link
Member Author

mjp41 commented Jul 1, 2024

@SchrodingerZhu can this be used for your use case?

@SchrodingerZhu
Copy link
Contributor

Yes. I can workaround the issue with this patch

@mjp41 mjp41 changed the title Pickup page size from unistd.h Provide option to CMake for Page Size Sep 24, 2024
@mjp41 mjp41 enabled auto-merge (squash) September 24, 2024 10:52
@mjp41 mjp41 disabled auto-merge September 25, 2024 10:27
@mjp41 mjp41 merged commit ab4fe84 into microsoft:main Sep 25, 2024
52 checks passed
@mjp41 mjp41 deleted the page_size branch September 25, 2024 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants