In your scheme what is the point in time at which the framework can be sure
that a mac endpoint has been quiesced and there won't be any further
upcalls/notifies from the driver on this endpoint ? You seem to have
postponed
this point of time right up to driver's detach -> mac_destroy(). Whether and
when exactly the driver's detach routine is called is decided by the
DDI framework.
Separating mac_unregister from mac_destroy is not a big issue, but the
framework needs a well defined way by which it can quiesce a mac endpoint.
Note that with polling and direct call features, the driver and the
framework
have opaque pointers to each other's data structures (such as rings) and we
need a positive mechanism for quiescing a mac endpoint to be sure that there
isn't a pending upcall or downcall thread with a stale pointer.
For example when all mac clients have closed a mac end point, the
mac framework would like to quiesce the end point and reclaim / cleanup
anything associated with that mac endpoint. The last client close ->
mac_stop()
-> xxx_m_stop is a point where this synchronization can happen.
From the driver side, it has to have the internal mechanisms to quiesce
itself
in response to a request from above say via the xxx_m_stop. If it is hard to
do a synchronous xxx_m_stop then may be a notification when it is completed
is another possibility. From the framework side, it must make sure that
xxx_m_* entry points are single threaded on a given mac endpoint. So we
won't have a xxx_m_start concurrently coming down with a xxx_m_stop.
Thirumalai
Garrett D'Amore wrote:
>On Fri, 2007-09-07 at 09:11 -0700, Thirumalai Srinivasan wrote:
>
>
>>>There are further complexities, because the driver cannot know if any
>>>upstream callers are still busy until it calls mac_unregister(). And
>>>then, because mac_unregister *also* frees the structure, its too late.
>>>
>>>
>>>
>>>
>>If there are indeed upstream callers, mi_ref is non-zero and
>>mac_register will fail. So it should be sufficient, if the driver
>>quiesces the chip, and become single threaded on the mac endpoint before
>>calling mac_unregister to avoid the race.
>>
>>
>
>You're missing a big point. That is that the effort to become single
>threaded, *before* this point, may be a significant PITA, particularly
>since mac_unregister may fail.
>
>
>
>> Another question, what does
>>xxx_m_stop really mean ? Why can't the driver quiesce everything at this
>>point itself ? If mac_unregister() succeeds, it implies that an
>>xxx_m_stop has happened before that. It doesn't seem right if we can
>>still have upcall threads and notifications going on even after
>>mac_stop() has finished successfully.
>>
>>
>
>Well, perhaps mac_stop says that we shouldn't upcall. I'm not sure. If
>so, then this would solve the race. Right now I *think* pretty much
>most drivers do shutdown the chip (though I'm not sure all of them do)
>in mac_stop, to the point of even dropping the link.
>
>I do know that there are cyclics, etc. that some of these drivers
>register in attach(), and don't tear down until detach(), and that they
>would *prefer* to do that after unregistration is complete.
>
>One question here... can mac_start() (or any other mac entry point) be
>called *while* running the DDI detach code? It isn't clear to me that
>DDI detach is single threaded from MAC's view until after mac_unregister
>is called.
>
>In other words, imagine this...
>
>
>m_stop() {
>... statep->STARTED = B_FALSE;
>}
>
>m_start() {
>... statep->STARTED = B_TRUE;
>}
>
>detach() {
>...
> if (statep->STARTED)
> return (DDI_FAILURE);
> /* race point */
> mac_unregister(...);
>}
>
>Now what happens if, at "race point" above, mac_start() is called?
>
>One solution to this is to add some complexity to the in the drivers,
>like this:
>
>m_start() {
> mutex_enter(&statep->LOCK);
> if (statep->DETACHING) {
> return B_FALSE;
> }
> statep->STARTED = B_TRUE;
> mutex_exit(&statep->LOCK);
>}
>
>detach() {
> mutex_enter(&statep->LOCK);
> if (statep->STARTED) {
> mutex_exit(&statep->LOCK);
> return (DDI_FAILURE);
> }
> statep->DETACHING = B_TRUE;
> mutex_exit(&statep->LOCK);
> return (DDI_SUCCESS);
>}
>
>So that adds complexity to the driver. Furthermore, it has a potential
>semantic change that I'm uncomfortable with. Specifically, it isn't
>clear to me that a stream in DL_ATTACHED, but not DL_BOUND, state, will
>have statep->STARTED set to true.
>
>In otherwords, does the very act of calling mac_open() with for a node
>(which holds the device busy from DR) *also* arrange to call
>mac_start()? And does it do so atomically, so that there is no race
>where DDI detach can be called between mac_open and mac_start?
>
>All these questions of atomicity are confusing, and lead to a
>substantial increase in complexity. I still maintain the simplest
>solution is to separate mac destruction from unregistration from the
>framework.
>
>Here's an sample implementation that I think is cleaner:
>
>mac_start() {
> ... /* no need to set or check any flags */
>}
>
>mac_stop() {
> /* no need to set or check any flags */
>}
>
>detach() {
> if (mac_unregister(mh) == B_FALSE)
> return (DDI_FAILURE);
> /* do any thing else required to go single threaded, etc. */
> mac_destroy(mh);
>}
>
> -- Garrett
>
>
>>Thirumalai
>>
>>Garrett D'Amore wrote:
>>
>>
>>
>>>On Fri, 2007-09-07 at 08:25 -0400, Sebastien Roy wrote:
>>>
>>>
>>>
>>>
>>>>Paul Durrant wrote:
>>>>
>>>>
>>>>
>>>>
>>>>>On 06/09/07, Garrett D'Amore <[EMAIL PROTECTED]> wrote:
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>>(Obviously mac_alloc() here would return "mac_t *", but under the covers
>>>>>>it is really allocating a whole mac_impl_t. And mac_free() would be
>>>>>>free()'ing a a whole mac_impl_t, though it is taking a mac_t as its
>>>>>>argument.)
>>>>>>
>>>>>>If we want to pursue this course of action, I can put together a webrev
>>>>>>with the changes... it will take me a while because there are a *lot* of
>>>>>>drivers to change... but I want to get this change in *before* we make
>>>>>>GLDv3 public.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>FWIW, I find the current mac_register_t scheme ugly; I don't see why a
>>>>>statically defined mac_register_t cannot simply be passed into
>>>>>mac_register(). I don't see why we need separate allocator functions
>>>>>there.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>It's done this way so that the structure can be expanded without
>>>>affecting binary compatibility. We can add a new field to the structure,
>>>>and the system won't crash and burn when an existing driver registers.
>>>>The framework will allocate an appropriately sized structure, but the
>>>>older driver simply won't fill in the new fields it doesn't know about.
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>>I also don't see that you can't break this race using reference
>>>>>counting. There's no need for mac_unregister() to actually free the
>>>>>mac_impl_t; it could just drop a ref. and the last man out frees.
>>>>>Let's try not to complicate the interface because of implementation
>>>>>artifacts.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>I don't believe a reference counting scheme makes a difference in the
>>>>scenario that Thiru described. He's describing a scenario where the
>>>>mac_impl_t is freed by mac_unregister() before another thread gets a
>>>>chance to examine that mac_impl_t to verify its contents (either for a
>>>>flag or a reference count, it doesn't matter, the end result is still the
>>>>same; boom!)
>>>>
>>>>IMO, this case is borderline a driver bug. The driver has called some
>>>>MAC function (which hasn't returned yet), and decides to call
>>>>mac_unregister(). mac_unregister() is documented to invalidate the MAC
>>>>handle, so drivers shouldn't knowingly call mac_unregister() while using
>>>>that handle, or use the handle during or after calls to mac_unregister().
>>>>
>>>>To me, this is a similar problem to calling free() on a buffer that an
>>>>application knows that it's still using... Synchronization on the
>>>>allocation and deallocation of that buffer can't be done by the memory
>>>>framework, it has to be done by the application. The framework can't
>>>>possibly know what the application is going to do with that buffer once
>>>>the free() function has been called.
>>>>
>>>>
>>>>
>>>>
>>>There are further complexities, because the driver cannot know if any
>>>upstream callers are still busy until it calls mac_unregister(). And
>>>then, because mac_unregister *also* frees the structure, its too late.
>>>
>>>Forcing drivers to work around this will incur substantial code
>>>complexity across every Nemo driver. (Think an extra lock, flag, and
>>>condition variable used with *every* place that a driver calls up into
>>>Nemo.) I think that is a poor decision.
>>>
>>> -- Garrett
>>>
>>>
>>>
>>>
>>>
>>>
>
>
>
_______________________________________________
networking-discuss mailing list
[email protected]
