Bug #4527

[PATCH] IO#close releases GVL if possible

Added by Eric Wong over 4 years ago. Updated about 1 year ago.

[ruby-core:35555]
Status:Closed
Priority:Normal
Assignee:Motohiro KOSAKI
ruby -v:ruby 1.9.3dev (2011-03-25 trunk 31181) [x86_64-linux] Backport:2.0.0: DONTNEED, 2.1: DONTNEED

Description

=begin
close() may block for certain file types (NFS, SO_LINGER
sockets, inotify), so let other threads run.
=end

0001-IO-close-releases-GVL-if-possible.patch Magnifier - [PATCH] IO#close releases GVL if possible (1.6 KB) Eric Wong, 03/26/2011 03:35 AM


Related issues

Related to Ruby trunk - Bug #4558: TestSocket#test_closed_read fails after r31230 Closed 04/07/2011
Related to Ruby trunk - Feature #4570: [PATCH v2] io.c (rb_io_close): release GVL if possible Closed 04/12/2011

Associated revisions

Revision 31230
Added by Motohiro KOSAKI over 4 years ago

  • io.c (io_reopen): IO#close releases GVL if possible.
    close() may block for certain file types (NFS, SO_LINGER
    sockets, inotify), so let other threads run. The patch was
    created by Eric Wong [Bug #4527]

  • io.c (fptr_finalize): ditto.

  • io.c (maygvl_fclose): new.

  • io.c (nogvl_fclose): ditto.

  • io.c (maygvl_close): ditto.

  • io.c (nogvl_close): ditto.

Revision 31230
Added by Motohiro KOSAKI over 4 years ago

  • io.c (io_reopen): IO#close releases GVL if possible.
    close() may block for certain file types (NFS, SO_LINGER
    sockets, inotify), so let other threads run. The patch was
    created by Eric Wong [Bug #4527]

  • io.c (fptr_finalize): ditto.

  • io.c (maygvl_fclose): new.

  • io.c (nogvl_fclose): ditto.

  • io.c (maygvl_close): ditto.

  • io.c (nogvl_close): ditto.

Revision 36932
Added by Motohiro KOSAKI almost 3 years ago

  • io.c (nogvl_close, maygvl_close, nogvl_fclose, maygvl_fclose): new functions.
  • io.c (fptr_finalize): release GVL if possible. Patched by Eric Wong. [Feature #4570]

History

#1 Updated by Motohiro KOSAKI over 4 years ago

  • Assignee set to Motohiro KOSAKI
  • Status changed from Open to Assigned

Right. good catch.

#2 Updated by Motohiro KOSAKI over 4 years ago

  • Status changed from Assigned to Closed

Eric, I commited slightly modified version because your patch couldn't apply for latest trunk. Could you please confirm it?

#3 Updated by Eric Wong over 4 years ago

Motohiro KOSAKI kosaki.motohiro@gmail.com wrote:

Eric, I commited slightly modified version because your patch couldn't
apply for latest trunk. Could you please confirm it?

Thanks, looks like you missed the following hunk in rb_io_reopen():

diff --git a/io.c b/io.c
index 7ce7148..0c1593b 100644
--- a/io.c
+++ b/io.c
@@ -5930,7 +5930,7 @@ rb_io_reopen(int argc, VALUE *argv, VALUE file)
 #endif
     }
     else {
-        if (close(fptr->fd) < 0)
+        if (maygvl_close(fptr->fd, 0) < 0)
             rb_sys_fail_path(fptr->pathv);
         fptr->fd = -1;
         fptr->fd = rb_sysopen(fptr->pathv, oflags, 0666);

Eric Wong

#4 Updated by Motohiro KOSAKI over 4 years ago

  • Status changed from Closed to Rejected

The patch reverted because it made a regression (see [Bug #4558])

#5 Updated by Eric Wong over 4 years ago

Motohiro KOSAKI kosaki.motohiro@gmail.com wrote:

Eric, I commited slightly modified version because your patch couldn't
apply for latest trunk. Could you please confirm it?

Thanks, looks like you missed the following hunk in rb_io_reopen():

diff --git a/io.c b/io.c
index 7ce7148..0c1593b 100644
--- a/io.c
+++ b/io.c
@@ -5930,7 +5930,7 @@ rb_io_reopen(int argc, VALUE *argv, VALUE file)
 #endif
     }
     else {
-        if (close(fptr->fd) < 0)
+        if (maygvl_close(fptr->fd, 0) < 0)
             rb_sys_fail_path(fptr->pathv);
         fptr->fd = -1;
         fptr->fd = rb_sysopen(fptr->pathv, oflags, 0666);

Eric Wong

#6 Updated by Yui NARUSE about 1 year ago

  • Description updated (diff)
  • Backport set to 2.0.0: REQUIRED, 2.1: REQUIRED
  • Target version deleted (2.0.0)
  • Status changed from Rejected to Assigned

#7 Updated by Yui NARUSE about 1 year ago

A patch proposed Eric in is not merged yet.

#8 Updated by Eric Wong about 1 year ago

naruse@airemix.jp wrote:

A patch proposed Eric in is not merged yet.

Won't apply after r43373 (which is way more important).

I don't think the current close(tmpfd) needs to release GVL in the
new code path it is dropping a refcount after rb_cloexec_dup2;
so nothing expensive going on in the kernel.

close performance on inotify descriptors (my main reason for this) is
much improved since 2011, too. AFAIK the heavy lifting was moved to
asynchronous kernel workqueues.

#9 Updated by Yui NARUSE about 1 year ago

  • Status changed from Assigned to Closed
  • Backport changed from 2.0.0: REQUIRED, 2.1: REQUIRED to 2.0.0: DONTNEED, 2.1: DONTNEED

Eric Wong wrote:

naruse@airemix.jp wrote:

A patch proposed Eric in is not merged yet.

Won't apply after r43373 (which is way more important).

I don't think the current close(tmpfd) needs to release GVL in the
new code path it is dropping a refcount after rb_cloexec_dup2;
so nothing expensive going on in the kernel.

close performance on inotify descriptors (my main reason for this) is
much improved since 2011, too. AFAIK the heavy lifting was moved to
asynchronous kernel workqueues.

Sure, thanks for explanation.

Also available in: Atom PDF