bpo-32941: Add madvise() for mmap objects by ZackerySpytz · Pull Request #6172 · python/cpython (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation32 Commits7 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

ZackerySpytz

@ZackerySpytz

Allow mmap objects to access the madvise() system call.

pitrou

pitrou

pitrou

pitrou

pitrou

Choose a reason for hiding this comment

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

Thanks for doing this! I left a couple comments that need to be addressed.

@bedevere-bot

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be put in the comfy chair!

@ZackerySpytz

@mhvk

Over at numpy/numpy#13172, it became clear that having this would be very useful for ~numpy.memmap (which wraps a mmap as an array). @ZackerySpytz - will you be able to implement requested changes? If not, I'll note in the numpy issue that it would be good for someone to take it over; it seems fairly straightforward.

@pitrou

Might also be useful for multiprocessing. @applio

@ZackerySpytz

vadmium

@ZackerySpytz

I will make the requested changes.

@pitrou, thank you for the review, and I'm sorry for the enormous delay.

@ZackerySpytz

@ZackerySpytz

Allow lengths greater than self->size. Add docs for MADV_* Constants.

@ZackerySpytz

I have made the requested changes; please review again.

I have also added a sentence to the docs explaining that start must be a multiple of the PAGESIZE.

@bedevere-bot

Thanks for making the requested changes!

@pitrou: please review the changes made to this pull request.

pitrou

Choose a reason for hiding this comment

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

Thank you! A few nits below, otherwise the PR looks good to me.

return NULL;
}
if (length <= 0) {
PyErr_SetString(PyExc_ValueError, "madvise length invalid");

Choose a reason for hiding this comment

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

Here as well: perhaps "madvise length must be > 0"?

Choose a reason for hiding this comment

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

By the way, Linux madvise() allows length to be 0 (according to its manpage).

Choose a reason for hiding this comment

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

Well, I'm not sure why one would pass a length of zero. I think it's okay to change the check to length < 0, though.

@bedevere-bot

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@pitrou

(also, please ensure you rebase or merge from master and fix conflicts)

@ZackerySpytz

@ZackerySpytz

@ZackerySpytz

Thanks, @pitrou.

I have made the requested changes; please review again.

@bedevere-bot

Thanks for making the requested changes!

@pitrou: please review the changes made to this pull request.

@ZackerySpytz

@pitrou Ping. I would like to get this merged.

@pitrou

Yes, I'm taking a final look this evening.

pitrou

@pitrou

@bedevere-bot

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot PPC64 Fedora 3.x has failed when building commit 02db696.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/53/builds/3027) and take a look at the build logs.
  4. Check if the failure is related to this commit (02db696) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/53/builds/3027

Click to see traceback logs

From https://github.com/python/cpython

