timerfd: Define a locking regime

[jfree]

Summary

Define a locking regime for the members of struct timerfd and document
it so future code can follow the standard. The lock legend can be found
in a comment above struct timerfd.

Additionally,
* Add assertions based on locking regime.
* Fill kn_data with the expiration count when EVFILT_READ is triggered.
* Report st_ctim for stat(2).
* Check if file has f_type == DTYPE_TIMERFD before assigning timerfd
  pointer to f_data.

[kib]

sys/kern/sys_timerfd.c

288

This is a change in the behavior. Before, you returned bool, now you truncate tfd_count from uint64_t to int and return truncated value (which is wrong).

That said, there must be a recursion on tfd_lock after the patch.

307

Did you tested this? I am sure that you get a recursion on tfd_lock.

484

How can tfd be NULL there?

sys/sys/timerfd.h

56 ↗

(On Diff #126541)

The line struct thread; is enough there, instead of full header.

[jfree]

• Add recursion to tfd_lock mtx.
• Return boolean in filt_timerfdread().
• Don't check if tfd == NULL.
• Declare struct thread in timerfd.h instead of including sys/proc.h.

Thanks to @kib for pointing these out.

sys/kern/sys_timerfd.c

307

Did you tested this? I am sure that you get a recursion on tfd_lock.

I just created a timerfd test for kevent(). Yes, it recursed and panicked, but I just fixed it.

[kib]

Recursive locks is much harder to reason about and correctly use. In this case there is no sense in making the lock recursive, at worst you should add assertions instead of acquiring it more than needed.

[jfree]

In D41600#948035, @kib wrote:

Recursive locks is much harder to reason about and correctly use. In this case there is no sense in making the lock recursive, at worst you should add assertions instead of acquiring it more than needed.

I apologize for my ignorance. I had never taken the time to read the documentation for knotes, so I did not know if the recursion was inevitable. I just read the knlist_add(9) manual, and I see "Locks must not be acquired in f_event." Oops. Looks like the lock is acquired for us via the mtx kl_lock(). I've also updated the f_event to correctly return the number of events in tfd_count, as reading does.

[jfree]

• Do not initialize tfd_lock with MTX_RECURSE.
• Add assertion in filt_timerfdread(), showing that the tfd_lock is held.
• Place tfd_count in kn->kn_data.

[kib]

sys/kern/sys_timerfd.c

119–120

There should be a blank line after locals declaration block.

308

Why not keep using knlist_add(..., 0); there and save two lines of code? Pre-patch code is correct.

[jfree]

sys/kern/sys_timerfd.c

308

Why not keep using knlist_add(..., 0); there and save two lines of code? Pre-patch code is correct.

Locking before accessing tfd_sel.si_note follows the locking regime.
tfd_sel is protected by the tfd_lock.
tfd_sel.si_note may not change, but I wanted to uphold the locking convention anyway.

[jfree]

sys/kern/sys_timerfd.c

308

tfd_sel.si_note may not change, but I wanted to uphold the locking convention anyway.

Now that I look back at this, it is not very clear.
What I meant to say was: tfd_sel.si_note is a member of tfd_sel and tfd_sel is protected by tfd_lock. So we must acquire tfd_lock before accessing members of tfd_sel.

[kib]

sys/kern/sys_timerfd.c

308

tfd_sel existence (not content) is guaranteed by assignment at initialization. It is parallel modifications that create race, and if the mutator takes the lock, it provides adequate protection against the race. Taking the tfd lock earlier, before dereferencing tfd_sel, adds nothing except more code.

[jfree]

• Remove locking around knlist_add() and set islocked arg to 0
• Add space under declarations in timerfd_getboottime()

[jfree]

Include <sys/time.h> instead of <sys/timespec.h>. This causes intentional namespace pollution that mimics Linux.

g/musl libcs include <time.h> in their <sys/timerfd.h>, exposing timerfd_gettime() and CLOCK_ macro constants.

Ports like Chromium expect this namespace pollution and fail without it.

See bug report by @jbeich for more information:
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=273373

[imp]

This is a lot of unrelated (except being around timerfd) changes. Once they are sorted out as being correct, it would be nice if, say, the compat32 changes were separate from the locking changes etc

[jfree]

This patch was getting long, as @bsdimp pointed out. Split it into three separate patches. This patch will still define the locking regime.

[imp]

Seems ok. But it would be good to get the nod from a couple of others on the locking.