Page MenuHomeFreeBSD

Preallocate pipe buffers on pipe creation.
ClosedPublic

Authored by kib on Mar 7 2020, 11:36 AM.
Tags
None
Referenced Files
F106003298: D23993.diff
Mon, Dec 23, 5:44 PM
Unknown Object (File)
Fri, Dec 13, 11:57 PM
Unknown Object (File)
Sat, Nov 30, 2:28 PM
Unknown Object (File)
Nov 15 2024, 11:48 AM
Unknown Object (File)
Nov 5 2024, 12:11 PM
Unknown Object (File)
Oct 2 2024, 8:29 AM
Unknown Object (File)
Sep 30 2024, 2:44 AM
Unknown Object (File)
Sep 17 2024, 5:58 AM
Subscribers

Details

Summary

Return ENOMEM if one of the buffer cannot be created even with the minimal size. This should avoid subsequent spurious ENOMEM errors from e.g. write(2) when buffer cannot be allocated later.

Style cleanup is for separate commit.

Reported by: Keno Fischer <keno@juliacomputing.com>

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 29837

Event Timeline

I wrote a test program for this, see https://gist.github.com/kostikbel/b2844258b7fba6e8ce3ccd8ef9422e5a
Results for the stock HEAD:

created 116544 pipes. syscall error Too many open files
pipe 20229 fds 40461 40462 error Cannot allocate memory
pipe 20230 fds 40463 40464 error Cannot allocate memory
pipe 20231 fds 40465 40466 error Cannot allocate memory
pipe 20232 fds 40467 40468 error Cannot allocate memory
pipe 20233 fds 40469 40470 error Cannot allocate memory

and a lot more, practically for all pipes after 20229.

For the patched kernel, the output is

created 11326 pipes. syscall error Cannot allocate memory

which is IMO much better, since apps are normally prepared to errors at the objects creation time, but less so in the steady stages of the operation.

Also it should be noted that patched kernel allowed to create 10x times less pipes, which is expected but shows that we no longer overcommit. I might change the patch to create initial buffer with SMALL_PIPE_SIZE always, not only for the direct buffer, but I do not think it is important.

kib added reviewers: markj, jilles.

I might change the patch to create initial buffer with SMALL_PIPE_SIZE always, not only for the direct buffer, but I do not think it is important.

I don't understand what you mean about the direct buffer.

sys/kern/sys_pipe.c
652

Shouldn't we fall back to a SMALL_PIPE_SIZE allocation if this fails, e.g., due to fragmentation of the map?

653

It might be useful to have a comment stating that the unit number is leaked. Otherwise it looks like a bug.

markj added inline comments.
sys/kern/sys_pipe.c
652

I missed that pipespace_new() handles this already.

This revision is now accepted and ready to land.Mar 9 2020, 2:59 PM

I might change the patch to create initial buffer with SMALL_PIPE_SIZE always, not only for the direct buffer, but I do not think it is important.

I don't understand what you mean about the direct buffer.

We have the direct direction, writes go to pipe[1] and reads from pipe[0], which is the only direction supported by classic Unix. And we have reverse direction, from pipe[0] to pipe[1]. Both directions need buffers, but direct direction is used much more often since bidirectional pipes are not portable. See the large_backing argument for pipe_create(), which is false for reverse buffer.

sys/kern/sys_pipe.c
653

Well, I decided that it is too much work for no return to clean unused inode number in case the second half of the pipe pair failed to allocate the buffer. More, I think that unrhdr would become fragmented, without deallocation we get the ideal single continuous block.

I wil say something after the current patch is committed.

In D23993#527812, @kib wrote:

I might change the patch to create initial buffer with SMALL_PIPE_SIZE always, not only for the direct buffer, but I do not think it is important.

I don't understand what you mean about the direct buffer.

We have the direct direction, writes go to pipe[1] and reads from pipe[0], which is the only direction supported by classic Unix. And we have reverse direction, from pipe[0] to pipe[1]. Both directions need buffers, but direct direction is used much more often since bidirectional pipes are not portable. See the large_backing argument for pipe_create(), which is false for reverse buffer.

I see, I thought you were referring to direct writes somehow.

This revision was automatically updated to reflect the committed changes.
kib added a commit: rS358816: Style..
This revision was not accepted when it landed; it landed in state Needs Review.Mar 9 2020, 9:55 PM
This revision was automatically updated to reflect the committed changes.