Stop defining `PAGE_SIZE`, and use `getpagesize()` on Unix

[dim]

Summary

The kmp_os.h header is defining the PAGE_SIZE macro unconditionally,
even while it is only used directly after its definition, for the
Windows implementation of the KMP_GET_PAGE_SIZE() macro.

On at least FreeBSD, but likely all other BSDs too, this macro conflicts
with the one defined in system headers, so remove it, since nothing else
uses it. Make all Unixes use getpagesize() instead, and return 0x4000
directly for the Windows case.

(Note that Windows also has a way to retrieve page size, via the
SYSTEM_INFO structure:
https://msdn.microsoft.com/en-us/library/windows/desktop/ms724958(v=vs.85).aspx
)

[zturner]

There's a comment indicating that we should find the correct function on Windows and use it, but in the description of this review you mention that you found it in the SYSTEM_INFO structure. So why not use it?

static inline size_t getpagesize() {
  SYSTEM_INFO S;
  GetNativeSystemInfo(&S);
  return S.dwPageSize;
}
#define KMP_GET_PAGE_SIZE() getpagesize()

[dim]

In D35072#800989, @zturner wrote:

There's a comment indicating that we should find the correct function on Windows and use it, but in the description of this review you mention that you found it in the SYSTEM_INFO structure. So why not use it?

I'm fine with that, but I have no way to test building or functionality on Windows...

[dim]

Note the TODO case also covers KMP_OS_CNK, of which I have no idea what it is. So the 0x4000 case must stay in there anyway.

[dim]

Apparently KMP_OS_CNK is for Blue Gene/Q, which seems to be a Linux-like, maybe @hfinkel might know whether this OS supports getpagesize()? If so, the case can be simplified to providing a getpagesize() only for Windows, then defining KMP_GET_PAGE_SIZE() to getpagesize() unconditionally.

[dim]

Use getpagesize() on all platforms, and add a Windows implementation of it.

[zturner]

You'll probably need to #include <windows.h>. As I'm not familiar with this code, there may be issues with including windows.h from a header which I'm not aware of (it doesn't appear to be included in this header file). (also, it's SYSTEM_INFO, not SYSTEMINFO)

[dim]

Move inclusion of windows.h a bit higher, and change SYSTEMINFO to SYSTEM_INFO.

[hfinkel]

In D35072#801007, @dim wrote:

Apparently KMP_OS_CNK is for Blue Gene/Q, which seems to be a Linux-like, maybe @hfinkel might know whether this OS supports getpagesize()? If so, the case can be simplified to providing a getpagesize() only for Windows, then defining KMP_GET_PAGE_SIZE() to getpagesize() unconditionally.

Yes, I'm fairly sure that CNK supports getpagesize(). You shouldn't have a problem here.

[AndreyChurbanov]

Isn't it safer to just put the definition of PAGE_SIZE under "#ifndef PAGE_SIZE"?

This would not require to test all the platforms affected except Linux.

[dim]

In D35072#801129, @AndreyChurbanov wrote:

Isn't it safer to just put the definition of PAGE_SIZE under "#ifndef PAGE_SIZE"?

This would not require to test all the platforms affected except Linux.

Unfortunately it isn't, since system headers can also be included after kmp_os.h, and then you get another conflict. It is better to not try to redefine system macros.

[AndreyChurbanov]

OK, thanks, I agree system macros should not be re-defined.

Then, maybe rename it into non-system one, like KMP_PAGE_SIZE? I am just scared that a lot of platforms need testing because of so little change, while there should be a way to avoid the testing, I think.

[jcownie]

In the proposed Windows code, I think it would be useful to stash the value in a static variable, so that we only make the system call once. While it's very unlikely that this is ever really going to be on a performance path, it's easy to do.
Something like

static inline int getpagesize(void) {
  static int cachedPageSize = 0;
  if (cachedPageSize == 0) {
      SYSTEM_INFO si;
      GetSystemInfo(&si);
      cachedPageSize =  si.dwPageSize;
  }
  return cachedPageSize;
}

Of course, you could also do this with a static constructor, but the order of static construction gets complicated, and this is guaranteed to be safe.

[dim]

In D35072#801197, @AndreyChurbanov wrote:

OK, thanks, I agree system macros should not be re-defined.

Then, maybe rename it into non-system one, like KMP_PAGE_SIZE? I am just scared that a lot of platforms need testing because of so little change, while there should be a way to avoid the testing, I think.

The only consumer of this PAGE_SIZE macro was the KMP_GET_PAGE_SIZE macro, so if you replace that with a call to getpagesize(), there is no need for the PAGE_SIZE macro at all anymore.

If everybody is more happy by leaving the Windows situation as-is, I'm fine with it, as I do not have any good way to test on Windows.

[dim]

Minimized changes:

• Rename PAGE_SIZE macro to KMP_PAGE_SIZE
• Remove untested Windows code and revert to just returning KMP_PAGE_SIZE
• For all other OSes, return getpagesize()

[dim]

Pinging this again. Should we go for this minimal change, or do we want the GetSystemInfo call? :)

