Hello Eric,
Thank you for reviewing the code changes, my comments in-line below
for both MAC and TRILL comments.
Eric Cheng wrote:
[...]
>
> 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.
ACCEPTED, I have renamed it to learnval.
> 5381:
> shouldn't the mac_capab_update() be done after mac_poll_state_change()?
ACCEPTED
> 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:
ACCEPTED, I have created macros MAC_RING_TX and MAC_TX.
>
> with the above, you can replace all calls to mac_ring_tx() with MAC_TX().
ACCEPT
> 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().
mac_tx_common() wasn't necessary, callers incl. bridging use MAC_TX and
for bridging we use the function mac_bridge_tx. New macros are:
263 #define MAC_RING_TX(mhp, rh, mp, rest) { \
264 mac_ring_handle_t mrh = rh; \
265 mac_impl_t *mimpl = (mac_impl_t *)mhp; \
266 /* \
267 * Send packets through a selected tx ring, or through the \
268 * default handler if there is no selected ring. \
269 */ \
270 if (mrh == NULL) \
271 mrh = mimpl->mi_default_tx_ring; \
272 if (mrh == NULL) { \
273 rest = mimpl->mi_tx(mimpl->mi_driver, mp); \
274 } else { \
275 mac_ring_t *mring = (mac_ring_t *)mrh; \
276 mac_ring_info_t *info = &mring->mr_info; \
277 \
278 ASSERT(mring->mr_type == MAC_RING_TYPE_TX && \
279 mring->mr_state >= MR_INUSE); \
280 rest = info->mri_tx(info->mri_driver, mp); \
281 } \
282 }
283
284 /*
285 * This is the final stop before reaching the underlying driver
286 * or aggregation, so this is where the bridging hook is implemented.
287 * Packets that are bridged will return through mac_bridge_tx(), with
288 * rh nulled out if the bridge chooses to send output on a different
289 * link due to forwarding.
290 */
291 #define MAC_TX(mip, rh, mp, share_bound) { \
292 /* \
293 * If there is a bound Hybrid I/O share, send packets through \
294 * the default tx ring. (When there's a bound Hybrid I/O share, \
295 * the tx rings of this client are mapped in the guest domain \
296 * and not accessible from here.) \
297 */ \
298 _NOTE(CONSTANTCONDITION) \
299 if (share_bound) \
300 rh = NULL; \
301 /* \
302 * Grab the proper transmit pointer and handle. Special \
303 * optimization: we can test mi_bridge_link itself atomically, \
304 * and if that indicates no bridge send packets through tx ring.\
305 */ \
306 if (mip->mi_bridge_link == NULL) { \
307 MAC_RING_TX(mip, rh, mp, mp); \
308 } else { \
309 mp = mac_bridge_tx(mip, rh, mp); \
310 } \
311 }
> 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.
ACCEPTED
Eric Cheng wrote:
> Hi rishi,
>
> here are the rest of my comments:
>
> trill.c:
>
> 178:
> is the tsp->ts_link->bl_mh check necessary?
> seems like a ts_link should always have a non-NULL bl_mh.
ACCEPT, added an assert instead.
> 440:
> could use sizeof (struct ether_vlan_extinfo).
ACCEPT
> 888:
> why not just 'return' here instead of creating a wrong kstat?
ACCEPT
> 1099:
> is it possible to bound this to a reasonable number like 1Mb?
> (to avoid erroneously consuming too much memory)
ACCEPT
> 1116:
> what if tn_tsp is NULL?
It is okay if tn_tsp is NULL, I have added a missing check in the
LISTNICK ioctl.
> what if the same nick is added twice? I don't see duplicate checks.
REJECT, We check if there is an existing node and call trill_del_nick at 1140
> 1148:
> do you need to check that the flags in the array are valid before
> passing to bridge_trill_setvlans()?
REJECT, It is a bitarray to track a set of VLANs and no specific values to
check against.
> 1179:
> this should return an error if the nick doesn't exist.
ACCEPT
> 1195:
> could use VALID_NICK to check treeroot.
ACCEPT
> 1525:
> it may be better to do the privilege check at trill_create(). this
> ensures non-privileged users cannot use AF_TRILL.
REJECT, We need to support non privileged ioctls as well. Updated code
includes non privileged ioctls.
> 270,1511:
> do you need to increment tks_drops for these error cases?
> this is done in 567.
ACCEPT
> 1051,1069,1246:
> could use VALID_NICK for these checks.
ACCEPT
> also, it may be safer to change VALID_NICK to
> ((n) > RBRIDGE_NICKNAME_NONE && (n) < RBRIDGE_NICKNAME_MAX)
REJECT, nick is uint16 so the check is more specific.
I will post an updated webrev covering all the changes from
code reviews once I am done testing.
Thanks,
Rishi
_______________________________________________
networking-discuss mailing list
[email protected]
