This is an archive of the discontinued LLVM Phabricator instance.

[AIX][z/OS][Support] Provide no-op mapped_file_region::dontNeedImpl implementation
ClosedPublic

Authored by daltenty on Jan 4 2022, 8:53 AM.

Details

Summary

mapped_file_region::dontNeedImpl added in D116366 calls madvise, which causes problems for z/OS and AIX.

For z/OS, we don't have either madvise, so treat this as a no-op, same as Windows does.

For AIX, it doesn't have any effect, doesn't have a standardized signature, and it needs certain feature test macros (i.e. _ALL_SOURCE) we don't set by default on for LLVM on AIX, so just make it a no-op too.

Diff Detail

Unit TestsFailed

Event Timeline

daltenty created this revision.Jan 4 2022, 8:53 AM
daltenty requested review of this revision.Jan 4 2022, 8:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 4 2022, 8:53 AM
aganea added inline comments.Jan 4 2022, 8:56 AM
llvm/lib/Support/Unix/Path.inc
875

Shouldn't we protect the contents of this function with #ifdef HAVE_SYS_MMAN_H?

daltenty updated this revision to Diff 397337.Jan 4 2022, 10:35 AM

Add HAVE_SYS_MMAN_H guard

daltenty marked an inline comment as done.Jan 4 2022, 10:35 AM
zibi added inline comments.Jan 4 2022, 10:56 AM
llvm/lib/Support/Unix/Path.inc
875

I don't think we can use HAVE_SYS_MMAN_H since this is defined on z/OS since we do have sys/mman.h header file. We just don't have madvise().

This comment was removed by daltenty.
zibi added a comment.Jan 4 2022, 11:01 AM

LGTM, but I would prefer if we could use ::posix_madvise() not just on AIX. This will remove the unnecessary directive path.
However, we need to buy-in from the originator which introduced this call.

daltenty added inline comments.Jan 4 2022, 11:02 AM
llvm/lib/Support/Unix/Path.inc
875

The version here now should provide the fallback for both the case where we the header isn't available or if we are on z/OS, so I think we're ok.

LGTM, but I would prefer if we could use ::posix_madvise() not just on AIX. This will remove the unnecessary directive path.

However, we need to buy-in from the originator which introduced this call.

Unfortunately it sounds like there is a different semantic for glibc on Linux though (namely the kernel is allowed to immediately discard dirty pages, which is why we assert the readonly property I assume), so the POSIX version is a no-op there, so we can't really unify the paths or we'll break the original behaviour. Worse, looking at the trace AIX I think we also don't make the system call, so I think we may need to abandon posix_madvise.

daltenty planned changes to this revision.Jan 4 2022, 11:25 AM
daltenty updated this revision to Diff 397362.Jan 4 2022, 11:50 AM

Use madvise on AIX as well, since the semantics of posix_madvise don't match,

daltenty edited the summary of this revision. (Show Details)Jan 4 2022, 11:51 AM
daltenty updated this revision to Diff 397363.Jan 4 2022, 11:55 AM

Update comment

daltenty updated this revision to Diff 397365.Jan 4 2022, 12:02 PM

Fix indent

POSIX specifies sys/mman.h and it is available on all major *NIX operation systems, so I don't think we need HAVE_SYS_MMAN_H.

We cannot use posix_madvise on Linux glibc/musl because MADV_DONTNEED (desired semantics on a read-only file mapping) is a no-op in their posix_madvise implementations.

If madvise doesn't have a benefit on AIX, consider just not implementing it to avoid some CMake complexity.

llvm/lib/Support/CMakeLists.txt
3 ↗(On Diff #397365)
llvm/lib/Support/Unix/Path.inc
875–883

if (Mapping) { or use early return

If madvise doesn't have a benefit on AIX, consider just not implementing it to avoid some CMake complexity.

I think you are right, I did some more digging and it seems like the OS is unlikely to use the information we are giving it any way:

The madvise subroutine is provided for compatibility only. The system takes no action on the advice specified.

https://www.ibm.com/docs/en/aix/7.1?topic=memory-understanding-mapping
https://www.ibm.com/docs/en/aix/7.1?topic=m-madvise-subroutine

I'll simplify this patch as suggested.

daltenty updated this revision to Diff 397408.Jan 4 2022, 2:39 PM
daltenty edited the summary of this revision. (Show Details)

Simply the patch since we've confirmed madvise doesn't have a useful effect on AIX anyway

daltenty retitled this revision from [AIX][z/OS][Support] Provide alternate mapped_file_region::dontNeedImpl implementations to [AIX][z/OS][Support] Provide no-op mapped_file_region::dontNeedImpl implementation.Jan 4 2022, 2:39 PM
daltenty marked an inline comment as done.

LGTM with minor formatting comment

llvm/lib/Support/Unix/Path.inc
874

Can this be #endif instead of #else? Not sure if this counts as not using an else after return.

This revision is now accepted and ready to land.Jan 5 2022, 6:42 AM
daltenty added inline comments.Jan 5 2022, 7:10 AM
llvm/lib/Support/Unix/Path.inc
874

IIUC what you're suggesting, we'd end up including the problem callsite bellow then, so I think we need to keep this.

This revision was landed with ongoing or failed builds.Jan 5 2022, 7:21 AM
This revision was automatically updated to reflect the committed changes.