Page MenuHomePhabricator

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

Authored by dim on Jul 6 2017, 10:32 AM.

Details

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
)

Diff Detail

Repository
rL LLVM

Event Timeline

dim created this revision.Jul 6 2017, 10:32 AM

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 added a comment.Jul 6 2017, 10:39 AM

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 added a comment.Jul 6 2017, 10:42 AM

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 edited subscribers, added: hfinkel; removed: openmp-commits.Jul 6 2017, 10:47 AM

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 updated this revision to Diff 105483.Jul 6 2017, 10:57 AM

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

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 updated this revision to Diff 105488.Jul 6 2017, 11:11 AM

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

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.

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 added a comment.Jul 6 2017, 11:38 AM

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.

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 edited edge metadata.Jul 7 2017, 1:20 AM

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 added a comment.Jul 8 2017, 5:35 AM

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 updated this revision to Diff 105739.Jul 8 2017, 5:39 AM

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 added a comment.Jul 11 2017, 11:48 AM

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

AndreyChurbanov accepted this revision.Jul 12 2017, 1:25 AM

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.

This revision is now accepted and ready to land.Jul 12 2017, 1:25 AM
This revision was automatically updated to reflect the committed changes.
dim added a comment.Jul 18 2017, 1:33 PM

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.