> Date: Sat, 30 Jul 2022 18:45:39 +0300 > From: Vitaliy Makkoveev <[email protected]> > > On Wed, Jul 27, 2022 at 08:53:38PM +0200, Mark Kettenis wrote: > > > Date: Wed, 27 Jul 2022 00:11:19 +0200 > > > From: Alexander Bluhm <[email protected]> > > > > > > On Tue, Jul 26, 2022 at 11:19:27PM +0200, Mark Kettenis wrote: > > > > > Date: Tue, 26 Jul 2022 18:11:01 +0200 > > > > > From: Alexander Bluhm <[email protected]> > > > > > > > > > > On Fri, Jul 22, 2022 at 06:13:04PM +0200, Alexander Bluhm wrote: > > > > > > But I am fine with committing ifmedia, gem(4) and bge(4) now. Then > > > > > > we can decide on a per driver basis. As long kernel lock is not > > > > > > removed from the ifmedia layer, this diff is not strictly necessary. > > > > > > I want to commit it anyway to show how MP in ifmedia should work. > > > > > > > > > > This is the part for gem(4) and bge(4). I have tested them. > > > > > ifmedia_current() is MP safe and provides the data which all the > > > > > drivers need. > > > > > > > > > > After commiting this, I can look for more hardware to test. > > > > > > > > > > ok? > > > > > > > > I still don't understand what this fixes. Most drivers create a list > > > > of media when the PHY attaches during autoconf. That list never > > > > changes, which means that the pointer to the ifmedia struct remains > > > > valid until the driver detaches. > > > > > > > > There is of course a race when you use ifconfig to change the media > > > > type. But your new interface doesn't really fix that race. Code that > > > > uses ifmedia_current() may still end up with the old information and > > > > use that at a point where that information is no longer accurate. > > > > > > It does not fix a real bug. The diff tries to keep internal data > > > structures within a file, instead of spreading them over all drivers. > > > MP work is easier if each struct field has a clear lock that is > > > responsible for it. 70 drivers that should change it only during > > > attach is not that clear. > > > > But making data structures "mpsafe" is not a goal in itself. The way > > the data is used must be safe as well. If you don't think that > > through, you run the risk of adding mutexes that don't actually help. > > And this is starting to smell like one of those to me. > > > > > The ifmedia_list is protected by mutex. ifm_cur is a pointer to > > > an element of the list. It would be natural to protect it with the > > > same mutex. ifm_cur is changed by ifmedia_set() and ifmedia_ioctl() > > > and ifmedia_delete_instance(). > > > > > > Look at bnxt_hwrm_port_phy_qcfg(). There the list is changed by > > > bnxt_media_status() which is called from ifmedia_ioctl(). > > > ixgbe_handle_msf() changes the list during SPF interrupt. But I > > > have to admit that these drivers do not access ifm_cur. > > > > > > Why do you refuse to make the ifmedia layer MP safe by itself? > > > It is just one function and a mutex. I cannot see harm. > > > > I looked a bit at what NetBSD does. I don't think what they do is a > > good idea (in particular having pointers to locks). But they actually > > hold a lock across the callbacks. Which made me realize that if you > > don't do that you don't really fix the race between changes detected > > by the PHY and manual changes through ioctl. And I think the > > callbacks can sleep. So using a mutex may simply be wrong. > > > > > > NetBSD uses spin mutex by default for ifmedia protection. But it could > use driver's mutex for that purpose. That's why they have pointer to > lock. > > I don't like to keep lock while callback called. It introduces lock > inconsistency like we have with (*if_qstart)() callbacks. May be we > should pass the new media to the media change callback directly, like > below: > > if (ifm->new_ifm_change_cb) > error = (*ifm->new_ifm_change_cb)(ifm, ifp, match, > newmedia); > else { > oldentry = ifm->ifm_cur; > oldmedia = ifm->ifm_media; > /* ... */ > > error = (*ifm->ifm_change_cb)(ifp); > if (error && error != ENETRESET) { > mtx_enter(&ifmedia_mtx); > if (ifm->ifm_cur == match) { > ifm->ifm_cur = oldentry; > ifm->ifm_cur = oldentry; > } > mtx_leave(&ifmedia_mtx); > } > } > > This keeps the media change atomicy, and leaves `ifm' protection to the > driver, but without pointers to locks and callback calls with lock held.
I once more ask you guys. What problem are you trying to solve?
