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

Unify the definition of page size #49

Closed
lundgren87 opened this issue Feb 18, 2021 · 3 comments
Closed

Unify the definition of page size #49

lundgren87 opened this issue Feb 18, 2021 · 3 comments

Comments

@lundgren87
Copy link
Member

Related to the discussion in:
#45 (comment)

ArgoDSM page size should be defined in one module instead of appearing as separate static const definitions in multiple modules, and retrieved from there where needed throughout the system.

@lundgren87
Copy link
Member Author

lundgren87 commented Apr 22, 2021

Should this issue also deal with the hardcoded CACHELINE value, which I am sure changing to anything besides 1L has not worked very well for a long time (and definitely does not work with allocation policies)?

I would suggest removing it entirely, or possibly baking it in to the future unified definition of page size. It should now serve very little purpose with the introduction of allocation policies, prefetching and a new write buffer.

@davidklaftenegger Any opinion on this?

kostis added a commit that referenced this issue Jun 23, 2022
This constant is used for the size of a page, but was hard-coded in
various places using various forms (4096, 4096UL, ...).  Also, various
header files used different definitions / names for it (pagesize,
page_size, PAGESIZE).

This commit adds a `PAGE_SIZE` constexpr and uses it in all `src` files.

Addresses the biggest part of #49.  (Does not deal with CACHELINE.)
@kostis kostis mentioned this issue Jun 23, 2022
kostis added a commit that referenced this issue Jun 23, 2022
This constant is used for the size of a page, but was hard-coded in
various places using various forms (4096, 4096UL, ...).  Also, various
header files used different definitions / names for it (pagesize,
page_size, PAGESIZE).

This commit adds a `PAGE_SIZE` constexpr and uses it in all `src` files.

Addresses the biggest part of #49.  (Does not deal with CACHELINE.)
@lundgren87
Copy link
Member Author

lundgren87 commented Jun 23, 2022

I have opened a new issue regarding CACHELINE so that #125 can resolve this issue.

kostis added a commit that referenced this issue Nov 21, 2022
This constant was used for the size of a page, but was hard-coded in
various places using various forms (4096, 4096UL, ...).  Also, various
header files used different definitions / names for it (`pagesize`,
`page_size`, `PAGESIZE`).

To avoid polluting the namespace and also avoid all discussions on which
header file is best to include the definition of the `PAGE_SIZE` constant,
we instead pass it as a command-line definition.

We also hard-code its value because we want to ensure it is an unsigned,
rather than integer that the commented out code to get it from the
environment or a simple 4096 would default to.

Addresses the biggest part of #49.  (Does not deal with CACHELINE.)
@kostis
Copy link
Contributor

kostis commented Nov 21, 2022

Closing this issue due to merging #125.
The CACHELINE issues will be discussed and/or resolved in #128.

@kostis kostis closed this as completed Nov 21, 2022
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

No branches or pull requests

2 participants