Page MenuHomeFreeBSD

Create wrapper for Giant taken for newbus
Needs ReviewPublic

Authored by imp on Sep 4 2021, 4:36 AM.

Details

Reviewers
shurd
jhb
mav
hselasky
Group Reviewers
iflib
Summary

Create an API to take and drop the topo lock needed to manage
the newbus tree (bus_topo_lock() and bus_topo_unlock()). Add
bus_topo_mtx() to return this mutext (current Giant but soon to
be an SX lock I think).

Sponsored by: Netflix

Test Plan

This needs a man page.
Asserts will be a separate commit
This may have been posted as a review before, but I can't find it. I know hps hated the name newbus_lock()/unlock() so I've changed it.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 41384
Build 38273: arc lint + arc unit

Event Timeline

imp requested review of this revision.Sep 4 2021, 4:36 AM
imp retitled this revision from Push Giant lock down inside of mii_attach to Create wrapper for Giant taken for newbus.Sep 4 2021, 4:42 AM
imp edited the summary of this revision. (Show Details)
imp edited the test plan for this revision. (Show Details)
imp added reviewers: jhb, mav, hselasky. imp removed 1 blocking reviewer(s): iflib.

Use bus_topo_lock()/unlock() instead of giant in devctl2

imp removed 1 blocking reviewer(s): iflib.

Patch looks good to me. The only comment I have is the function name?

Why not just call it:

bus_top_lock();

The four letters "topo" doesn't sound that good to me. Not that it matters.

--HPS

Patch looks good to me. The only comment I have is the function name?

Why not just call it:

bus_top_lock();

The four letters "topo" doesn't sound that good to me. Not that it matters.

"topo" is the typical abbreviation for topology. Geom uses g_topology_lock() etc, but I thought that too long. I'd rather do topology than top since top means something a little different.

I am mostly unfamiliar with miibus interface, but I see ~80 calls to mii_attach() in a tree, many of which are going from driver attach methods and are already under the lock. Are you expecting to recurse on the lock?

In D31831#718073, @mav wrote:

I am mostly unfamiliar with miibus interface, but I see ~80 calls to mii_attach() in a tree, many of which are going from driver attach methods and are already under the lock. Are you expecting to recurse on the lock?

Yea, that's one of the reasons why I need to separate out that change.... I convinced myself it was OK when I originally did it last year, but I need to study the code again I think to articulate the reason. With giant it doesn't matter, but it may in the future...

remove miibus pushdown change that arrived I think via a rebase mistake when
updating earlier in the summer.

imp edited the test plan for this revision. (Show Details)

Add some notes for the reviewers that maybe don't belong in comments

sys/dev/mfi/mfi.c
1930

This could move up (we don't need to hold giant past the end of add_child(), but I'm just retaining the same code structure as before.

2018

This could move up (we don't need to hold giant past the end of add_child(), but I'm just retaining the same code structure as before.

sys/dev/mlx5/mlx5_core/mlx5_health.c
377

I'm unsure what newbus interaction is here that this protects... @jhb ?

sys/dev/pci/pci_user.c
1062

I'm not sure what this is actually protecting here... Or was protecting in the past... We don't change topologies here, so shouldn't need it for everything, though I may have overlooked something in an individual ioctl call...

sys/dev/usb/net/if_ure.c
1025

I narrowed things here because only mii_attach needs this topo lock. We don't need it to add media types to if_media.
I'm also sure this path will be executed during the rollout of these changes, so I won't leave a hidden bomb (though there's some risk I'll have to fix something after we commit)

In D31831#718033, @imp wrote:

The four letters "topo" doesn't sound that good to me. Not that it matters.

"topo" is the typical abbreviation for topology. Geom uses g_topology_lock() etc, but I thought that too long. I'd rather do topology than top since top means something a little different.

I suggest you spell it out then, bus_topology_lock() is better than bus_topo_lock(), because "topo" means "top" in Portuguese, and in Spanish it means something completely different, and you already pointed out that "top" is not what you want to describe?

--HPS

Hi Warner,

We don't want to have another "minion" discussion, look at this:

In Italian "topo" means "mouse".
In Spanish "topo" means "mole".
In Portugese "topo" means "top".
In English "topo" means "a topographic map".

--HPS

Hans, I think you are too picky here. We already have xpt_topo_lock in CAM, ng_topo_lock in NetGraph, and 4 more line that in random drivers, so I think this abbreviation is quite typical.

Hi Warner,

We don't want to have another "minion" discussion, look at this:

In Italian "topo" means "mouse".
In Spanish "topo" means "mole".
In Portugese "topo" means "top".
In English "topo" means "a topographic map".

I think this may be different. We have the following:

smp already has topo_nodes to map out the system topography, with lots of machdep code that uses the _topo_ names in various places.
CAM already has a xpt_topo_lock to control changes to the device lists
We have clock, regulator, syscon, phy and other embedded code that uses a topology lock is named *_topo with similar _lock, _slock, _unlock, etc
The mpr and mps driver's code to map out the nodes on the bus uses 'topo_change' and other variations on 'topo'
The fiberchanel code (osc_fc) has 'read_topo' used with the SLI4_MBOX_COMMAND_READ_TOPOLOGY command

So unlike minion, which didn't have an established use, this just follows the patter that exists across a range of kernel subsystems.
On the other side, geom, netgraph and bits of VMM and VM spell out topology, the former two for their topo locks, the latter two
only for info routines.

So there's already a preference for topo_ in this circumstance, imho, but I'm keeping an open mind for the moment.

imp edited the test plan for this revision. (Show Details)

Make a lock/unlock of Giant in cardbus bus_topo_lock()/unlock()

In D31831#718192, @mav wrote:

Hans, I think you are too picky here. We already have xpt_topo_lock in CAM, ng_topo_lock in NetGraph, and 4 more line that in random drivers, so I think this abbreviation is quite typical.

OK, maybe I need to extend my vocabulary :-)

You could also add the explicit topology locking into sysctl_hdac_pindump() and hdaa_sysctl_reconfig(), and remove CTLFLAG_NEEDGIANT from all 3 places in hda(4) driver. Or I can do it separately after, if you prefer.