[ CC'd to clearview-dev with Ramesh's permission. To keep a visible
record of the files he reviewed, I haven't elided any of his comments. ]
> Thanks for the excellent comments in the code and static Dtrace probes.
Great, you're welcome :-) For the record, there are relatively few DTrace
probes in the code because the new IPMP API already provides natural FBT
tracing points. As such, I added DTrace probes only for things where FBT
was insufficient, such as inside loops or key conditional paths.
> I have gone through below mentioned files (and skipped specific
> sections like IPSQ changes). I spent most of the time understanding
> the new code and logic :-).
> Here are some comments.
Thank you. A webrev covering the resulting changes is at:
http://cr.opensolaris.org/~meem/clearview-ipmp-cr-diff5
> -----------------------------------------------------------------------
> IPMP ARP support
>
> usr/src/uts/common/inet/arp.h
> usr/src/uts/common/inet/arp_impl.h
>
> No comments.
>
> usr/src/uts/common/inet/arp/arp.c
>
> Line 426:
>
> Can we check if xmit_arl !=NULL and return, instead of doing
> all comparisions after that. I am not sure how compiler does
> the optimisation.
The check at 426 is only to see if we were able to get an xmit_arl from
the hardware address. If not, then we attempt to get an xmit_arl from the
source address, and then finally from any active arl in the IPMP group.
In other words, xmit_arl may (and is likely not) the same value at each
check since we reload it at lines 432, 439, and 440.
> Line: 678-685
> Do we have to clear the ACE_F_PERMANENT before deleting the
> ace? we delete the ace anyway, and ar_ce_delete(), sets the
> ACE_F_DYING flag. The ar_query_reply called from
> ar_ce_delete() will see the ACE_F_DYING flag. Is there any
> other reson to clear the ACE_F_PERMANENT flag?
You're right, this looks a bit silly. I think I got fooled by the
existing code in ar_ce_delete_per_arl() that also clears ACE_F_PERMANENT
before doing the ar_ce_delete(), and concluded that it must be required.
(It's not clear to me that it's necessary in the ar_ce_delete_per_arl()
routine either.) In any case, I've updated the comments and removed the
clearing of ACE_F_PERMANENT.
> Line: 3601-3603
> Is this check necessary?
> Above conditon will take care of it.
(Jim also flagged this, and I've since fixed it.) This was due to a
mismerge with 6691986.
> -------------------------------------------------------------------
>
> IPMP Kernel core
>
> usr/src/uts/common/inet/ip.h
> usr/src/uts/common/inet/ip/igmp.c
>
> No comments.
>
> usr/src/uts/common/inet/ip/ip.c
>
> Line: 335
> Typo:
> shoule be "writer on the ipsq."
Indeed; fixed.
> Lines: 1207, 1209-1210, 1212, 1214:
> Aren't we supposed to give four spaces for continuing the next
> line. All other lines follows that.
The other lines are actually indented to the next tabstop. The previous
line starts with the slashterisk (column 8) and thus these lines should
technically be indented at column 12, but that looks horrible, so some
compromises have been made. In any case, I've changed the lines you
indicated to follow the existing convention of the next tabstop.
> usr/src/uts/common/inet/ip/ip6.c
> usr/src/uts/common/inet/ip/ip6_if.c
> usr/src/uts/common/inet/ip/ip6_ire.c
> usr/src/uts/common/inet/ip/ip6_rts.c
> usr/src/uts/common/inet/ip/ip_ftable.c
> usr/src/uts/common/inet/ip/ip_if.c
> usr/src/uts/common/inet/ip/ip_mroute.c
> usr/src/uts/common/inet/ip/ip_ire.c
> usr/src/uts/common/inet/ip/ip_mroute.c
> usr/src/uts/common/inet/ip/ip_multi.c
> usr/src/uts/common/inet/ip/ip_ndp.c
> usr/src/uts/common/inet/ip/ip_netinfo.c
> usr/src/uts/common/inet/ip/ip_rts.c
> usr/src/uts/common/inet/ip6.h
> usr/src/uts/common/inet/ip_if.h
> usr/src/uts/common/inet/ip_ire.h
> usr/src/uts/common/inet/ip_multi.h
> usr/src/uts/common/inet/ip_ndp.h
> usr/src/uts/common/inet/ip_stack.h
>
> No comments
>
> usr/src/uts/common/inet/ip.h
>
> ill_nomcast sounds like, ill no mcast, and bit confusing. Can
> you change it to iill_nom_cast or something else.
Good point; changed to ill_nom_cast. Also added a comment to
ip_rput_process_multicast() to make things clearer.
> usr/src/uts/common/inet/ip/ipmp.c
>
> Line: 925 - 928
>
> Do we need ipmp_illgrp_mark_arpent() function. This fucntion
> is called only to set this flag and called only in two
> places. Can't we set the flag in the callers and put a static
> dtarce probe. I am fine with using this function also.
I considered both approaches but ultimately felt this was better since it
preserved the ipmp_arpent_t abstraction. (Perhaps not obvious, but one of
my goals was having all access to IPMP-specific data structure fields
isolated to ipmp.c. Sometimes, other goals (such as keeping the locking
design clear) took precedent, though.)
> Line: 1690
> extra the in the sentence. Should be "will indicate to the
> resolver".
Indeed; fixed. Also, while I was in here, I decided to rename
"Res_act_move" to "Res_act_rebind" since I think that's a more accurate
name given the Clearview IPMP design.
--
meem
