crossbow discuss - Aug 2009 - mystery surrounding mac_set_mtu()

Crossbow team,

If you read the Public Comments in 6871916, there is an open question
regarding why mac_set_mtu() calls mac_mark_exclusive().  I personally
don''t see why the i_mac_perim_enter() on its own doesn''t
implement the
exclusivity semantics required by the function (it''s essentially just a
kernel wrapper around setting the "mtu" link property).  Can anyone
shed
some light on this?

Aside from panicing the system due to 6871916, this also has the
negative side-effect of preventing other threads from issuing mac_open()
while mac_set_mtu() is in progress (other threads will get back EBUSY).
It doesn''t seem right to me that opening /dev/net/foo0 (for example)
would return EBUSY while setting the "mtu" link property, but not any
other link property, or issuing any other "administrative" operation
that requires the perimeter (such as adding multicast addresses, etc.).

-Seb

Sebastien Roy wrote:
> Crossbow team,
>
> If you read the Public Comments in 6871916, there is an open question
> regarding why mac_set_mtu() calls mac_mark_exclusive().  I personally
> don''t see why the i_mac_perim_enter() on its own doesn''t
implement the
> exclusivity semantics required by the function (it''s essentially
just a
> kernel wrapper around setting the "mtu" link property).  Can
anyone shed
> some light on this?
>   
The perimeter does ensure that only 1 thread can do mac_set_mtu() at a 
time on the given mac. But I am not sure why mac_set_mtu() wants to  
call mac_mark_exclusive().  Is it that not all drivers implement the 
mac_maxsdu_update() and so we can''t change the mtu except if we are the
only mac client ?

Thirumalai
> Aside from panicing the system due to 6871916, this also has the
> negative side-effect of preventing other threads from issuing mac_open()
> while mac_set_mtu() is in progress (other threads will get back EBUSY).
> It doesn''t seem right to me that opening /dev/net/foo0 (for
example)
> would return EBUSY while setting the "mtu" link property, but not
any
> other link property, or issuing any other "administrative"
operation
> that requires the perimeter (such as adding multicast addresses, etc.).
>
> -Seb
>
>
> _______________________________________________
> crossbow-discuss mailing list
> crossbow-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/crossbow-discuss
>

On (08/14/09 18:23), Thirumalai Srinivasan wrote:
>>   
> The perimeter does ensure that only 1 thread can do mac_set_mtu() at a  
> time on the given mac. But I am not sure why mac_set_mtu() wants to   
> call mac_mark_exclusive().  Is it that not all drivers implement the  
> mac_maxsdu_update() and so we can''t change the mtu except if we
are the
> only mac client ?
>
I think Girish and Nicolas had some complex phone discussions
about this, so maybe they can elaborate. As I recall, it had to
do with some re-entry in the aggr driver?

--Sowmini

On Fri, 2009-08-14 at 21:46 -0400, Sowmini.Varadhan at Sun.COM
wrote:
> On (08/14/09 18:23), Thirumalai Srinivasan wrote:
> >>   
> > The perimeter does ensure that only 1 thread can do mac_set_mtu() at a
> > time on the given mac. But I am not sure why mac_set_mtu() wants to   
> > call mac_mark_exclusive().  Is it that not all drivers implement the  
> > mac_maxsdu_update() and so we can''t change the mtu except if
we are the
> > only mac client ?
> >
> 
> I think Girish and Nicolas had some complex phone discussions
> about this, so maybe they can elaborate. As I recall, it had to
> do with some re-entry in the aggr driver?
If aggr does an upcall while handling a downcall, or even holds any lock
while making an upcall, then isn''t that a bug in the aggr driver?  One
would assume that sane locking semantics cannot be maintained if locks
are allowed to be held while making both up and down calls.  This is
why, in the new iptun driver, notifications to the mac layer are handled
asynchronously without holding locks or perimeters (stress testing
revealed that this was the only way to make things work reliably).

In any case, yes, if Girish or Nicolas could elaborate, that would be
nice.  At the moment, I''ve had to remove the calls to
mac_mark_exclusive() in order to do any sort of stress testing with
GLDv3 that include setting the mtu link property.

-Seb

One goal is to prevent multiple clients to simultaneously update the  
MTU. The perimeter should be sufficient to do this. There are also  
drivers which cannot change their MTU while they are started, but on  
the other hand some drivers may support this, and therefore we could  
depend on the drivers to do test for this condition instead of doing  
this from mac.

