newbus integration of MOD_QUIESCE

[M. Warner Losh]

In message: <12316.1089829525 at critter.freebsd.dk>
            "Poul-Henning Kamp" <phk at phk.freebsd.dk> writes:
: In message <20040714.090222.36824783.imp at bsdimp.com>, "M. Warner Losh" writes:
: 
: >: More like "If you are idle, prepare to be unloaded, otherwise yell".
: >
: >OK.  So there is a race here.  The modules have to cope with the lag
: >between the MOD_QUIESCE call and the MOD_UNLOAD that will come later.
: >Can the modules assume that if they get a MOD_QUIESCE that they WILL
: >get a MOD_UNLOAD later if they return 0?
: 
: Yes, they can assume that.  There is the footnote that we can have
: multiple modules in one kld and if one of them vetos the MOD_MODQUIESCE
: the MOD_UNLOAD may not happen until the user tries to kldunload again
: or at all.

We should document that they should.

: Notice that MOD_UNLOAD can also be veto'ed and that we already have
: this multi-modules in one KLD issue if one of the modules veto
: MOD_UNLOAD and the others don't.

True.  But a failure of one of them doesn't cause us to call
MOD_QUIESCE and then fail to call MOD_UNLOAD when it is zero.

: >That's a fairly high standard to live up to.  It would mean setting a
: >flag and not accepting any new business.
: 
: I'm not sure I find it a particular high standard to add code to:
: 
: 	case MOD_QUIESCE:
: 		...
: 		if (error == 0)
: 			sorry_we_re_close = 1;
: 		return (error)
: 
: and then check the flag in the probe/attach/taste/config routines.

Every single routine will have an extra check?  and what happens if
the sorry_we_re_closed is set after the check, but before the routine
returns?  It is unclear how to do this efficiently.  Do I have to lock
this somehow?  Does this force a global lock for each driver?  It
sounds very simple, but I think that closing the races here might be
hard.

: >: Finegrained guidance for what a module should do in MOD_QUISCE:
: >: 
: >:     Network driver:
: >: 
: >: 	If any instance is IF_UP, return EBUSY.
: >: 	Otherwise detach all instances and do not attach to
: >: 	any new instances offered.
: >
: >This sounds a lot like what we call detach in newbus.  Yet this is
: >different than what we'd talked about in terms of the
: >'notify/shutdown' split that we've been designing.  This makes it
: >harder to do that.
: 
: (I should clarify here that the detachment doesn't have to happen
: until MOD_UNLOAD, the important bit is determining if EBUSY should
: be returned and if not, prevent new business.)

True.  newbus can mark the newbus data structures 

: >I don't like this guidance.  It doesn't map to
: >anything in newbus that we have or we're planning.
: 
: MOD_QUIESCE is designed for kldunload -f, not for newbus so this
: is hardly surprising.

Right.  However, it was designed without making sure that newbus could
do sensible things with it, but I don't want to get into that snarling
match right now.

: >So this would also act as a gate to all further probe routines
: >returning 'good' as well?  That's a lot of side effects.
: 
: You lost me there.  Once MOD_QUISCE returned zero, all probe
: routines in the module should fail to recognize things.  I
: would expectat that to mean that they return 'bad', but this
: may just be a terminology thing.

We're saying the same thing: probe in the future should fail.

: >This makes it hard to map into other plans we have for newbus, and
: >hard for us to provide a hook into newbus drivers for this module
: >message.
: 
: I hate to be so blunt about this, but why is it that you think you
: need to do that ?

Leave the blunt objects at home.  I want to do this because I think it
is the right thing to do.  we have to tell the drivers to quiesce, and
it is highly desirable to have one type of quiesce rather than many
different flavors that are hard to get right.

: Let me explain why I can't see the connection:
: 
: Imagine that we have a pretty hairy driver which attaches to things
: on ISA, PCI and S-BUS.
: 
: MOD_QUIESCE acts on all instances of this driver, no matter which bus
: it is attached to, and no matter what it attaches to upwards in the
: kernel (netgraph, geom, netstack, filesystemc, MAC etc).  The user
: wants the kld unloaded, he asks for it, and MOD_QUISCE + MOD_UNLOAD
: handles this for him.
: 
: If I have understood anything about what you are talking about, the
: newbus involvement comes from the event expressing that I pressed
: the "I'll yank this one" button on a hot-swap PCI slot.

Yes.

: That would not and should not affect the instances which sit on 
: the ISA or S-BUS.
:
: I fail to see how it can avoid having a device_t argument to tell
: which instance the event is going to operate on, and further more,
: I entirely fail to see why you would use a module event to communicate
: this rather than have newbus call the instance in question directly
: with a quiesce method.

You are correct.  There will be a device_t argument.  I won't use a
module event to communicate this.  However, I will also map the module
event to this quiesce, since it is the same concept.

: What am I missing ?

You are looking at it backwards.  I want to map this button to some
newbus method.  I also want to map the MOD_QUIESCE to this same newbus
method if possible to reduce the load on driver writers.  They are the
same thing.  In the case of the pci hotswap example, only one driver
would get this message, while on a MOD_QUICESE all device_t's in that
module would get the quiesce call.

: How is MOD_QUIESCE any different from MOD_UNLOAD with respect to
: newbus ?

I've already gone into that: it is a mess now and MOD_QUIESECE makes
it a little messier and I'm trying to clean up the mess.

