On Sun, 2009-08-16 at 22:53 -0700, Peter Memishian wrote: > [ Most comments elided as no further discussion is needed. ] > > > > Comments relative to http://cr.opensolaris.org/~seb/clearview-iptun-cr2/ > > > > > > uts/common/net/if.h: > > > > > > * 684-731: Seems like all this old iftun_req-related stuff can go. > > > > ACCEPT: It can, and it was removed at one point in the past. An hg > > merge brought it back to life. :-( > > I had this same problem during Clearview IPMP. The only solution I found > was to run cronjobs that looked for "old" terminology (e.g., iftun). Sad > it had to come to that.
Okay, I'll whip up something similar. > > > 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. > > > * 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. > > > * 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). > > > 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!). 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. 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. 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. -Seb [1] Current statistics of tunnelbroker.net (http://tunnelbroker.net/usage/tunnels_by_tserv.php) shows over 3226 active tunnels in their most active location, but they don't divulge how many actual routers are serving up these tunnels in that location. They do claim that locations operate more than one tunnel broker router to distribute load.
