From what I can see, now, we initialize the mutex only if sc_midi_chan.valid is true, but we will destroy it unconditionally, so we might destroy an uninitialized mutex.

The commit log message should be a bit more explicit. Describing the implementation as "branchless" makes it sound like you are micro-optimizing the code to avoid branches, but really this change is just factoring out the backend of the driver into separate functions. I presume this is to make the code easier to read and modify.

Add an isfd check

In D55789#1276669, @jfree wrote:

In D55789#1276630, @markj wrote:

A bit tangential, but: we found a while back that it was possible to trivially livelock a core by scheduling timeouts of 1ns using EVFILT_TIMER or setitimer(). Basically, it takes longer than the callout interval to execute the callout handler, so the callout handler ends up running in a loop.

I should note that the current timerfd implementation fails to pass the timerfd__periodic_timer_performance test, which expects 400000000 individual nanosecond timeouts. I usually see 360000000 - 380000000 timeouts printed by the test case. I'm not sure if this performance issue is related to the core livelock in some way. The callout routine (timerfd_expire()) is fairly lightweight, specifying absolute expiration time with maximum precision (no C_PREL() or pr specified).

In D55670#1276909, @christos wrote:

In D55670#1276620, @markj wrote:

In D55670#1276311, @christos wrote:

I'm thinking about this again and I'm suspecting this will make things very inflexible. I think that instead of patching virtual_oss, it might be better to have an environment variable that specifies the control device name, and let devd use that. @markj What do you think?

As I understand it, the basic problem is that virtual_oss is providing a virtual audio device, and there's an associated control device, and there is no mechanism to figure out the control device name from the audio device name, aside from looking at whatever's configured in rc.conf.

Can virtual_oss create files under /var/run which provide this mapping? Similar to a pidfile, it could create /var/run/virtual_oss.dsp at startup, and the file would contain the path to the control device. Then snd.conf would contain something like action "/usr/sbin/virtual_oss_cmd $(cat /var/run/virtual_oss.${cdev}) -R /dev/${cdev}". I'm pretty sure devd can perform substitutions like that, the devd.conf man page has an example of it.

The thing is that if we want to be flexible, we cannot know which control device the user wants to use. Perhaps the user wants to use a control device for a virtual_oss instance that does not replace /dev/dsp, hence why an environment variable that is easily modifiable might be simpler. Not sure yet.

Is this covered by the timerfd tests you posted already?

A bit tangential, but: we found a while back that it was possible to trivially livelock a core by scheduling timeouts of 1ns using EVFILT_TIMER or setitimer(). Basically, it takes longer than the callout interval to execute the callout handler, so the callout handler ends up running in a loop.

I think the tests are ok, though as before the errno checking doesn't seem very useful. I'm also not sure about this leak detection stuff, I would probably inline it into the timerfd.c tests if we're going to keep it. With ATF each test runs in its own process so fd leaks aren't really a problem, and many existing test cases are not careful to close fds.

In D55670#1276311, @christos wrote:

I'm thinking about this again and I'm suspecting this will make things very inflexible. I think that instead of patching virtual_oss, it might be better to have an environment variable that specifies the control device name, and let devd use that. @markj What do you think?

It's hard for me to understand what this change does, or the tradeoffs involved, without a commit log message.

I think this is fine. It's a bit of layering violation since opensolaris is supposed to be able to serve multiple consumers (dtrace and ZFS in the past, just dtrace now). I'm in favour of converting assertion and print function calls in dtrace to just using FreeBSD interfaces directly.

Looks ok to me.

In D55661#1274834, @glebius wrote:

I don't see a point in clearing the addresses either. Speculatively (didn't check!) it should even fix getsockname(2) on a disconnected TCP connection.

This removes the smr_advance() calls, but also the clearing of the addr/port. Is it intentional?

In D55536#1274178, @ashafer wrote:

I only see the "upgrading to write fault" for many processes. Tested with both xfce and plasma. Seems like all the apps that initiate graphics get reported by the message (xfwm4, firefox, plasmashell, Xorg, etc)

In D55536#1271983, @alc wrote:

@markj Do we know if there is actually a mix of clean and dirty pages being mapped?

Add some prints

In D55539#1272091, @arrowd wrote:

Is my understanding correct that it is not supposed to work with ZFS yet?

Rather than preemptively dirtying pages, make the map read-only unless all pages are already dirty.

Looks ok with one nit fixed.

Does it actually make a difference in generated code? I would expect a compiler to be able to do this transformation.

Thank you.

https://reviews.freebsd.org/D55536 is my attempt at a solution.

In D55525#1270754, @markj wrote:

vm_fault_soft_fast() is careful to create a writable superpage mapping only if all constituent pages are dirty. In that case it will preset PG_M by setting fs->fault_type |= VM_PROT_WRITE. More generally, there is an invariant that writable superpage mappings are dirty, which is what the assertion is complaining about.

vm_fault_populate() does not preset VM_PROT_WRITE, even though it marks all constituent pages dirty at the MI layer (though I'm not 100% sure why it does that unconditionally... what if the mapping is read-only?). I think this is an oversight, so this might be a better solution:

vm_fault_soft_fast() is careful to create a writable superpage mapping only if all constituent pages are dirty. In that case it will preset PG_M by setting fs->fault_type |= VM_PROT_WRITE. More generally, there is an invariant that writable superpage mappings are dirty, which is what the assertion is complaining about.

In D55399#1270409, @des wrote:

In D55399#1270387, @markj wrote:

I'm not sure about a 3 day MFC timeout, but I'm also not sure that any significant number of people are using this code on main.

I assumed that we'd want to merge it quickly to 15 and especially 14...

I'm not sure about a 3 day MFC timeout, but I'm also not sure that any significant number of people are using this code on main.

Why exactly do you use a device interface rather than a sysctl? Is it just to make the functionality available to non-root users (i.e., members of the operator group)?