I don''t remember issues related to re-entry with aggr from the top of  
my head. If there is one I agree that this should not require a  
workaround like this in mac. BTW, it''s a bit silly to require a driver
to call mac_maxsdu_update() while updating its MTU. If the framework  
is calling the driver to update the MTU it should know that the MTU is  
changing.

But this is really not even address the real issue. There''s still the  
risk of having mac_perim_enter_by_macname() fail when it''s called from
dls_devnet_close() if there''s any operation holding the perimeter in  
progress. dls_devnet_close() should be able to handle the case where  
there are pending MAC clients accessing the MAC instance and wait  
until all references to the mac_impl_t are dropped, or we should  
ensure that dls_devnet_close() is called only after these references  
are dropped, or we should change dls_devnet_close() to handle the case  
when it cannot enter the perimeter.

Nicolas.

On Aug 14, 2009, at 7:49 PM, Sebastien Roy wrote:
>
> On Fri, 2009-08-14 at 21:46 -0400, Sowmini.Varadhan at Sun.COM wrote:
>> On (08/14/09 18:23), Thirumalai Srinivasan wrote:
>>>>
>>> The perimeter does ensure that only 1 thread can do mac_set_mtu()  
>>> at a
>>> time on the given mac. But I am not sure why mac_set_mtu() wants to
>>> call mac_mark_exclusive().  Is it that not all drivers implement
the
>>> mac_maxsdu_update() and so we can''t change the mtu except
if we
>>> are the
>>> only mac client ?
>>>
>>
>> I think Girish and Nicolas had some complex phone discussions
>> about this, so maybe they can elaborate. As I recall, it had to
>> do with some re-entry in the aggr driver?
>
> If aggr does an upcall while handling a downcall, or even holds any  
> lock
> while making an upcall, then isn''t that a bug in the aggr driver? 
One
> would assume that sane locking semantics cannot be maintained if locks
> are allowed to be held while making both up and down calls.  This is
> why, in the new iptun driver, notifications to the mac layer are  
> handled
> asynchronously without holding locks or perimeters (stress testing
> revealed that this was the only way to make things work reliably).
>
> In any case, yes, if Girish or Nicolas could elaborate, that would be
> nice.  At the moment, I''ve had to remove the calls to
> mac_mark_exclusive() in order to do any sort of stress testing with
> GLDv3 that include setting the mtu link property.
>
> -Seb
>
>
> _______________________________________________
> crossbow-discuss mailing list
> crossbow-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/crossbow-discuss
-- 
Nicolas Droux - Solaris Kernel Networking - Sun Microsystems, Inc.
nicolas.droux at sun.com - http://blogs.sun.com/droux

Hi Nicolas,

On Fri, 2009-08-14 at 21:10 -0700, Nicolas Droux wrote:
> One goal is to prevent multiple clients to simultaneously update the  
> MTU. The perimeter should be sufficient to do this.
I agree.  This should be no different than updating any other property,
really, which simply involves holding the perimeter.
> There are also  
> drivers which cannot change their MTU while they are started, but on  
> the other hand some drivers may support this, and therefore we could  
> depend on the drivers to do test for this condition instead of doing  
> this from mac.
Yes, I''ve tested this with a few drivers and have inspected all drivers
in ON, and drivers respond with ENOTSUP if they don''t handle setting
the
"mtu" property in their property set callbacks.
> I don''t remember issues related to re-entry with aggr from the top
of
> my head. If there is one I agree that this should not require a  
> workaround like this in mac. BTW, it''s a bit silly to require a
driver
> to call mac_maxsdu_update() while updating its MTU. If the framework  
> is calling the driver to update the MTU it should know that the MTU is  
> changing.
I totally agree, but the semantics should be well-defined either way.
For example, if indeed the mac module is modified to change mi_sdu_max
and send notifications itself, then the (upcoming) driver API
documentation should make it clear that the mac_maxsdu_update() would
only need to be called when the MTU changes in all cases except for the
"mtu" link property.
> But this is really not even address the real issue.
I agree, my motivation was to split this small bit of code-trivia out of
the CR I mentioned, which is still a problem, and fix it along with IP
tunneling.
> There''s still the  
> risk of having mac_perim_enter_by_macname() fail when it''s called
from
> dls_devnet_close() if there''s any operation holding the perimeter
in
> progress. dls_devnet_close() should be able to handle the case where  
> there are pending MAC clients accessing the MAC instance and wait  
> until all references to the mac_impl_t are dropped, or we should  
> ensure that dls_devnet_close() is called only after these references  
> are dropped, or we should change dls_devnet_close() to handle the case  
> when it cannot enter the perimeter.
If dls_devnet_rename() is the only remaining consumer of this exclusive
flag, then perhaps we need to rethink this.  If dls_devnet_close() is
being called, then certainly there is a reference on the dls_devnet_t
that was held on open, and dls_devnet_rename() will return EBUSY and
won''t get to set the exclusive flag.  So I think if rename is the only
consumer, then dls_devnet_close() _can_ assert that
mac_perim_enter_by_macname() won''t return EBUSY.

