> > > > Comments relative to > > http://cr.opensolaris.org/~seb/clearview-iptun-cr2/ > > > > > > > > uts/common/inet/ip/ip_if.c: > > > > > > > * 1084-1087: Dead code/comments concerning tunnel ioctls. > > > > > > ACCEPT > > > > > > > > > > > * 1194, 1997: Likewise. > > > > > > ACCEPT: 1194 and 1197, yes. There also appears to be a few more > > > instances of processing M_IOCTL. Were those all guaranteed to be the > > > tunnel ioctls that I removed, or can there be others? E.g, other all > > > other *_pending_mp_*() functions, and ipsq_xopq_mp_cleanup(). I'm > > > guessing that those can go as well... > > > > The ipsq_xopq_*() functions are still needed -- those clean up the > > ipsq_pending_mp_*() state, which is the main codepath. The > > ill_pending_mp_* codepaths are still used by SIOG*ARP. The comments > > indicate that SIOCG*ARP will only enqueue M_IOCDATAs but a cursory > > inspection of the code suggests it may enqueue M_IOCTLs; I think you'll > > need to do some testing here to find out what can be removed. > > Got it, thanks. I wasn't implying that the functions themselves could > go, but rather that their assertions regarding M_IOCTL possibly coming > through them might be obsolete. I'll verify and test.
Sounds good. > > > > * 18811: Not your doing, but how can we get here with a non-zero > > > > ill_sap, and why would we want to preserve the old ill_sap > > > > value? > > > I don't know, there seems to be quite of bit of voodoo involved in > > > setting ill_sap (it looks like it might get set three times to the same > > > value during the plumbing of an IP interface!). Where do we preserve > > > the old ill_sap value? > > > > The code only sets ill_sap if it's zero -- so if ill_sap is already set to > > a value, that value is preserved. > > Ah, of course. I can't think of a reason why unconditionally setting > the sap here would be wrong. I'll remove the check for 0. Cool. > > > > * 19936-19939: Seems like a bugfix; is there a CR? > > > > > > ACCEPT: Ironically, it was a bug for all link types except for tunnels. > > > In the previous code, the ip module would intercept the M_IOCACK for the > > > old SIOCSTUNPARAM ioctl and change the link-local address by peeking > > > inside the iftun_req. It was a total hack-job. The hack is replaced by > > > promoting tunnel addresses to the link-layer addresses they really were. > > > When they change, a DLPI notification is generated by GLDv3 and the > > > right thing happens here. > > > > > > For other types of links, this wasn't handled correctly, and this is CR > > > 4289774. It looks like I need to work on this fix a tad more, though. > > > I need to make sure that if the token is manually set, that we don't > > > automatically change it from under the admin. > > > > It'd be nice to get that antique fixed along with this wad. > > Yes, and that's the intention, I simply neglected to include the CR int > he webrev. As I said, though, I need to make an adjustment to the fix > (something that prevents the system from automatically overriding > administratively set tokens and addresses). Understood. I was just making sure that you planned to cover this. > > > > uts/common/inet/ip/ip_classifier.c: > > > > > > > > * 311: 389 because? (Yes, I know it's prime -- but so are 383 > > > > and 397.) > > > > > > ACCEPT: Will add a comment, but I'm not sure what to say other than > > > "it's a good enough prime number"... > > > > Why we think it's big enough would also be useful. Given that we > > generally strive for hashes with an average bucket length of 1, > > 383 seems a bit small, unless there's some dynamic resizing somewhere. > > It's a valid point. There is no dynamic resizing of classifier tables, > and arguably, ipclassifier.c ought to have such a generic mechanism for > all of the tables. > > For now, we either have to pick a value based on our understanding of > how the feature is likely to be used, or do something like the TCP table > (ips_ipcl_conn_hash), which is sized according to available memory (my > desktop has an ips_ipcl_conn_fanout_size of 98229!). Wow. > The latter makes sense for TCP connections given that TCP is almost > guaranteed to be widely used on every system. Not so much for IP > tunnels. I suggest picking a reasonable size for now until we get > smarter about resizing classifier tables. Sure. > As for what constitutes a reasonable size, for the case of a tunnel > concentrator such as a VPN server or tunnel broker, perhaps thousands of > tunnels isn't out of the question[1], and your observation that 389 > being too small is valid. Perhaps increasing that ~20x or so to 6143 > would be more than adequate. Yes, more than adequate (assuming a good distribution, of course -- and I know you're checking into that already). > For a bit of amusing context, remember that IP tunnels were previously > in a linked list (!) because they all hashed based on protocol number > (and not addresses) in a single "raw socket" hash bucket in > ips_ipcl_proto_fanout (ip_fanout_proto()). This is CR 4627970. Embarrassing that one's been around for so long. -- meem