[AndreyChurbanov]

LGTM

To me this change is the same as one with GetSystemInfo because it does change the const to getpagesize() on BSDs, BGQ, OS X (that is what I was not happy with initially because I cannot test all these platforms).

So if you think the change on these platforms is OK, then either patch with const or with GetSystemInfo looks fine to me regarding Linux and Windows. Feel free to choose one you prefer. Each one has pros and cons, so I have no strong opinion here.

[dim]

I went for using GetSystemInfo() on Windows, pagesize() everywhere else, in the end. I think that is the most complete version. I'm keeping an eye on the build bots, if anything strange happens, I will revert it ASAP.

runtime/src/kmp_os.h

 #endif // __cplusplus
 
 #define KMP_EXPORT extern /* export declaration in guide libraries */
 
 #if __GNUC__ >= 4
 #define __forceinline __inline
 #endif
 
-#define PAGE_SIZE (0x4000)
+#if KMP_OS_WINDOWS
+#include <windows.h>
 
-#if KMP_OS_LINUX
-#define KMP_GET_PAGE_SIZE() getpagesize()
-#else
-// TODO: find the corresponding function to getpagesize() in Windows
-// and use it whenever possible.
-#define KMP_GET_PAGE_SIZE() PAGE_SIZE
+static inline int getpagesize(void) {
+  SYSTEM_INFO si;
+  GetSystemInfo(&si);
+  return si.dwPageSize;
+}
 #endif
+#define KMP_GET_PAGE_SIZE() getpagesize()
 
 #define PAGE_ALIGNED(_addr)                                                    \
   (!((size_t)_addr & (size_t)(KMP_GET_PAGE_SIZE() - 1)))
 #define ALIGN_TO_PAGE(x)                                                       \
   (void *)(((size_t)(x)) & ~((size_t)(KMP_GET_PAGE_SIZE() - 1)))
 
 /* ---------- Support for cache alignment, padding, etc. ----------------*/
 
enum kmp_mem_fence_type {
   kmp_release_fence, /* Release (write) memory fence */
   kmp_full_fence /* Full (read+write) memory fence */
 };
 
 // Synchronization primitives
 
 #if KMP_ASM_INTRINS && KMP_OS_WINDOWS
 
-#include <Windows.h>
-
 #pragma intrinsic(InterlockedExchangeAdd)
 #pragma intrinsic(InterlockedCompareExchange)
 #pragma intrinsic(InterlockedExchange)
 #pragma intrinsic(InterlockedExchange64)
 
 // Using InterlockedIncrement / InterlockedDecrement causes a library loading
 // ordering problem, so we use InterlockedExchangeAdd instead.
 #define KMP_TEST_THEN_INC32(p) InterlockedExchangeAdd((volatile long *)(p), 1)