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

(fp->f_flag & FNONBLOCK) != 0

733

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

735

!= 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

730

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

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

737

Please use atomic_load_* for these unlocked loads.

[mjg]

sys/kern/sys_pipe.c

735

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

737

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

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

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

Missing something after "over".

737

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.

737

This should be atomic_load too.

[mjg]

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

[markj]

sys/kern/sys_pipe.c

744

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.