No 4096

[kostis]

Eliminate all uses of the magic 4096 constant
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]

Good changes. My only question is if this should be defined in backend/backend.hpp as this PR suggests, or somewhere at a higher level, as it is used in multiple modules and not only in the backend?

[davidklaftenegger]

Is there a good reason to not stick the constant into a namespace? Not polluting the default namespace sounds like a good idea for such a generic name.

Also, as we are trying to improve coding style, this might be a good opportunity to change our coding style document and follow the more modern advice [1] to use lower case for constants, to avoid clashes with any preprocessor macros. This can be done in a mass commit later, of course, but would then touch all these lines again.

[1] https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es9-avoid-all_caps-names

[kostis]

Is there a good reason to not stick the constant into a namespace?

No, there is not. Actually, this is something I wanted to mention in the commit message but forgot. Of course it should be.

Since there are two (very) related issues here, can we first decide on which header file this should be? Any suggestion?

Secondly, I think we should clean up CACHELINE as part of this PR (so that we close #49).

Finally, regarding:

... this might be a good opportunity to change our coding style document ...

let me mention that this was probably the first time that I did follow the code style document, but now we do not like that particular part of it anymore ;-)

[davidklaftenegger]

Sorry for the messy comment, I tried to factor out the sidetracking to the bottom, but either I send this now, or I delete it entirely.

I thought about src/backend/backend.hpp, src/argo.hpp, and src/system/hardware_constants.hpp (name not important).
There is currently very little content [1] for a new directory to group hardware and operating system specific code, and I don't see strong arguments for either of the other two. I agree that namespace should follow from the file path.

Subjectively I think it feels better outside the backend namespace, as it is a hardware detail and not a backend detail, but I don't think this is a strong argument.

CACHELINE is a different situation: it has a terrible name [2], and belongs to the cache implementation [3], and I think it should stay with that code. Alternatively, as suggested in #49, it could be removed entirely [4]. (I also think the cache implementation should move out of the backend, but that is certainly beyond the scope of this PR.)

[1] we have the memfd-syscall, which is currently handled in a linux-specific way (in src/virtual_memory/memfd.cpp), and I think there are some interesting use cases for other operating system features (e.g. hugepages) and hardware constants (e.g. cacheline size of the processor), but currently we don't use or need these.
[2] it's how many pages are in the software cache, not how many bytes are in the hardware cache
[3] currently split across src/backend/mpi/swdsm.* and src/backend/mpi/write_buffer.hpp
[4] we would lose the untested feature of treating multiple pages as a single unit. Maybe @lundgren87 can elaborate (here or in #49) how this effect could be re-achieved without the CACHELINE constant.

[lundgren87]

I opened #128 on CACHELINE as I feel it is better discussed separately. I think that covers what David mentions in [2-4].

[lundgren87]

I thought about src/backend/backend.hpp, src/argo.hpp, and src/system/hardware_constants.hpp (name not important). There is currently very little content [1] for a new directory to group hardware and operating system specific code, and I don't see strong arguments for either of the other two. I agree that namespace should follow from the file path.

Subjectively I think it feels better outside the backend namespace, as it is a hardware detail and not a backend detail, but I don't think this is a strong argument.

[1] we have the memfd-syscall, which is currently handled in a linux-specific way (in src/virtual_memory/memfd.cpp), and I think there are some interesting use cases for other operating system features (e.g. hugepages) and hardware constants (e.g. cacheline size of the processor), but currently we don't use or need these.

Given that PAGE_SIZE is in this PR used in backend/, virtual_memory/ and mempools/ already I would intuitively place this in argo.hpp if there isn't sufficient motivation for a new directory.

[kostis]

Given that PAGE_SIZE is in this PR used in backend/, virtual_memory/ and mempools/ already I would intuitively place this in argo.hpp if there isn't sufficient motivation for a new directory.

Unless we see any reason for PAGE_SIZE to be useful tor user programs, my view is that we should not put it in argo.hpp. Also, although it is true that PAGE_SIZE is in this PR used in backend/, virtual_memory/ and mempools/, out of the 63 occurrences of PAGE_SIZE, 59 of them are in backend/, three of them in mempools/ and only one in virtual_memory/.

I suggest we leave it in backend/, as is currently. If we agree, I will just move it inside its namespace, although as @davidklaftenegger also points out subjectively it does not feel part of it.

EDITED (TWICE): On a second thought, perhaps it's better to leave it outside the backend's namespace. Also, note that <unistd.h> defines _SC_PAGE_SIZE from which the page size can be calculated. I can change the code to look as follows:

constexpr std::size_t PAGE_SIZE = sysconf(_SC_PAGE_SIZE);  // this does not work because it's not statically evaluated

or we can even completely remove this definition and use some configure-time-evaluated expression from _SC_PAGE_SIZE in all places. Thoughts?

[lundgren87]

Ok, since no one has come up with a better solution it can be left where it is.

[kostis]

After some thought and some discussion, we decided that it is best to not define a constant PAGE_SIZE in any file but instead pass it as a command-line define in cmake.

[lundgren87]

I think the latest proposed solution is OK. It also circumvents the issue of which file and namespace this should be declared in. I will approve it but perhaps give @davidklaftenegger a chance to respond before merging.