Sebastien Roy wrote:
Here are my initial set of comments. I'll hopefully have more on Monday:
Here are the rest of my comments: usr/src/uts/common/inet/ip/ip.c 12665: Take this comment however you wish, but personally, I don't like having two entirely disjoint paths through ip to do forwarding (one fast and one slow). This kind of split brained functionality makes it difficult to integrate features into the stack, which proliferates complexity. It also makes tracing and debugging difficult. At the very least, it would be benefitial to have a single point at which IPv4 packets leave the ip module (ip_xmit_v4()). Here, we introduce yet another place where a direct putnext() is done. Moreover, it seems like calling ip_xmit_v4() may not be a performance problem based on the comment on line 12775. 12677: A local uint8_t * variable pointing to the IP header would make me feel better. 12700: This comment is slightly confusing. It says that we're going back to the slow path, but there's no alternate path being taken here. We're just adding an ire and continuing along the same path. 12716: I'm confused with this comment. It says ip_rput_forward(), but the block of code following the comment really calls ip_rput_process_forward(). It would also be nice if the comment could explain what the conditional is trying to do. It looks like under some of the conditions here, we should just be dropping the packet, no? 12749-12784: I don't understand why there's a separate case 1 and case 2. ip_wput_attach_llhdr() does everything that's in case 1 (plus some hand wavy IPQOS thingies), so why bother with case 1? Why not just always call ip_wput_attach_llhdr()? 12757: You're setting ipha to the beginning of the link-layer header? ipha isn't used after this, so I don't see the point. Shouldn't we be setting mp->b_rptr here instead of ipha? 12775: Has this been resolved? 12809: It would be good to add a comment to this function explaining what the difference is between it and ip_fast_forward(). 12841: I don't see what IRE_IF_NORESOLVER has to do with whether an ire has ire_rfq set of not. Looking at all of the code that sets ire_rfq, it would seem that all ire's have this set to something except for loopback ire's. I also don't see why Surya would have anything to do with this. Why "Surya note"? 12967: It would be cleaner to set *irep to NULL at the beginning of the function so that you don't have to set it to NULL in every error case. 12996: Why was this removed? 26965: I'm not a fan of these kinds of debug statements. Turning on ip debug level one will now result in a large amount of indistinguishable spewage, which is not at all useful. I'd rather have debug statements only for special conditions that are hard to reach with dtrace. This particular debug statement can be duplicated with a few lines of dtrace for those that care. usr/src/uts/common/inet/ip/ip_ftable.c 692: This is also called from ip_fast_forward(). -Seb _______________________________________________ networking-discuss mailing list [email protected]