Objects/obmalloc.c:1376:1: warning: ‘no_sanitize_thread’ attribute directive ignored [-Wattributes] { ^

test_devpoll skipped -- test works only on Solaris OS family test_ossaudiodev skipped -- [Errno 2] No such file or directory: '/dev/dsp' test_kqueue skipped -- test works only on BSD test_ttk_guionly skipped -- Tk unavailable due to TclError: no display name and no $DISPLAY environment variab [...] test_winreg skipped -- No module named 'winreg' test_tk skipped -- Tk unavailable due to TclError: no display name and no $DISPLAY environment variab [...] /home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64/build/Lib/multiprocessing/resource_tracker.py:203: UserWarning: resource_tracker: There appear to be 1 leaked shared_memory objects to clean up at shutdown warnings.warn('resource_tracker: There appear to be %d ' /home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64/build/Lib/multiprocessing/resource_tracker.py:216: UserWarning: resource_tracker: '//psm_874f9b9b': [Errno 2] No such file or directory: '//psm_874f9b9b' warnings.warn('resource_tracker: %r: %s' % (name, e)) test_winsound skipped -- No module named 'winsound' test_msilib skipped -- No module named '_msi' test_startfile skipped -- object <module 'os' from '/home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64/build/Lib/os.py'> has no attribute 'startfile' test_zipfile64 skipped -- test requires loads of disk-space bytes and a long time to run test_around_2GB (test.test_mmap.LargeMmapTests) ... ok test_around_4GB (test.test_mmap.LargeMmapTests) ... ok test_large_filesize (test.test_mmap.LargeMmapTests) ... ok test_large_offset (test.test_mmap.LargeMmapTests) ... ok test_access_parameter (test.test_mmap.MmapTests) ... ok test_anonymous (test.test_mmap.MmapTests) ... ok test_bad_file_desc (test.test_mmap.MmapTests) ... ok test_basic (test.test_mmap.MmapTests) ... ok test_concat_repeat_exception (test.test_mmap.MmapTests) ... ok test_context_manager (test.test_mmap.MmapTests) ... ok test_context_manager_exception (test.test_mmap.MmapTests) ... ok test_crasher_on_windows (test.test_mmap.MmapTests) ... skipped 'requires Windows' test_double_close (test.test_mmap.MmapTests) ... ok test_empty_file (test.test_mmap.MmapTests) ... ok test_entire_file (test.test_mmap.MmapTests) ... ok test_error (test.test_mmap.MmapTests) ... ok test_extended_getslice (test.test_mmap.MmapTests) ... ok test_extended_set_del_slice (test.test_mmap.MmapTests) ... ok test_find_end (test.test_mmap.MmapTests) ... ok test_flush_return_value (test.test_mmap.MmapTests) ... ok test_invalid_descriptor (test.test_mmap.MmapTests) ... skipped 'requires Windows' test_io_methods (test.test_mmap.MmapTests) ... ok test_length_0_large_offset (test.test_mmap.MmapTests) ... ok test_length_0_offset (test.test_mmap.MmapTests) ... ok test_madvise (test.test_mmap.MmapTests) ... ERROR test_move (test.test_mmap.MmapTests) ... ok test_non_ascii_byte (test.test_mmap.MmapTests) ... ok test_offset (test.test_mmap.MmapTests) ... ok test_prot_readonly (test.test_mmap.MmapTests) ... ok test_read_all (test.test_mmap.MmapTests) ... ok test_read_invalid_arg (test.test_mmap.MmapTests) ... ok test_resize_past_pos (test.test_mmap.MmapTests) ... ok test_rfind (test.test_mmap.MmapTests) ... ok test_sizeof (test.test_mmap.MmapTests) ... skipped 'requires Windows' test_subclass (test.test_mmap.MmapTests) ... ok test_tagname (test.test_mmap.MmapTests) ... skipped 'requires Windows' test_tougher_find (test.test_mmap.MmapTests) ... ok test_weakref (test.test_mmap.MmapTests) ... ok test_write_returning_the_number_of_bytes_written (test.test_mmap.MmapTests) ... ok

====================================================================== ERROR: test_madvise (test.test_mmap.MmapTests)

Traceback (most recent call last): File "/home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64/build/Lib/test/test_mmap.py", line 754, in test_madvise m.madvise(mmap.MADV_NORMAL, PAGESIZE, sys.maxsize) ValueError: madvise start out of bounds

Ran 39 tests in 0.802s

FAILED (errors=1, skipped=4) test test_mmap failed stty: standard input: Inappropriate ioctl for device test_tix skipped -- Tk unavailable due to TclError: no display name and no $DISPLAY environment variab [...] test_winconsoleio skipped -- test only relevant on win32 test_ioctl skipped -- Unable to open /dev/tty test_flock (main.FNTLEINTRTest) ... ok test_lockf (main.FNTLEINTRTest) ... ok test_read (main.OSEINTRTest) ... ok test_wait (main.OSEINTRTest) ... ok test_wait3 (main.OSEINTRTest) ... ok test_wait4 (main.OSEINTRTest) ... ok test_waitpid (main.OSEINTRTest) ... ok test_write (main.OSEINTRTest) ... ok test_devpoll (main.SelectEINTRTest) ... skipped 'need select.devpoll' test_epoll (main.SelectEINTRTest) ... ok test_kqueue (main.SelectEINTRTest) ... skipped 'need select.kqueue' test_poll (main.SelectEINTRTest) ... ok test_select (main.SelectEINTRTest) ... ok test_sigtimedwait (main.SignalEINTRTest) ... ok test_sigwaitinfo (main.SignalEINTRTest) ... ok test_accept (main.SocketEINTRTest) ... ok test_open (main.SocketEINTRTest) ... ok test_os_open (main.SocketEINTRTest) ... ok test_recv (main.SocketEINTRTest) ... ok test_recvmsg (main.SocketEINTRTest) ... ok test_send (main.SocketEINTRTest) ... ok test_sendall (main.SocketEINTRTest) ... ok test_sendmsg (main.SocketEINTRTest) ... ok test_sleep (main.TimeEINTRTest) ... ok

Ran 24 tests in 6.757s

OK (skipped=2) test test_mmap failed make: *** [buildbottest] Error 2

@bedevere-bot

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot PPC64LE Fedora 3.x has failed when building commit 02db696.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/85/builds/2848) and take a look at the build logs.
  4. Check if the failure is related to this commit (02db696) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/85/builds/2848

Click to see traceback logs

From https://github.com/python/cpython

Objects/obmalloc.c:1376:1: warning: ‘no_sanitize_thread’ attribute directive ignored [-Wattributes] { ^

test_winreg skipped -- No module named 'winreg' test_devpoll skipped -- test works only on Solaris OS family test_msilib skipped -- No module named '_msi' test_ossaudiodev skipped -- [Errno 2] No such file or directory: '/dev/dsp' test_ioctl skipped -- Unable to open /dev/tty /home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64le/build/Lib/multiprocessing/resource_tracker.py:203: UserWarning: resource_tracker: There appear to be 1 leaked shared_memory objects to clean up at shutdown warnings.warn('resource_tracker: There appear to be %d ' /home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64le/build/Lib/multiprocessing/resource_tracker.py:216: UserWarning: resource_tracker: '//psm_101838b4': [Errno 2] No such file or directory: '//psm_101838b4' warnings.warn('resource_tracker: %r: %s' % (name, e)) stty: standard input: Inappropriate ioctl for device /home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64le/build/Lib/multiprocessing/resource_tracker.py:203: UserWarning: resource_tracker: There appear to be 1 leaked shared_memory objects to clean up at shutdown warnings.warn('resource_tracker: There appear to be %d ' /home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64le/build/Lib/multiprocessing/resource_tracker.py:216: UserWarning: resource_tracker: '//psm_cb70aca7': [Errno 2] No such file or directory: '//psm_cb70aca7' warnings.warn('resource_tracker: %r: %s' % (name, e)) test_kqueue skipped -- test works only on BSD /home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64le/build/Lib/multiprocessing/resource_tracker.py:203: UserWarning: resource_tracker: There appear to be 1 leaked shared_memory objects to clean up at shutdown warnings.warn('resource_tracker: There appear to be %d ' /home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64le/build/Lib/multiprocessing/resource_tracker.py:216: UserWarning: resource_tracker: '//psm_ea9bd3e2': [Errno 2] No such file or directory: '//psm_ea9bd3e2' warnings.warn('resource_tracker: %r: %s' % (name, e)) test_around_2GB (test.test_mmap.LargeMmapTests) ... ok test_around_4GB (test.test_mmap.LargeMmapTests) ... ok test_large_filesize (test.test_mmap.LargeMmapTests) ... ok test_large_offset (test.test_mmap.LargeMmapTests) ... ok test_access_parameter (test.test_mmap.MmapTests) ... ok test_anonymous (test.test_mmap.MmapTests) ... ok test_bad_file_desc (test.test_mmap.MmapTests) ... ok test_basic (test.test_mmap.MmapTests) ... ok test_concat_repeat_exception (test.test_mmap.MmapTests) ... ok test_context_manager (test.test_mmap.MmapTests) ... ok test_context_manager_exception (test.test_mmap.MmapTests) ... ok test_crasher_on_windows (test.test_mmap.MmapTests) ... skipped 'requires Windows' test_double_close (test.test_mmap.MmapTests) ... ok test_empty_file (test.test_mmap.MmapTests) ... ok test_entire_file (test.test_mmap.MmapTests) ... ok test_error (test.test_mmap.MmapTests) ... ok test_extended_getslice (test.test_mmap.MmapTests) ... ok test_extended_set_del_slice (test.test_mmap.MmapTests) ... ok test_find_end (test.test_mmap.MmapTests) ... ok test_flush_return_value (test.test_mmap.MmapTests) ... ok test_invalid_descriptor (test.test_mmap.MmapTests) ... skipped 'requires Windows' test_io_methods (test.test_mmap.MmapTests) ... ok test_length_0_large_offset (test.test_mmap.MmapTests) ... ok test_length_0_offset (test.test_mmap.MmapTests) ... ok test_madvise (test.test_mmap.MmapTests) ... ERROR test_move (test.test_mmap.MmapTests) ... ok test_non_ascii_byte (test.test_mmap.MmapTests) ... ok test_offset (test.test_mmap.MmapTests) ... ok test_prot_readonly (test.test_mmap.MmapTests) ... ok test_read_all (test.test_mmap.MmapTests) ... ok test_read_invalid_arg (test.test_mmap.MmapTests) ... ok test_resize_past_pos (test.test_mmap.MmapTests) ... ok test_rfind (test.test_mmap.MmapTests) ... ok test_sizeof (test.test_mmap.MmapTests) ... skipped 'requires Windows' test_subclass (test.test_mmap.MmapTests) ... ok test_tagname (test.test_mmap.MmapTests) ... skipped 'requires Windows' test_tougher_find (test.test_mmap.MmapTests) ... ok test_weakref (test.test_mmap.MmapTests) ... ok test_write_returning_the_number_of_bytes_written (test.test_mmap.MmapTests) ... ok

====================================================================== ERROR: test_madvise (test.test_mmap.MmapTests)

Traceback (most recent call last): File "/home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64le/build/Lib/test/test_mmap.py", line 754, in test_madvise m.madvise(mmap.MADV_NORMAL, PAGESIZE, sys.maxsize) ValueError: madvise start out of bounds

Ran 39 tests in 1.052s

FAILED (errors=1, skipped=4) test test_mmap failed test_zipfile64 skipped -- test requires loads of disk-space bytes and a long time to run test_tix skipped -- Tk unavailable due to TclError: no display name and no $DISPLAY environment variab [...] test_ttk_guionly skipped -- Tk unavailable due to TclError: no display name and no $DISPLAY environment variab [...] test_startfile skipped -- object <module 'os' from '/home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64le/build/Lib/os.py'> has no attribute 'startfile' test_winsound skipped -- No module named 'winsound' test_flock (main.FNTLEINTRTest) ... ok test_lockf (main.FNTLEINTRTest) ... ok test_read (main.OSEINTRTest) ... ok test_wait (main.OSEINTRTest) ... ok test_wait3 (main.OSEINTRTest) ... ok test_wait4 (main.OSEINTRTest) ... ok test_waitpid (main.OSEINTRTest) ... ok test_write (main.OSEINTRTest) ... ok test_devpoll (main.SelectEINTRTest) ... skipped 'need select.devpoll' test_epoll (main.SelectEINTRTest) ... ok test_kqueue (main.SelectEINTRTest) ... skipped 'need select.kqueue' test_poll (main.SelectEINTRTest) ... ok test_select (main.SelectEINTRTest) ... ok test_sigtimedwait (main.SignalEINTRTest) ... ok test_sigwaitinfo (main.SignalEINTRTest) ... ok test_accept (main.SocketEINTRTest) ... ok test_open (main.SocketEINTRTest) ... ok test_os_open (main.SocketEINTRTest) ... ok test_recv (main.SocketEINTRTest) ... ok test_recvmsg (main.SocketEINTRTest) ... ok test_send (main.SocketEINTRTest) ... ok test_sendall (main.SocketEINTRTest) ... ok test_sendmsg (main.SocketEINTRTest) ... ok test_sleep (main.TimeEINTRTest) ... ok

Ran 24 tests in 7.082s

OK (skipped=2) test_tk skipped -- Tk unavailable due to TclError: no display name and no $DISPLAY environment variab [...] test_winconsoleio skipped -- test only relevant on win32 test test_mmap failed make: *** [buildbottest] Error 2

@pitrou

Interesting. It seems PPC64 has a 64kB page size.

@vstinner What is the procedure to test a quick fix on a buildbot without going through Github? Edit: I found the answer.

@pitrou

DinoV pushed a commit to DinoV/cpython that referenced this pull request

Jan 14, 2020

@ZackerySpytz @DinoV

Allow mmap objects to access the madvise() system call.