Preallocate pipe buffers on pipe creation.

[kib]

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 <[e-mail]>

[kib]

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.

[markj]

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 ↗

(On Diff #69288)

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

653 ↗

(On Diff #69288)

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

[markj]

sys/kern/sys_pipe.c

652 ↗

(On Diff #69288)

I missed that pipespace_new() handles this already.

[kib]

In D23993#527802, @markj 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.

sys/kern/sys_pipe.c

653 ↗

(On Diff #69288)

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.

[markj]

In D23993#527812, @kib wrote:

In D23993#527802, @markj 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.