The problem is then, do we want dls_devnet_rename() to be the only
consumer of this exclusive interface in the future to ensure that this
fragile bit of code keeps working?  Or do we need a more complex
mechanism to allow dls_devnet_close() to work in the face of any
possible future consumer of mac_mark_exclusive()?

-Seb

On Sat, 2009-08-15 at 09:55 -0400, Sebastien Roy wrote:
> If dls_devnet_rename() is the only remaining consumer of this exclusive
> flag, then perhaps we need to rethink this.  If dls_devnet_close() is
> being called, then certainly there is a reference on the dls_devnet_t
> that was held on open, and dls_devnet_rename() will return EBUSY and
> won''t get to set the exclusive flag.  So I think if rename is the
only
> consumer, then dls_devnet_close() _can_ assert that
> mac_perim_enter_by_macname() won''t return EBUSY.
> 
> The problem is then, do we want dls_devnet_rename() to be the only
> consumer of this exclusive interface in the future to ensure that this
> fragile bit of code keeps working?  Or do we need a more complex
> mechanism to allow dls_devnet_close() to work in the face of any
> possible future consumer of mac_mark_exclusive()?
And if mac_perim_enter_by_macname() can fail for any other reason in
this context, then certainly there is more thinking to be done on how
dls_devnet_close() can be made to reliably work altogether.

-Seb

On Sat, 2009-08-15 at 09:55 -0400, Sebastien Roy wrote:
> Hi Nicolas,
> 
> On Fri, 2009-08-14 at 21:10 -0700, Nicolas Droux wrote:
> > One goal is to prevent multiple clients to simultaneously update the  
> > MTU. The perimeter should be sufficient to do this.
> 
> I agree.  This should be no different than updating any other property,
> really, which simply involves holding the perimeter.
I''ve filed 6872221 to address the correctness of mac_set_mtu()
separately.
> > I don''t remember issues related to re-entry with aggr from
the top of
> > my head. If there is one I agree that this should not require a  
> > workaround like this in mac. BTW, it''s a bit silly to require
a driver
> > to call mac_maxsdu_update() while updating its MTU. If the framework  
> > is calling the driver to update the MTU it should know that the MTU is
> > changing.
> 
> I totally agree, but the semantics should be well-defined either way.
> For example, if indeed the mac module is modified to change mi_sdu_max
> and send notifications itself, then the (upcoming) driver API
> documentation should make it clear that the mac_maxsdu_update() would
> only need to be called when the MTU changes in all cases except for the
> "mtu" link property.
And I''ve filed RFE 6872222 to address this particular issue.

-Seb

I looked at the code, both in current onnv, and in onnv_110. The 
call to mac_mark_exclusive() seems pretty redundant since mac_open()
will itself block on the i_mac_perim_enter as Seb initially pointed out.

so I agree with Seb''s removal of that particular line..
--Sowmini

On Mon, 2009-08-17 at 15:09 -0400, Sowmini.Varadhan at Sun.COM
wrote:
> I looked at the code, both in current onnv, and in onnv_110. The 
> call to mac_mark_exclusive() seems pretty redundant since mac_open()
> will itself block on the i_mac_perim_enter as Seb initially pointed out.
> 
> so I agree with Seb''s removal of that particular line..
Great, thanks.  I''ve also stress-tested the pants out of the fix (the
removal of the mac_mark_exclusive() call in mac_set_mtu()), and it has
been running the stress test since Friday non-stop instead of panicing
every few hours. :-)

-Seb