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
Thirumalai Srinivasan
2009-Aug-15 01:23 UTC
[crossbow-discuss] mystery surrounding mac_set_mtu()
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 >
sowmini.varadhan at sun.com
2009-Aug-15 01:46 UTC
[crossbow-discuss] mystery surrounding mac_set_mtu()
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
sowmini.varadhan at sun.com
2009-Aug-17 19:09 UTC
[crossbow-discuss] mystery surrounding mac_set_mtu()
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