Re: [PATCH 0/2] pipe: Fixes [ver #2]

[Linus Torvalds]

On Thu, Dec 12, 2019 at 2:18 AM Konstantin Khlebnikov
<khlebnikov@xxxxxxxxxxxxxx> wrote:
>
> commit f467a6a66419 pipe: fix and clarify pipe read wakeup logic
> killed "wake writer when buffer becomes half empty" part added by
> commit cefa80ced57a ("pipe: Increase the writer-wakeup threshold to reduce context-switch count").
>
> I suppose that was unintentional. Jobserver juggles with few bytes and
> should never reach half/full buffer thresholds.

It wasn'tunintentional - the rest of the cleanups should mean that we
never wake things up unnecessarily - we now only wake up if it _used_
to be 100% full, and we only do it once per read.

And it did it without the watermark, which I was worried about might
break something that "knew" the size of the pipe.

But performance testing would be good. Both for the "no unnecessary
wakeups" case, but also for the thundering herd issue.

To answer Josh's email in this same thread:

On Wed, Dec 18, 2019 at 12:59 PM Josh Triplett <josh@xxxxxxxxxxxxxxxx> wrote:
>
> Debian and Ubuntu have make 4.2.1-1.2, which includes "[SV 51159] Use a
> non-blocking read with pselect to avoid hangs." and various other fixes.
> https://metadata.ftp-master.debian.org/changelogs/main/m/make-dfsg/make-dfsg_4.2.1-1.2_changelog
> So, both Debian and Ubuntu should be fine with the pipe improvements.
> (I'm testing that now.)
>
> Is the version of your non-thundering-herd pipe wakeup patch attached to
> https://lore.kernel.org/lkml/CAHk-=wicgTacrHUJmSBbW9MYAdMPdrXzULPNqQ3G7+HkLeNf1Q@xxxxxxxxxxxxxx/
> still the best version to test performance with?

That's my latest version, but you'll have to tweak it a tiny bit
because of d1c6a2aa02af ("pipe: simplify signal handling in
pipe_read() and add comments") which I did after that patch.

The easiest way to resolve it is likely to revert that d1c6a2aa02af,
then apply the non-thundering-herd patch and then apply d1c6a2aa02af
again by hand - it's fairly straightforward (and you can return
-ERESTARTSYS directly if wait_event_interruptible_exclusive() fails,
because of all the same reasons why it coul dhappen without the
thundering-herd patch.

I can look at re-creating that patch if  you find it to be too annoying.

               Linus