Page MenuHomeFreeBSD

if: Remove ifnet_rwlock
ClosedPublic

Authored by kp on Nov 19 2020, 1:53 PM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 11 2024, 6:34 PM
Unknown Object (File)
Feb 11 2024, 6:30 PM
Unknown Object (File)
Feb 11 2024, 1:43 AM
Unknown Object (File)
Dec 22 2023, 11:18 PM
Unknown Object (File)
Dec 14 2023, 10:29 PM
Unknown Object (File)
Dec 11 2023, 12:17 AM
Unknown Object (File)
Nov 25 2023, 3:31 AM
Unknown Object (File)
Nov 22 2023, 1:57 PM

Details

Summary

It no longer serves any purpose, as evidenced by the fact that we never
take it without ifnet_sxlock.

Sponsored by: Modirum MDPay

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kp requested review of this revision.Nov 19 2020, 1:53 PM
kevans added subscribers: glebius, kevans.

I agree with your assessment, rS334118 converted the IFNET_RLOCK_NOSLEEP* macros to using epoch instead of the rwlock and were subsequently removed in a cleanup by @glebius in rS342872 -- this looks like a small oversight that the lock wasn't removed entirely at that point.

This revision is now accepted and ready to land.Nov 19 2020, 3:43 PM

I'd like to be able to MFC D27279. I didn't expect it to rely on this patch, and I suspect this one can't be safely merged to stable/12 because it might break kernel ABI compatibility.

I wonder if we should MFC only the IFNET_WLOCK, IFNET_WUNLOCK and IFNET_WLOCK_ASSERT changes, leaving the ifnet_rwlock struct around for any out-of-tree user that may want to take it.

In D27278#609380, @kp wrote:

I wonder if we should MFC only the IFNET_WLOCK, IFNET_WUNLOCK and IFNET_WLOCK_ASSERT changes, leaving the ifnet_rwlock struct around for any out-of-tree user that may want to take it.

Unfortunately I'm not sure that you can MFC this, but hopefully someone else will chime in -- it's perhaps unlikely, but if we call into any out-of-tree module that wants to IFNET_WLOCK_ASSERT(), then we must have grabbed it.

That said, I'm not immediately seeing a path into if_vmove() from an external driver where it could have possibly been holding the wlock. Both callers that aren't vnet_if_return will take the allprison_lock immediately. You could probably get away with MFC'ing the other one and just dropping the rwlock if it's held around if_reassign() then picking it back up after for stable/12. The lock order here seems to be in your favor.

sys/net/if_var.h
607 ↗(On Diff #79754)

This comment should just go away entirely, probably... you only need to know whether it's for read or write now.

This revision now requires review to proceed.Nov 24 2020, 9:56 AM
kp marked an inline comment as done.Nov 24 2020, 9:56 AM

Remove now useless do {} while(0) constructs.

This revision was not accepted when it landed; it landed in state Needs Review.Nov 25 2020, 10:57 AM
Closed by commit rS368015: if: Remove ifnet_rwlock (authored by kp). · Explain Why
This revision was automatically updated to reflect the committed changes.

How about the lock order in subr_witness.c for IPv4 and Ipv6 multicast. Is it still required in the lock order?

How about the lock order in subr_witness.c for IPv4 and Ipv6 multicast. Is it still required in the lock order?

Good point. See https://reviews.freebsd.org/D31585.