[ Elided everything that didn't merit further discussion. ] > > http://cr.opensolaris.org/~meem/clearview-noipmp-cr-diff1 > > The changes look good. > > > > 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 > > > > Well, I could preserve it if it's an issue, but I would tend to think we > > could just notify folks that it's gone, as there appears to be no evidence > > anyone has actually used it. (In general, I would expect the set of IPQoS > > selectors to be free to evolve from release to release.) > > In the possibly unusual case that someone uses both IPMP and IPQoS, I > wouldn't expect that previously-valid configuration files would > suddenly become invalid and fail to load on an upgrade. Having new > selectors and features show up is unsurprising from a user's point of > view, but having things break is not expected. > > I agree that IPQoS is and was a questionable feature at best with > dubious design and feature set, and I'm as anxious as any to have it > removed (I obviously didn't want it there at all in the first place), > but I think the administrative interface piece needs to be handled in > the same way as any other feature removal.
OK, I will explicitly mention its removal in the Clearview IPMP PSARC materials, and we'll send out a notice to customers. > > > 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. > > > > In general, I left symbols used by userland around because I didn't want > > to drag accompanying userland modifications into this wad (since in > > general IPMP userland is not being entirely rewritten). For instance, > > removing SIOCSIPMPFAILBACK means in.mpathd no longer compiles. > > OK; I see. > > I think it might have been been better to include some of the user > space removals as well. I assume that the above ioctl does in fact go > away with the final version of code, right? It does. In retrospect, it might've been worth doing the same thing with userland that we did with the kernel, though for the most part there's either just removed functionality (e.g., no explicit failover/failback logic) or new functionality (ipmpstat and supporting infrastructure), rather than a rewrite. > > > 1218: note: will likely need to scan for contract holders on > > > ipif_t. (CGTP and Cluster seem possible here.) > > > > I didn't find any contracts for any of the removed fields. > > It's not just the fields themselves, but the offset of any fields that > follow. There are contracts that include access to ipif_t -- see, for > example, 'contract-cgtp' in PSARC 2000/539. > > There well may be no issue here, but we're supposed to notify. OK, I'll notify 'em. > > > 7863-7867: as for IPv6, do we ever really need ire_ipif? > > > > Yes, VNI uses it. Moreover, it's part of the new Clearview IPMP design, > > so removing it would introduce more code churn in the Clearview IPMP gate. > > OK ... I guess that's one of the few hassles with reviewing a separate > removal and then rewrite. Yeah, as I mentioned in my initial email, there were a few places where I "cheated" because it was convenient for the new implementation. In several places, I could've taken it a little bit further and saved some confusion -- lesson learned for next time :-) > > > usr/src/uts/common/inet/ip/ip6_ire.c > > > > 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? > > > > I'd expect offlink IREs to often have an ire_ipif -- e.g., an IRE_DEFAULT > > for a route created with -ifp will have a matching ire_ipif->ipif_ill. > > I guess the basic question here is: why do we need to test ire_ipif? > It looks like we're painting around the assertions that the subsequent > lookup ends up making (if you set MATCH_IRE_ILL, you have to pass in a > non-NULL ill_t pointer), but unless these two cases (with and without > ire_ipif set) actually _mean_ something, it just looks sloppy to me. > > Why bother asserting inside the lookup functions that ill_t isn't NULL > if the callers just paint around that check by turning off the flag > when it is? I think the idea is that the caller is at least supposed to think about the case, but I agree it's a bit bondage-and-discipline. > > > usr/src/uts/common/inet/ip/ip_if.c > > > > 1496: nit: still-valid comment removed. > > > > I couldn't decipher what its point was, or why it didn't apply to any of > > the other similar cases. Could you? > > It's saying that we're going to force the socket to fall back to "late > binding" -- picking an output ill_t when the user transmits -- even > though the user explicitly specified a particular interface to use. > It's a silent failure case. > > I guess I don't care very much, but I was a little surprised to see > the comment go. OK, I'll restore it. > > > 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 ... ? > > > > I'm not sure I follow. Surely, separately from IPMP it can be useful to > > have multiple IP addresses on the same subnet and same ill (e.g., > > shared-stack zones). > > I'm not talking about cases like that, because we'd filter out on > zoneid first. I was talking only about the "regular" case -- no IPMP, > no Zones, nothing special like that; just two interfaces configured > with the same subnet. > > The new code seems to assume that either this configuration is never > used or that if it is used, then the first ipif_t we encounter is > always sufficient. Yes, it's assuming that's true when you ask the kernel to do the selection and the subnet matches your destination -- and I can't see a reason to have it work otherwise. That said, the Clearview IPMP source address algorithm treats the ipif list as circular and starts where it last left off (for IPMP ills) to "randomize" the source address selection. It'd be straightforward to do this for normal ills too, if need be. > > > > usr/src/uts/common/inet/ip/ip_ndp.c > > > > 2755-2770: this code doesn't seem to do much. Maybe this whole > > > function can go away. > > > > Unfortunately, these corner cases have to be added back in with Clearview > > IPMP, so I'd prefer to leave it there to reduce code churn. > > Really? OK. The only thing I see here is ip6i cruft that appeared to > be related to the old attach_ill bits, and I though that was mostly > being nuked. The attach_ill stuff is gone, but ip6i_t as a whole remains as it's used for other things. One of those other things is putting ND solicitations for IPMP probe packets at the head of the queue -- and to do that we need to identify that the IP packet as a probe using the ip6i_t. > > > usr/src/uts/common/ipp/ipgpc/classifierddi.c > [...] > > > 458: nit: s/and if_group// > > > > Fixed (and just when I'd thought I'd caught every misspelling ;-) > > Might need to leave one behind for posterity. ;-} I'm sure that'll happen by accident anyway :-/ -- meem
