sx: Add `sx_has_waiters()` macro

[dumbbell]

Summary

This macro will return non-zero if there are threads waiting for this lock; otherwise, it will return zero.

The function assumes (but does not assert) that the caller already holds the lock and that it is interested in other threads waiting for it to release the lock.

The motivation to add this is the implementation of rwsem_is_contended() in linuxkpi.

This Linux function indicates the same thing to the caller: if other threads are waiting for this semaphore.

The amdgpu DRM driver started to use rwsem_is_contended() in Linux 6.12.

This is part of the update of DRM drivers to Linux 6.12.

Sponsored by: The FreeBSD Foundation

[dumbbell]

Uploaded the wrong patch before.

[bz]

sys/compat/linuxkpi/common/include/linux/rwsem.h

45

The should use linuxkpi_ not linux_ given we are changing things anyway already.

sys/compat/linuxkpi/common/src/linux_rwsem.c

10 ↗

(On Diff #175708)

This is copyright someone else in theory; 684bcfec8994ed608775ecd3f848406253be60b1 and before that in ofed aa0a1e58f0189.

@emaste?

22 ↗

(On Diff #175708)

So this is done w/o any lock; can we rely on ++ / -- for this here and in down_read?

[dumbbell]

Address comments from @bz

[dumbbell]

sys/compat/linuxkpi/common/src/linux_rwsem.c

22 ↗

(On Diff #175708)

I’m not 100% confident myself. Should I use atomic operations here?

[olce]

I'd suggest instead to give up on this new implementation as sx locks can directly provide this information, e.g., you can implement rwsem_is_contended() simply as:

static inline int
rwsem_is_contended(struct rw_semaphore *sem)
{
	return (SX_READ_VALUE(&sem->sx) & SX_LOCK_WAITERS);
}

(perhaps adding !! if Linux consumers expect 0 or 1)

sys/compat/linuxkpi/common/src/linux_rwsem.c

22 ↗

(On Diff #175708)

If you keep that counter, you must, but see the overall alternative suggestion in the herald comment.

[bz]

In D56443#1298661, @olce wrote:

I'd suggest instead to give up on this new implementation as sx locks can directly provide this information, e.g., you can implement rwsem_is_contended() simply as:

static inline int
rwsem_is_contended(struct rw_semaphore *sem)
{
	return (SX_READ_VALUE(&sem->sx) & SX_LOCK_WAITERS);
}

(perhaps adding !! if Linux consumers expect 0 or 1)

I would highly advise against this. Don't start splattering lock internals without KPI over the kernel. Seems not a good idea to me.

The SX_READ_VALUE define should likely be moved to kern_sx.c, its only consumer so far--or at least seen as such. If you really want this, add a tiny (inline) wrapper to the sx implementation and then use that as a proper KPI from here in LinuxKPI.

According to style the check is not !! but != 0

[olce]

In D56443#1298756, @bz wrote:

I would highly advise against this. Don't start splattering lock internals without KPI over the kernel. Seems not a good idea to me.

Sorry if that wasn't clear, but that reaction is completely missing my point, which is that sx(9) locks can readily provide the information of whether there are waiters, and there's no need to graft a machinery just for that purpose on top of it. I stand firm by it.

The snippet was just to illustrate how to implement retrieving the information (hence the e.g.). It goes without saying that sx internals should not be leaked, and that the best place for this code is in fact a sx(9) routine. As I was in a hurry when commenting, I just didn't make it explicit, but I'm pretty sure this point is obvious to everyone here.

The SX_READ_VALUE define should likely be moved to kern_sx.c, its only consumer so far--or at least seen as such.

Even kern_sx.c does not use SX_READ_VALUE() everywhere it should (sc_lock direct accesses), and conversely internals of struct sx, including sc_lock, will stay exposed to the outside to allow for some inline implementations, regardless of whether SX_READ_VALUE() is hidden, and they are currently in use by LinuxKPI and ZFS. So moving SX_READ_VALUE() is not nearly enough.

That said, I agree this kind of leakage is a sorry state of affairs that should be fixed going forward.

If you really want this, add a tiny (inline) wrapper to the sx implementation and then use that as a proper KPI from here in LinuxKPI.

There's not the slightest doubt that leveraging sx(9) information is the best way.

According to style the check is not !! but != 0

Yes. !! is indeed a Linuxism (also used elsewhere).

[bz]

In D56443#1298877, @olce wrote:

In D56443#1298756, @bz wrote:

I would highly advise against this. Don't start splattering lock internals without KPI over the kernel. Seems not a good idea to me.

Sorry if that wasn't clear, but that reaction is completely missing my point, which is that sx(9) locks can readily provide the information of whether there are waiters, and there's no need to graft a machinery just for that purpose on top of it. I stand firm by it.

The snippet was just to illustrate how to implement retrieving the information (hence the e.g.). It goes without saying that sx internals should not be leaked, and that the best place for this code is in fact a sx(9) routine. As I was in a hurry when commenting, I just didn't make it explicit, but I'm pretty sure this point is obvious to everyone here.

The SX_READ_VALUE define should likely be moved to kern_sx.c, its only consumer so far--or at least seen as such.

Even kern_sx.c does not use SX_READ_VALUE() everywhere it should (sc_lock direct accesses), and conversely internals of struct sx, including sc_lock, will stay exposed to the outside to allow for some inline implementations, regardless of whether SX_READ_VALUE() is hidden, and they are currently in use by LinuxKPI and ZFS. So moving SX_READ_VALUE() is not nearly enough.

That said, I agree this kind of leakage is a sorry state of affairs that should be fixed going forward.

If you really want this, add a tiny (inline) wrapper to the sx implementation and then use that as a proper KPI from here in LinuxKPI.

There's not the slightest doubt that leveraging sx(9) information is the best way.

According to style the check is not !! but != 0

Yes. !! is indeed a Linuxism (also used elsewhere).

Sorry for the firm comment; I believe we agree on the overall solution. Thank you for the more detailed explanation!

[dumbbell]

Reimplement the patch, based on the suggestions from @olce.

[dumbbell]

Thank you for the great suggestion! I implement it and just uploaded the patch.

[dumbbell]

Remove unwanted unrelated change to the sx.9 manpage

[emaste]

Ah yes, this is nicer.

[markj]

sys/sys/sx.h

262

[dumbbell]

FIx typo reported by @markj

[dumbbell]

sys/sys/sx.h

262

Fixed, thanks!

[bz]

share/man/man9/sx.9

27

Please update .Dd before pushing.