pipe: try to skip locking the pipe if a non-blocking fd is used

[mjg]

Summary

make -j 104 tinderbox has over half of the calls to read find an empty pipe. As the pipe is shared, this contends a lot.

commit d7cfa63d1de31e176e50562cdb57efa596792d93 (HEAD, mjgzoo/pipenb)
Author: Mateusz Guzik <[e-mail]>
Date:   Mon Aug 8 18:02:15 2022 +0000

    pipe: try to skip locking the pipe if a non-blocking fd is used
    
    Reviewed by:
    Differential Revision:

commit a5ee622427f86017939bc9ef47e5a7c804072825
Author: Mateusz Guzik <[e-mail]>
Date:   Mon Aug 8 19:14:09 2022 +0000

    mac: cheaper check for mac_pipe_check_read
    
    Reviewed by:
    Differential Revision:

[kib]

sys/kern/sys_pipe.c

732 ↗

(On Diff #109019)

(fp->f_flag & FNONBLOCK) != 0

733 ↗

(On Diff #109019)

If this check is worth it, why it is done under FNONBLOCK/mac predicates? It should be performed unconditionally, it seems.

735 ↗

(On Diff #109019)

!= 0

sys/security/mac/mac_framework.h

270

Why is it defined as 0 and not as false?

[markj]

sys/kern/sys_pipe.c

726 ↗

(On Diff #109019)

730 ↗

(On Diff #109019)

This comment doesn't make much sense to someone just reading the code: what is being avoided? Doesn't the lockless check avoid the contention?

I'd suggest explaining it at a higher level: some programs use pipes to distribute jobs to workers, so polling can induce lock contention if we don't check the buffer here.

735 ↗

(On Diff #109019)

In the locked path, the reader might clear PIPE_WANTW. Why is it not necessary to handle that here?

737 ↗

(On Diff #109019)

Please use atomic_load_* for these unlocked loads.

[mjg]

sys/kern/sys_pipe.c

735 ↗

(On Diff #109019)

it should be an invariant that if the pipe is empty someone can write to it

737 ↗

(On Diff #109019)

note atomic access on only one end is not really a full solution.

perhaps it is time to introduce atomic_int_t and other types.

[mjg]

sys/kern/sys_pipe.c

733 ↗

(On Diff #109019)

there is some processing done right now even when literally nothing got read. I do think it is questionable, but I don't want to remove it in this patch. I consider it fine for the non-blocking case which just takes a peek.

[mjg]

sys/kern/sys_pipe.c

733 ↗

(On Diff #109019)

also it's this check is not an optimization, it is needed to return the right error code -- 0 instead of EAGAIN

[markj]

sys/kern/sys_pipe.c

730 ↗

(On Diff #109203)

Missing something after "over".

737 ↗

(On Diff #109203)

This should be atomic_load too.

737 ↗

(On Diff #109019)

Indeed, atomic_load does not solve a problem per se, it's just a hint to someone reading the code.

To make KCSAN avoid reporting false positives, the stores need to be atomic-qualified too.

[mjg]

• fix 32-bit build
• tidy up comment
• atomic for pipe state

[markj]

sys/kern/sys_pipe.c

744 ↗

(On Diff #109423)

We could use u_int for cnt in struct pipemapping. pipe_build_write_buffer() and pipe_clone_write_buffer() use an int already. Then there's no need for this ugly ifdef.