Peter Memishian writes: > At long last, we're ready to start the Clearview IPMP code review. > The review is divided into two phases: > > * Phase 1 covers the removal of the existing Nevada IPMP kernel > implementation. We're starting this now. > > * Phase 2 covers the Clearview IPMP kernel implementation, plus all > userland changes. This is the real meat of the project, and will > start in a couple of weeks.
Cool! This is a great way to organize a review of a rewrite. It would have been completely illegible otherwise. I hope others (including myself ;-}) can learn from this. > http://cr.opensolaris.org/~meem/clearview-noipmp-cr/ General: ip6i_t is some sweet haulage. We probably need a new ARC case to obsolete ipmp_hook_emulation and let customers know that this is going away. IPQoS is probably tougher. if_groupname is part of the publicly documented configuration interface for it: http://docs.sun.com/app/docs/doc/816-4554/ipqos-reference-1?a=view Why does SIOCSIPMPFAILBACK still exist in sys/sockio.h (and truss and in.mpathd)? It's gone from the kernel, and I wouldn't expect the new IPMP to make use of it, so I don't quite see how it's useful. Can the ilm discrepancy (ipif for IPv4, but ill for IPv6) go away now? I think this was done for IPMP. usr/src/cmd/mdb/common/modules/ip/ip.c Reviewed; no comments. usr/src/uts/common/inet/ip.h 758: this XXX comment appears related to the old IPMP code. 1218: note: will likely need to scan for contract holders on ipif_t. (CGTP and Cluster seem possible here.) 1411: nit: while you're here, a translation of this to English would be helpful. 1438,1456: as long as you're modifying this, changing it to a bona fide enum (rather than #define-like usage) would be nice. 1476: unreferenced; remove. 2710: remove. usr/src/uts/common/inet/ip/icmp.c usr/src/uts/common/inet/ip/icmp_opt_data.c Reviewed; no comments. usr/src/uts/common/inet/ip/igmp.c 1402-1403: I'm not sure I understand this comment. This project makes the main reason to enter the IPSQ here historical (and thus irrelevant). What things used by the timer are protected by the IPSQ? Is it more than ilm_filter? usr/src/uts/common/inet/ip/ip.c 7537: shouldn't this be ret_ill == NULL for the error case? Otherwise, this function always returns NULL and sometimes leaks a reference. 7851-7853: still references load spreading, but the code to do that load spreading (using ipif_rand()) is gone. 7863-7867: as for IPv6, do we ever really need ire_ipif? 7863: as for IPv6, RTF_MULTIRT code seems to have disappeared. 7869: how does this happen? 24927: this doesn't look right to me. The original assert said "!attach_if || ill_index != 0". Now, since attach if is going away, this means that attach_if is always false, and thus !attach_if should always be true. If the original assert was right, that means we don't know anything about ill_index ... doesn't it? 26310: ip_ioctl_finish? usr/src/uts/common/inet/ip/ip6.c 101: duplicate of line 110. 1845-1847: it's unclear to me what the lock here is protecting, given that 'srcp' is later dereferenced without the lock held. 4407: inbound load spreading? 4415-4418: looks like an old IPMP comment. 4420-4424: do we ever need to look at ire_ipif? When? 4420: CGTP RTF_MULTIRT code seems to have been nuked. 4435-4446: I don't think I understand this code. When is dst_ill != ill? What does it mean? Does it signal a serious design flaw (in which case ip0dbg isn't strong enough; assert seems right)? Or does it signal a configuration problem (in which case ip0dbg is way too strong)? At a guess, we shouldn't really be doing ip0dbg here. 5479-5486: how can this code be reached? Won't we panic on line 5478 if dst_ill is NULL? (I think this was left-over code for handling ip_newroute_get_dst_ill_v6 failure ... but that's not called anymore.) 5941: this comment doesn't seem to match the code. 11398,11400: locks here are probably ineffective. usr/src/uts/common/inet/ip/ip6_if.c 2347: nit: s/iset/set/ 2567: nit: would prefer != and remove 'continue'. 2770: not your code, but what's this (char *) cast about? It doesn't seem to be needed. 3045,3047: these assignments don't do anything; the variables are used only in the 'bad' branch, and there are no more jumps there after line 3022. (These were here because of the group-formation code, now removed.) 3108: this can never be true. usr/src/uts/common/inet/ip/ip6_ire.c 535-537: is this still true? Is it possible for ire_ipif->ipif_ill and ire_stq->q_ptr to go to different ill_t instances? I thought that was the case only for IPMP. (Maybe usesrc/vni is a factor here ...) 1351-1352,1436-1437,1908-1909: odd that these design constraints aren't assertions. 1749-1759: this comment is all wet. 2095-2096: should this be an assertion, or maybe just removed? The deleted comment said that we should "never be here for onlink." How do we get here in a case where we have to match the ill? The function is named "ire_ihandle_lookup_offlink_v6" ... shouldn't the ire be offlink and thus lack ire_ipif? 2131-2133: nit: duplicated code; could perhaps do more simply with: match_flags &= ~MATCH_IRE_HANDLE & ~MATCH_IRE_MASK; usr/src/uts/common/inet/ip/ip_ftable.c 160-161: more odd things that might be assertions. 290-298: comment no longer seems to have much to do with the adjacent code. (This is another case where the previous comment said that we should *not* be doing MATCH_IRE_ILL ... but the new code uses that flag. Is this right?) 722-723: nit: this does nothing. usr/src/uts/common/inet/ip/ip_if.c 192: this doesn't exist. 475: removing the "unused" IPMP flags means that they can't be reported in the ndd output. It's unclear to me whether the flags are eventually going away completely (and thus the IFF_* ones should be scratched too) or if this is potentially broken. 1496: nit: still-valid comment removed. 1509-1513,1602-1604: is this still true? It was true with IPMP, as the previous comment said, but what makes it still true after IPMP is gone? (usesrc?) 1515-1517: a simpler check would be just for IRE_MARK_CONDEMNED. Note that ill_down already walks all the ire_ts before walking the conn_ts, so we're guaranteed here that if conn_ire_cache is still non-NULL, and it points to this ill_t, it'll already have been condemned by ill_downi(). Simpler still: we could just use conn_cleanup_stale_ire. 1586: second argument is void *, not char *. 3875-3876: comment is still stale; there's no "for each" here. 4751-4757: why is this a loop over all phyints? This should just be a simple AVL lookup by index now, shouldn't it? 4971-4975: this is "lo0" that we're plumbing here ... how can its ipsq be merged with anything else? Isn't there at most one phyint for lo0? (Or am I misunderstanding how v4/v6 now works ... ?) 11344: this is new functionality ... but it looks right to me. 13516: "some assembly required" 14278: where are the others? Wasn't ill_move the only culprit? 15476-15479: no longer relevant; there are no more jumps to 'bad' past this point. 15489: this assignment doesn't do anything; the flag isn't used until 'bad', and the last jump to that branch was at line 15469. 15584: this can't be true due to the group formation removal. 15833: small nit: should exclude any IPIF_DEPRECATED interfaces from ipif_allzones. They'll be picked up at line 15842 if they're usable at all, and we should stick with non-deprecated for the allzones choosing logic. 15854: this doesn't necessarily look good to me, though I might be missing something. At one point, we had code here that intentionally used ipif_rand() to choose among a set of equally viable ipif_t entries for a source address. Now we don't, even if the user creates many of them. It looks to me like the assumption here is that the _only_ reason you'd ever create multiple ipifs on the same subnet is to run IPMP. Do we know that this is true ... ? usr/src/uts/common/inet/ip/ip_ire.c 2058: nit: cast not needed. 4123-4124: similar questions about MATCH_IRE_ILL as with v6; original comment seemed to say that this couldn't happen. 5520-5521: do any of these 'in-lines' still exist? If not, then does this need to be done here? usr/src/uts/common/inet/ip/ip_mroute.c usr/src/uts/common/inet/ip/ip_multi.c Reviewed; no comments. usr/src/uts/common/inet/ip/ip_ndp.c 570,586: not your code, but cast not needed. 1454: ipif_state_flags, right? (Are you fixing this with your wad?) 2041-2047,2087: remove this junk; it was part of the group-handling loop that used to be here. The passed-in 'ill' pointer is ref-held by the caller if needed, so none of this code is now necessary. 2755-2770: this code doesn't seem to do much. Maybe this whole function can go away. usr/src/uts/common/inet/ip/ip_netinfo.c usr/src/uts/common/inet/ip/ip_opt_data.c usr/src/uts/common/inet/ip/ip_rts.c usr/src/uts/common/inet/ip/ipclassifier.c usr/src/uts/common/inet/ip/spd.c usr/src/uts/common/inet/ip6.h Reviewed; no comments. usr/src/uts/common/inet/ip_if.h 100-103,129: unused; could be removed. IPMP flags are not examined within the kernel on phyint or ipif anymore. (I don't much care if they remain, though it might be nice to have some way of making sure we don't accidentally start using them again.) 135-139: looks like this can become a boolean now. usr/src/uts/common/inet/ip_impl.h 397: nit: while you're here, s/Definitons/Definitions/ usr/src/uts/common/inet/ip_ire.h 97: nit: unused value is 0x0800, not 0x1000. usr/src/uts/common/inet/ip_multi.h Reviewed; no comments. usr/src/uts/common/inet/ip_ndp.h 144: NCE_F_UNSOL_ADV doesn't seem to be used anymore. usr/src/uts/common/inet/ip_stack.h This change will break 'pidentd' and probably other open source tools that grovel in kernel structures. We'll probably need to contact the authors so they know what to do. usr/src/uts/common/inet/ipclassifier.h usr/src/uts/common/inet/ipsec_info.h Reviewed; no comments. usr/src/uts/common/inet/tcp/tcp.c 4975-4978: entire comment is probably uninteresting now. 19388: doesn't this leak a reference to conn_outgoing_ill? The conn_set_outgoing_ill() function returns a ref-held ill_t there, but the variable at 19386 seems to go out of scope without the reference being dropped. usr/src/uts/common/inet/tcp/tcp_opt_data.c usr/src/uts/common/inet/udp/udp.c usr/src/uts/common/inet/udp/udp_opt_data.c usr/src/uts/common/ipp/ipgpc/classifier-objects.h usr/src/uts/common/ipp/ipgpc/classifier.c usr/src/uts/common/ipp/ipgpc/classifier.h Reviewed; no comments. usr/src/uts/common/ipp/ipgpc/classifierddi.c 455: nit: s/fields/field/ 458: nit: s/and if_group// usr/src/uts/common/ipp/ipgpc/filters.c usr/src/uts/common/ipp/ipgpc/ipgpc.h usr/src/uts/common/netinet/in.h Reviewed; no comments. -- James Carlson, Solaris Networking <james.d.carlson at sun.com> Sun Microsystems / 35 Network Drive 71.232W Vox +1 781 442 2084 MS UBUR02-212 / Burlington MA 01803-2757 42.496N Fax +1 781 442 1677
