[PATCH 1/2] t/lei-sigpipe.t: use correct pipe size

[PATCH 1/2] t/lei-sigpipe.t: use correct pipe size

[Alyssa Ross]

According to fcntl(2), Linux will round up pipe sizes lower than a
page to a whole page.  Additionally, the kernel may use a larger size
in general, "if that is convenient for the implementation".  The
actual size of the pipe is returned from fcntl, so the test should use
that return value, rather than assuming the size requested was
accepted exactly.  This fixes the test on my system with 16K pages.
---
 t/lei-sigpipe.t | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/lei-sigpipe.t b/t/lei-sigpipe.t
index b9fd88a6..c01d9f83 100644
--- a/t/lei-sigpipe.t
+++ b/t/lei-sigpipe.t
@@ -33,8 +33,8 @@ test_lei(sub {
 	my $imported;
 	for my $out ([], [qw(-f mboxcl2)], [qw(-f text)]) {
 		pipe(my $r, my $w);
-		my $size = $F_SETPIPE_SZ && fcntl($w, $F_SETPIPE_SZ, 4096) ?
-			4096 : 65536;
+		my $size = ($F_SETPIPE_SZ && fcntl($w, $F_SETPIPE_SZ, 4096)) ||
+		    65536;
 		unless (-f $f) {
 			my $fh = write_file '>', $f, <<'EOM';
 From: big@example.com

base-commit: 7ec09f72bc12c3b882ba56c8bf16565f39afbc79
-- 
2.47.0

[PATCH 2/2] treewide: lose F_SETPIPE_SZ page size assumptions

[Alyssa Ross]

fcntl(2) is documented as rounding up sizes to an integer multiple of
the page size.  When a pipe buffer as small as possible is desired,
it's cleaner to pass 0 and let the kernel set the pipe buffer to
exactly one page than it is to assume that pages are 4096 bytes.

Currently, Linux doesn't support pages smaller than 4096 bytes, so this 
is a purely stylistic change, but I think it will help avoid future 
bugs caused by assuming page sizes are 4096 bytes.

 lib/PublicInbox/EOFpipe.pm        | 4 ++--
 lib/PublicInbox/LeiXSearch.pm     | 6 +++---
 lib/PublicInbox/SearchIdxShard.pm | 2 +-
 t/gcf2.t                          | 4 ++--
 t/lei-sigpipe.t                   | 2 +-
 5 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/lib/PublicInbox/EOFpipe.pm b/lib/PublicInbox/EOFpipe.pm
index 3eaa0e4e..9fe9f67d 100644
--- a/lib/PublicInbox/EOFpipe.pm
+++ b/lib/PublicInbox/EOFpipe.pm
@@ -10,8 +10,8 @@ use Errno qw(EAGAIN);
 sub new {
 	my (undef, $rd, @cb_args) = @_;
 	my $self = bless { cb_args => \@cb_args }, __PACKAGE__;
-	# 4096: page size
-	fcntl($rd, $F_SETPIPE_SZ, 4096) if $F_SETPIPE_SZ;
+	# 0: one page
+	fcntl($rd, $F_SETPIPE_SZ, 0) if $F_SETPIPE_SZ;
 	$rd->blocking(0); # avoid EINTR
 	$self->SUPER::new($rd, EPOLLIN); # level trigger for spurious wakeup
 }
diff --git a/lib/PublicInbox/LeiXSearch.pm b/lib/PublicInbox/LeiXSearch.pm
index e20b13c6..909e16ed 100644
--- a/lib/PublicInbox/LeiXSearch.pm
+++ b/lib/PublicInbox/LeiXSearch.pm
@@ -545,16 +545,16 @@ sub do_query {
 			# setup two barriers to coordinate ->has_entries
 			# between l2m workers
 			pipe(my ($a_r, $a_w)) or die "pipe: $!";
-			fcntl($a_r, $F_SETPIPE_SZ, 4096) if $F_SETPIPE_SZ;
+			fcntl($a_r, $F_SETPIPE_SZ, 0) if $F_SETPIPE_SZ;
 			pipe(my ($b_r, $b_w)) or die "pipe: $!";
-			fcntl($b_r, $F_SETPIPE_SZ, 4096) if $F_SETPIPE_SZ;
+			fcntl($b_r, $F_SETPIPE_SZ, 0) if $F_SETPIPE_SZ;
 			$l2m->{au_peers} = [ $a_r, $a_w, $b_r, $b_w ];
 		}
 		$l2m->wq_workers_start('lei2mail', undef,
 					$lei->oldset, { lei => $lei },
 					\&xsearch_done_wait, $lei);
 		pipe($lei->{startq}, $lei->{au_done}) or die "pipe: $!";
-		fcntl($lei->{startq}, $F_SETPIPE_SZ, 4096) if $F_SETPIPE_SZ;
+		fcntl($lei->{startq}, $F_SETPIPE_SZ, 0) if $F_SETPIPE_SZ;
 		delete $l2m->{au_peers};
 		close(delete $l2m->{-wq_s2}); # share wq_s1 with lei_xsearch
 	}
diff --git a/lib/PublicInbox/SearchIdxShard.pm b/lib/PublicInbox/SearchIdxShard.pm
index 7ee8a121..c12aef9e 100644
--- a/lib/PublicInbox/SearchIdxShard.pm
+++ b/lib/PublicInbox/SearchIdxShard.pm
@@ -26,7 +26,7 @@ sub new {
 		# are small, make the response pipe as small as possible
 		if ($F_SETPIPE_SZ) {
 			fcntl($self->{-ipc_req}, $F_SETPIPE_SZ, 1048576);
-			fcntl($self->{-ipc_res}, $F_SETPIPE_SZ, 4096);
+			fcntl($self->{-ipc_res}, $F_SETPIPE_SZ, 0);
 		}
 	}
 	$self;
diff --git a/t/gcf2.t b/t/gcf2.t
index 33f3bbca..28c69a01 100644
--- a/t/gcf2.t
+++ b/t/gcf2.t
@@ -110,7 +110,7 @@ SKIP: {
 	for my $blk (1, 0) {
 		my ($r, $w);
 		pipe($r, $w) or BAIL_OUT $!;
-		fcntl($w, $F_SETPIPE_SZ, 4096) or
+		fcntl($w, $F_SETPIPE_SZ, 0) or
 			skip('Linux too old for F_SETPIPE_SZ', 14);
 		$w->blocking($blk);
 		seek($fh, 0, SEEK_SET) or BAIL_OUT "seek: $!";
@@ -130,7 +130,7 @@ SKIP: {
 		$ck_copying->("pipe blocking($blk)");
 
 		pipe($r, $w) or BAIL_OUT $!;
-		fcntl($w, $F_SETPIPE_SZ, 4096) or BAIL_OUT $!;
+		fcntl($w, $F_SETPIPE_SZ, 0) or BAIL_OUT $!;
 		$w->blocking($blk);
 		close $r;
 		local $SIG{PIPE} = 'IGNORE';
diff --git a/t/lei-sigpipe.t b/t/lei-sigpipe.t
index c01d9f83..f093dc5c 100644
--- a/t/lei-sigpipe.t
+++ b/t/lei-sigpipe.t
@@ -33,7 +33,7 @@ test_lei(sub {
 	my $imported;
 	for my $out ([], [qw(-f mboxcl2)], [qw(-f text)]) {
 		pipe(my $r, my $w);
-		my $size = ($F_SETPIPE_SZ && fcntl($w, $F_SETPIPE_SZ, 4096)) ||
+		my $size = ($F_SETPIPE_SZ && fcntl($w, $F_SETPIPE_SZ, 0)) ||
 		    65536;
 		unless (-f $f) {
 			my $fh = write_file '>', $f, <<'EOM';
-- 
2.47.0

Re: [PATCH 1/2] t/lei-sigpipe.t: use correct pipe size

[Eric Wong]

Alyssa Ross <hi@alyssa.is> wrote:
> According to fcntl(2), Linux will round up pipe sizes lower than a
> page to a whole page.  Additionally, the kernel may use a larger size
> in general, "if that is convenient for the implementation".  The
> actual size of the pipe is returned from fcntl, so the test should use
> that return value, rather than assuming the size requested was
> accepted exactly.  This fixes the test on my system with 16K pages.

Good catch, will apply.  Thanks.

> --- a/t/lei-sigpipe.t
> +++ b/t/lei-sigpipe.t
> @@ -33,8 +33,8 @@ test_lei(sub {
>  	my $imported;
>  	for my $out ([], [qw(-f mboxcl2)], [qw(-f text)]) {
>  		pipe(my $r, my $w);
> -		my $size = $F_SETPIPE_SZ && fcntl($w, $F_SETPIPE_SZ, 4096) ?
> -			4096 : 65536;
> +		my $size = ($F_SETPIPE_SZ && fcntl($w, $F_SETPIPE_SZ, 4096)) ||
> +		    65536;

Re: [PATCH 2/2] treewide: lose F_SETPIPE_SZ page size assumptions

[Eric Wong]

Alyssa Ross <hi@alyssa.is> wrote:
> fcntl(2) is documented as rounding up sizes to an integer multiple of
> the page size.  When a pipe buffer as small as possible is desired,
> it's cleaner to pass 0 and let the kernel set the pipe buffer to
> exactly one page than it is to assume that pages are 4096 bytes.

OK.  I suppose the Linux kernel has enough testers that it's
chances of it mishandling someday 0 would be caught before any
release.

> Currently, Linux doesn't support pages smaller than 4096 bytes, so this 
> is a purely stylistic change, but I think it will help avoid future 
> bugs caused by assuming page sizes are 4096 bytes.

I'm not 100% convinced it's worth the noise since I expect page
sizes to move towards larger sizes in future arches.  However, I
don't think it conflicts with anything that's being worked on
atm; so leaning towards applying...  Will think about it.

Re: [PATCH 2/2] treewide: lose F_SETPIPE_SZ page size assumptions

[Eric Wong]

Eric Wong <e@80x24.org> wrote:
> Alyssa Ross <hi@alyssa.is> wrote:
> > fcntl(2) is documented as rounding up sizes to an integer multiple of
> > the page size.  When a pipe buffer as small as possible is desired,
> > it's cleaner to pass 0 and let the kernel set the pipe buffer to
> > exactly one page than it is to assume that pages are 4096 bytes.
> 
> OK.  I suppose the Linux kernel has enough testers that it's
> chances of it mishandling someday 0 would be caught before any
> release.

Well, that was only thinking about potential future bugs...

Looking backwards, older Linux appeared to handle 0 improperly before
d3f14c485867 (pipe: avoid round_pipe_size() nr_pages overflow on 32-bit, 2017-11-17)
https://yhbt.net/lore/lkml/d3f14c485867cfb2e0c48aa88c41d0ef4bf5209c/s/

I haven't tried an ancient kernel to verify and my arithmetic is awful
(d3f14c485867 also got backported to older distro kernels).