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.

Sign up for GitHub

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

Jump to bottom

btl/vader: handle unexpected short read/write in process_vm_{read,write}v #4832

Merged
merged 1 commit into from
Mar 2, 2018
Merged

btl/vader: handle unexpected short read/write in process_vm_{read,write}v #4832

merged 1 commit into from
Mar 2, 2018

Conversation

ggouaillardet
Copy link
Contributor

Thanks Heiko Bauke for the bug report.

Refs. #4829

Signed-off-by: Gilles Gouaillardet [email protected]

bosilca
bosilca reviewed Feb 19, 2018
View reviewed changes
Copy link
Member

@bosilca bosilca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I commented on the issue #4829 this fix do not match the expected behavior of these functions according to their man pages.

@ggouaillardet
Copy link
Contributor Author

@bosilca I also read

(Partial transfers apply at the granularity of iovec elements. These system calls won't perform a partial transfer that splits a single iovec element.)

Since our iovec has only one element, does this behavior points to a bug in glibc/kernel ?
(partial transfer is not permitted, and hence the returned value should be 0 or the full buffer)

@bosilca
Copy link
Member

bosilca commented Feb 19, 2018

I concur. I run on 2 kernel versions and it is the same behavior, and I did not had the time to look further.

@jsquyres
Copy link
Member

If this behavior is contrary to system man pages, I think you should definitely add a comment in both locations explaining why this code is necessary (and cite the specific system/OS/distro/version/whatever where the non-manpage-conforming behavior was seen). This won't matter in the next few months, but 2 years from now, someone will look at this code and say "That code is unnecessary because the man page guarantees that the unit of granularity of transfer is a single iovec!" -- opening the door to a possible regression if they delete this extra logic.

@jsquyres
Copy link
Member

Per 2018-02-20 webex:

  1. We agree that the comments should be added to this PR (as cited above).
  2. We should probably investigate why the man page digresses from the observed behavior. E.g., is this a glibc issue? Is it a kernel issue? Is this a regression (e.g., it used to obey iovec boundaries)? ...etc.
  3. This is an important issue and we should fix it, but given that it has existed in all of our release branches for years and this is the first time we're hearing about it (@bosilca says it only happens when you try to send >2GB in a single message), it doesn't feel like a blocker for any just-about-to-be-released version.

@hjelmn
Copy link
Member

hjelmn commented Mar 2, 2018

@ggouaillardet Can you add the comments that @jsquyres mentioned. I would like to get this into master.

@ggouaillardet ggouaillardet force-pushed the topic/vader_process_vm branch from 373bc9c to 3d6f714 Compare March 2, 2018 04:20
@ggouaillardet
btl/vader: handle unexpected short read/write in process_vm_{read,wri…
9fedf28 
…te}v

Important note :

According to the man page
"On success, process_vm_readv() returns the number of bytes read and
process_vm_writev() returns the number of bytes written.  This return
value may be less than the total number of requested bytes, if a
partial read/write occurred.  (Partial transfers apply at the
granularity of iovec elements.  These system calls won't perform a
partial transfer that splits a single iovec element.)"

So since we use a single iovec element, the returned size should either
be 0 or size, and the do loop should not be needed here.
We tried on various Linux kernels with size > 2 GB, and surprisingly,
the returned value is always 0x7ffff000 (fwiw, it happens to be the size
of the larger number of pages that fits a signed 32 bits integer).
We do not know whether this is a bug from the kernel, the libc or even
the man page, but for the time being, we do as is process_vm_readv() could
return any value.

Thanks Heiko Bauke for the bug report.

Refs. open-mpi#4829

Signed-off-by: Gilles Gouaillardet <[email protected]>
@ggouaillardet ggouaillardet force-pushed the topic/vader_process_vm branch from 3d6f714 to 9fedf28 Compare March 2, 2018 04:21
@ggouaillardet ggouaillardet changed the title btl/vader: correctly handle short read/write in process_vm_{read,write}v btl/vader: handle unexpected short read/write in process_vm_{read,write}v Mar 2, 2018
@ggouaillardet
Copy link
Contributor Author

@hjelmn I made the changes and will merge when CI completes

@ggouaillardet ggouaillardet merged commit f15d620 into open-mpi:master Mar 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bosilca bosilca bosilca left review comments

Assignees
No one assigned
Labels
bug Target: main
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ggouaillardet @bosilca @jsquyres @hjelmn