* [PATCH 1/2] t/lei-sigpipe.t: use correct pipe size @ 2025-01-07 17:46 Alyssa Ross 2025-01-07 17:46 ` [PATCH 2/2] treewide: lose F_SETPIPE_SZ page size assumptions Alyssa Ross 2025-01-07 22:22 ` [PATCH 1/2] t/lei-sigpipe.t: use correct pipe size Eric Wong 0 siblings, 2 replies; 5+ messages in thread From: Alyssa Ross @ 2025-01-07 17:46 UTC (permalink / raw) To: meta 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] treewide: lose F_SETPIPE_SZ page size assumptions 2025-01-07 17:46 [PATCH 1/2] t/lei-sigpipe.t: use correct pipe size Alyssa Ross @ 2025-01-07 17:46 ` Alyssa Ross 2025-01-07 22:26 ` Eric Wong 2025-01-07 22:22 ` [PATCH 1/2] t/lei-sigpipe.t: use correct pipe size Eric Wong 1 sibling, 1 reply; 5+ messages in thread From: Alyssa Ross @ 2025-01-07 17:46 UTC (permalink / raw) To: meta 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] treewide: lose F_SETPIPE_SZ page size assumptions 2025-01-07 17:46 ` [PATCH 2/2] treewide: lose F_SETPIPE_SZ page size assumptions Alyssa Ross @ 2025-01-07 22:26 ` Eric Wong 2025-01-08 4:51 ` Eric Wong 0 siblings, 1 reply; 5+ messages in thread From: Eric Wong @ 2025-01-07 22:26 UTC (permalink / raw) To: Alyssa Ross; +Cc: meta 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] treewide: lose F_SETPIPE_SZ page size assumptions 2025-01-07 22:26 ` Eric Wong @ 2025-01-08 4:51 ` Eric Wong 0 siblings, 0 replies; 5+ messages in thread From: Eric Wong @ 2025-01-08 4:51 UTC (permalink / raw) To: Alyssa Ross; +Cc: meta 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). ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] t/lei-sigpipe.t: use correct pipe size 2025-01-07 17:46 [PATCH 1/2] t/lei-sigpipe.t: use correct pipe size Alyssa Ross 2025-01-07 17:46 ` [PATCH 2/2] treewide: lose F_SETPIPE_SZ page size assumptions Alyssa Ross @ 2025-01-07 22:22 ` Eric Wong 1 sibling, 0 replies; 5+ messages in thread From: Eric Wong @ 2025-01-07 22:22 UTC (permalink / raw) To: Alyssa Ross; +Cc: meta 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; ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-01-08 4:51 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-01-07 17:46 [PATCH 1/2] t/lei-sigpipe.t: use correct pipe size Alyssa Ross 2025-01-07 17:46 ` [PATCH 2/2] treewide: lose F_SETPIPE_SZ page size assumptions Alyssa Ross 2025-01-07 22:26 ` Eric Wong 2025-01-08 4:51 ` Eric Wong 2025-01-07 22:22 ` [PATCH 1/2] t/lei-sigpipe.t: use correct pipe size Eric Wong
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).