On Thu, Jul 16, 2009 at 01:22:06PM -0400, Rishi Srivatsavai wrote:
> Code review request for the RBridges project. RBridges project
> is delivering layer-2 Bridging and TRILL support in Solaris.
>
> Webrev for the ON changes (Bridging and TRILL):
> http://cr.opensolaris.org/~rishi/rbridges/
>
Hi rishi,
I have looked at the mac related changes. I just found a few minor things:
mac.c:
2728:
it may be better to rename this to 'value' since it looks a bit strange
to assign limitval to mi_ldecay at 2739.
5381:
shouldn't the mac_capab_update() be done after mac_poll_state_change()?
tx related changes:
you've introduced two new functions mac_ring_tx() and mac_bridge_tx().
I suspect that adding two extra call frames may have a slight impact
on performance. (in my previous measurements, the impact was ~2%+ on
sun4v. this shows up in small packet sends)
to minimize the impact, I'd suggest you refactor as follows so the
fast paths could be kept inlined:
add the macros:
MAC_RING_TX
{
mac_impl_t *mip = (mac_impl_t *)mh;
if (rh == NULL)
rh = mip->mi_default_tx_ring;
if (rh == NULL) {
return (mip->mi_tx(mip->mi_driver, mp));
} else {
mac_ring_t *ring = (mac_ring_t *)rh;
mac_ring_info_t *info = &ring->mr_info;
ASSERT(ring->mr_type == MAC_RING_TYPE_TX);
ASSERT(ring->mr_state >= MR_INUSE);
ASSERT(info->mri_tx != NULL);
return (info->mri_tx(info->mri_driver, mp));
}
}
MAC_TX
{
if (share_bound)
rh = NULL;
/*
* Grab the proper transmit pointer and handle. Special optimization:
* we can test mi_bridge_link itself atomically, and if that indicates
* no bridge, then avoid the mutex and call directly, allowing
* lock-free tail-call optimization for the common case.
*/
if (mip->mi_bridge_link == NULL) {
return (MAC_RING_TX((mac_handle_t)mip, rh, mp));
} else {
return (mac_bridge_tx((mac_handle_t)mip, rh, mp));
}
}
and the functions:
mac_tx_common(...)
{
return (MAC_RING_TX(...));
}
mac_bridge_tx(...)
{
/*
* Once we take a reference on the bridge link, the bridge
* module itself can't unload, so the callback pointers are
* stable.
*/
mutex_enter(&mip->mi_bridge_lock);
if ((mh = mip->mi_bridge_link) != NULL)
mac_bridge_ref_cb(mh, B_TRUE);
mutex_exit(&mip->mi_bridge_lock);
if (mh == NULL) {
return (mac_tx_common((mac_handle_t)mip, rh, mp));
} else {
mp = mac_bridge_tx_cb(mh, rh, mp);
mac_bridge_ref_cb(mh, B_FALSE);
return (mp);
}
}
with the above, you can replace all calls to mac_ring_tx() with MAC_TX().
I've also renamed your original mac_bridge_tx() to mac_tx_common() since
this function is not really bridge specific. the actual bridge handling
code is renamed to mac_bridge_tx(). bridge.c should be changed to call
mac_tx_common().
on the rx side, you don't have to do the same refactoring since the
performance impact is much less. but you should rename mac_bridge_rx()
to mac_rx_common() for symmetry.
other mac changes look fine and I'll have comments for trill tomorrow.
eric
_______________________________________________
networking-discuss mailing list
[email protected]
