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

Drop code under #if HAVE_POSIX_MEMALIGN. #52

Closed
wants to merge 1 commit into from

Conversation

zackw
Copy link
Collaborator

@zackw zackw commented Nov 1, 2018

alg-yescrypt-platform.c contains code to use posix_memalign if mmap is
unavailable, but it’s not properly wired up to autoconf, so it never
actually gets used. I don’t think it’s worth keeping, because any
system that bothers to supply posix_memalign probably also has mmap.

I'd like @solardiz to comment on this change.

(This addresses one of the issues described in #30.)

alg-ysecrypt-platform.c contains code to use posix_memalign if mmap is
unavailable, but it’s not properly wired up to autoconf, so it never
actually gets used.  I don’t think it’s worth keeping, because any
system that bothers to supply posix_memalign probably also has mmap.
@solardiz
Copy link
Collaborator

solardiz commented Nov 1, 2018

I support this change and am likely to make the same change upstream.

However, I disagree with part of the reasoning that "any system that bothers to supply posix_memalign probably also has mmap" - I guess Windows is a notable exception here, it just might not be relevant to libxcrypt. My own reasoning is that we maintain alternative source code (and struct fields to support it) anyway and it isn't slower - it's just some trivial extra calculations. We can accept this trivial runtime complication on systems that have posix_memalign in order to simplify the source code and improve the coverage of its testing.

@zackw
Copy link
Collaborator Author

zackw commented Nov 1, 2018

I think I understand. Would you describe this as a better rationale for the change?

It is not worth attempting to use posix_memalign when mmap is not available, because the existing fallback to malloc + manual alignment is good enough for that situation.

(N.B. Current versions of Windows offer huge page allocations via VirtualAlloc, that might be worth looking into.)

@solardiz
Copy link
Collaborator

solardiz commented Nov 1, 2018

Yes, that wording for the rationale sounds better to me. And thanks for the info on Windows VirtualAlloc.

@zackw zackw closed this Nov 1, 2018
@zackw zackw deleted the drop-posix-memalign branch November 1, 2018 16:01
@zackw
Copy link
Collaborator Author

zackw commented Nov 1, 2018

Rebased and pushed with the edited rationale.

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

Successfully merging this pull request may close these issues.

2 participants