In D43399#1179757, @christos wrote:

Please rebase all relevant patches, including this one, on top of main, stack them properly, and also re-generate and update the diffs with full context: https://wiki.freebsd.org/Phabricator#Create_a_Revision_via_Web_Interface

The Makefile needs to be updated to install mmap.c.

Bump.

This is an initial quick review, as I haven't had a chance to properly test it yet.

In D53736#1227777, @kib wrote:

In D53736#1227776, @christos wrote:

In D53736#1227774, @kib wrote:

In D53736#1227757, @christos wrote:

In D53736#1227693, @kib wrote:

What is channel->pid? Where is it got assigned?

From the commit message:

channel->pid (as well as channel->comm) is always assigned in dsp_chn_alloc().

I will correct the message to say pcm_channel->pid, which is the actual struct's name.

This is not what I am asking about. Why do you need to save the pid, and how do you use it?

Both the pid and process name are mostly cosmetic, and are used in sndctl(8), /dev/sndstat and in some OSSv4 structures. It is just so that the user can easily tell which channel(s) belong to which processes.

So for instance, sharing the open file after fork, or passing it over unix domain socket, cause only a cosmetic issues, right?

In D53736#1227774, @kib wrote:

In D53736#1227757, @christos wrote:

In D53736#1227693, @kib wrote:

What is channel->pid? Where is it got assigned?

From the commit message:

channel->pid (as well as channel->comm) is always assigned in dsp_chn_alloc().

I will correct the message to say pcm_channel->pid, which is the actual struct's name.

This is not what I am asking about. Why do you need to save the pid, and how do you use it?

In D53736#1227693, @kib wrote:

What is channel->pid? Where is it got assigned?

In D53666#1226867, @kib wrote:

In D53666#1226866, @christos wrote:

2. Calling close(2) on the device: I'm not sure about this one. Is it possible that close(2) and read(2)/write(2) can run concurrently?

Yes, close(2) and io syscalls can be run in parallel. But the file is only closed (as in d_close for devfs, or cdevpriv destroy) when the file ref count goes to zero. Since both read(2) and close(2) take the reference, effectively the file deallocation occurs by the syscall that fdrop() it the last.

In D53666#1226813, @kib wrote:

In D53666#1226739, @christos wrote:

The buffer is allocated with with M_ZERO and a specific size by the device drivers when a channel is created.

Ok. Can channel deallocated while unlocked?

In D53666#1226622, @kib wrote:

In D53666#1226613, @christos wrote:

In D53666#1226612, @kib wrote:

In D53666#1226605, @christos wrote:

In D53666#1226600, @kib wrote:

I have no knowledge of the dev/sound code. After the channel unlock, what guarantees the validity of the buffer that uiomove() operates on?

What do you mean by validity?

I mean that uiomove() operates on kernel memory that is supposed to be in correct state for it.

As I said, I do not know the dev/sound code. But your note about parallel thread manipulating the channel after unlock (which is expectedly allowed) raises the question what manipulations can be done while uiomove() needs to access the channel buffer in parallel.

It doesn't really matter, whatever we write here will get processed further down the line by sound(4), and eventually go to the sound card's output. We just need to ensure that writes (and reads) happen at the correct buffer locations. Also, the parallel writing scenario I explain is really only a consideration in testing environments, as was the case now with stress2. In real world use-cases, usually a channel is operated on by a single thread.

It doesn't matter what happens usually, since we discussing the correctness of the kernel code. For instance, could this result in reading of uninitialized kernel memory (exposing its previous content)?

In D53666#1226612, @kib wrote:

In D53666#1226605, @christos wrote:

In D53666#1226600, @kib wrote:

I have no knowledge of the dev/sound code. After the channel unlock, what guarantees the validity of the buffer that uiomove() operates on?

What do you mean by validity?

I mean that uiomove() operates on kernel memory that is supposed to be in correct state for it.

As I said, I do not know the dev/sound code. But your note about parallel thread manipulating the channel after unlock (which is expectedly allowed) raises the question what manipulations can be done while uiomove() needs to access the channel buffer in parallel.

In D53666#1226600, @kib wrote:

I have no knowledge of the dev/sound code. After the channel unlock, what guarantees the validity of the buffer that uiomove() operates on?

Address kib's comments.

Clean up. Finalized on my part.

In D53666#1226459, @pho wrote:

In D53666#1226326, @christos wrote:

Do what I explained in the last message. stress2 has finished after 30 minutes
without panicking. I've yet to test chn_read() though.

@pho, can you also test please?

I ran the write(2) tests for 30 minutes followed by some corresponding read(2) tests.
LGTM

In D53353#1226519, @meka_tilda.center wrote:

I tested simple, select and poll on the real hardware. As that machine is not running CURRENT, I can't test kqueue. @christos, do you have any means on testing it on real hardware? I tried to test it in bhyve with -s 6,hda,play=/dev/dsp,rec=/dev/dsp but none of the examples work properly that way.

Do what I explained in the last message. stress2 has finished after 30 minutes
without panicking. I've yet to test chn_read() though.

After some pen and paper debugging, it seems that my current approach does solve most cases, but there is still one case that can reproduce the panic, albeit quite rare. You need to run stress2 usually for about ~20 minutes with enough workload to hit the race. Suppose the following scenario: the buffer has a size of 4 bytes, and we have two threads: (A) and (B).
In the first iteration, thread (A) wants to write 2 bytes, while the buffer is empty, BUT the pointer (sndbuf_getfreeptr()) is at the end (i.e., buf[3]). In the first iteration of the loop, because of the way we calculate t, we'll end up writing only 1 byte, so after sz -= t, sz will be 1, and so we'll need one more iteration in the inner loop, to write the remaining 1 byte. Now we're at the end of the first loop, thread (A) unlocks the channel, it has written 1 byte, it needs to write 1 more, and the buffer is left with 3 empty slots. Now thread (B) picks up the lock, and it wants to write 3 (or more) bytes. Eventually it writes the 3 bytes, and it leaves the buffer with 0 free slots. By the time thread (A) picks up the lock again, and continues with the second iteration of the inner loop, it will try to write the last byte, but sndbuf_acquire() will panic because there is no free space anymore. I'm pretty sure this is what is going on.

In D53353#1226314, @meka_tilda.center wrote:

I already fixed those as well as share/examples/Makefile, I just want to check if everything is working, and that has to wait for my day job to finish.

I can actually fix those last comments myself, no need to waste your time for this. Just let me know if tests worked fine on your machine. :-)

All of my inline comments are mostly grammar/typo fixes. If you tested the most recent code (especially simple.c) with the most recent changes, then we'll be good to go. I'll commit this once you address the last comments. Thanks a lot for this! :-)