Here's a more formal reply to your more formal code-review comments. Sorry for being slow.
On Tue, Aug 07, 2007 at 04:37:05PM -0500, Nicolas Williams wrote: > - usr/src/uts/common/inet/ip/ip.c:7008-7012 > > I thought we didn't like comments with "XXX" in them nor the names > (or nicknames, such as KEBE, which I gatehr is your nickname :) of > developers in source files. Like I said, all KEBE comments are there to provoke reviewers. > If a re-reading of RFCs 3947/8 is needed then, why not do it? > > I don't think we care about keepalive drop stats, but we should know > if we do... I've put in a new ipdropper (esp_nat_t_ka) to track these. I will be treating 2 and 3 byte packets the same as 1 byte ones. (Best application of Postel's Law.) > - usr/src/uts/common/inet/ip/ip.c:7031,7045 > > I gather that ESP-in-UDP is fairly costly -- we re-compute the IP > header checksum for what appears to be purely local purposes only, > and we move around the ESP payload (ovbcopy(), right?). It's not for local purposes if you follow the bouncing packet. ip_proto_input() does the off-the-wire check. To be fair, I need only recompute the checksum if it's really ESP-in-UDP (vs. a zero-SPI UDP packet). Therefore, I'm moving the ip_csum_hdr() call into the if/else branch specifically for ESP-in-UDP. General question to the networking folks --> why do we spread IP header checksum computations across ip_*_input() functions instead of keeping it in ip_input()? Is it because of IP options being a possibility? > Probably no big deal since the systems using ESP-in-UDP (laptops, > desktops at home) are hardly HPC systems and probably have > bottlenecks other than the CPU, making this not a hot path, but maybe > a comment is worthwhile in case it ever becomes a hot path. I've also put a comment in. Once I test these changes, I'll update the webrev both internally and on OpenSolaris. Thanks! Dan _______________________________________________ networking-discuss mailing list [email protected]