: >: 	If any instance is opened, return EBUSY.
: >: 	Otherwise destroy_dev all instances and do not attach to
: >: 	any new instances offered.
: >
: >Same as the above.  Likely with a lot of wrinkles for bus drivers.  If
: >pccard were to receive this, does it consider itself busy because it
: >has children drivers?  Or should it pass the request down to them?
: >Since the children drivers aren't necessarily modules, we can't just
: >rely on them.
: 
: I think you have overlooked something fundamental Warner.
: 
: MOD_QUIESCE only offers the ability to split the current MOD_UNLOAD
: semantics in two parts so that -f(orce) can override the
: policy decision part of the action in the module.
: 
: If pccard today implements MOD_UNLOAD, then you can split that code
: in two and put half of it in MOD_QUIESCE if you want to implement
: a -f(orce) semantic for pccard.
: 
: If modules cannot implement MOD_QUISCE they don't have to.
: 
: If you don't do that, then it will work exactly the way it does
: today (provided it returns a sensible value for an unknown events).

which isn't the case, as I've pointed out.

: MOD_UNLOAD has the same semantics as they already have, including
: the ability to simply not implement unloads because it is too hairy.

I'm trying to clean things up.  So arguing that things aren't perfect
today isn't relevant.

: >In addition, these sorts of recommendations fundamentally change the
: >way that drivers unload these days.  Today, kldunload of a typical
: >network driver will detach it, even if it is up or busy.  It is an
: >extraordinary driver that does anything differently.
: 
: No, it doesn't change anything "fundamentally".
: 
: It gives the module writer the ability to split the MOD_UNLOAD into
: two so that a -f(orce) semantic can be implemented, and if he doesn't
: do that, then you have the exact same situation you have today.

We view the change differently then.  As drivers start to implement
this, you get different behavior between those that have been
converted and those that haven't.  Those that have been converted
kldunload turns into 'unload if not busy' while those that haven't are
a 'force unload'.  This can lead to different behavior and a bad user
experience.  While MOD_QUIESCE appears to be optional, having
different behavior for different modules will be a problem for our
users.

"when I unload fxp, it always unloads.  but for dc I have to unload -f
the driver for it to unload if it is attached, why the difference" is
the sort of thing I'm trying to avoid.

: >Do you plan on fixing all the drivers/modules in the tree that doesn't
: >do this now?
: 
: No, MOD_QUIESCE is entirely optional.  If you don't implement it
: there will be no difference between kldunload with or without -f(orce)
: and that's it.  There is no difference today for any driver.

See abouve.

: >Having inconsistant unload behavior is one of the
: >problems we have in the tree, and it seems like this would make it
: >worse.
: 
: If you want consistent unload behaviour, you need to start out by
: defining what it should be.

Shouldn't you have done that before this commit?  You can't put that
back on me since you are the agent of this change.

: For GEOM we currently refuse unloading if anything is busy.  This is
: based on the behaviour of unmount which refuses if any vnodes are in use.
: 
: This entire thing comes about because sometimes you may want to
: unload busy GEOM classes, and that's what -f(orce) gives you a
: lever for.

I understand that.

: If we have modules which today unload while being in use then I would
: personally argue that it is a POLA violation, but given the lack of
: the -f(orce) lever until now, I can fully understand the reason
: why it got that way.

Yes.  However, we have to fix that.  Suddenly declaring all modules in
the tree to be doing the wrong thing seems a bit excessive.

: Hopefully now with -f(orce) we can get a consistent behaviour
: which IMO should be:
: 
: 	"kldunload" fails if the module is in use.
: 	"kldunload -f" suceeds even if the module is in use.

I agree that's a good goal.

: >Also, in preparing this message, I noticed a number of drivers that
: >return EINVAL instead of EOPNOSUPP for MOD_ events they don't
: >understand.  There are several that also return 0 for events that they
: >don't understand.  These should be corrected in the tree.  Do you have
: >plans to do this?
: 
: As I find them yes.
: 
: Currently EOPNOTSUPP and zero are treated as "OK" return from
: MOD_QUIESCE.  You'll notice that I put it in the man-page to define
: a sensible default behaviour so we can eliminate this particular
: inconsistency over time.

Please do.  I'd planned on making a sweep.

: >And none of this helps us to unload idle drivers, but I guess that
: >wasn't your goal.
: 
: You mean automatically unload drivers which are not/no longer used ?
: No, that was nowhere near my goals.  If you mean something else
: you'll have to explain to me what you meant.

No, that's what I mean.

: >Finally, I'm not trying to be negative here.  There are just a lot of
: >issues that haven't been considered yet.  I'm a little grumpy that I
: >was specifically told that you didn't want to hear about newbus
: >issues, but I'm trying to be constructive in my comments here none the
: >less.  I am looking to implement something in newbus with these
: >messages, so I'm trying to understand it at a high level of detail.
: 
: As you can see above I still totally fail to see the newbus connection
: in this.

Yes.  And it is very frurstrating that you don't. I'll explain it
again here so you can understand what you are missing.

: I can see that implementations of *both* MOD_UNLOAD and MOD_QUIESCE
: for bus modules could get some help from newbus, but we don't have
: that today for MOD_UNLOAD as far as I know, so I doubt that MOD_QUIESCE
: changes that.

You are declaring MOD_UNLOAD as doing something different than it does
now by fiat.  You are slowly making some drivers behave differently,
and I say that is going to violate POLA.

Finally, I'd like to reiterate that I want to integrate this into
newbus and am willing to do the work.  I find it rather frustrating
that you are attacking me for pointing out the weaknesses in the
resulting system after your changes and the subtle changes in
semantics that are implied by the mere existance of MOD_QUIESCE.
While some of them are true before, this whole thing feels rushed in
and I'm trying hard to styfle my frustration at the speed it went into
the tree and move forward to make things work in a sensible way.

Anyway, I look forward to getting things hammered out.  I see a way I
can do that right now.  I just want to make sure that we clearly
define what MOD_QUIESCE expects from the module as well as MOD_UNLOAD
so that our users have a consistant experience accross the tree.

Warner