Sebastien Roy wrote:
Sebastien Roy wrote:
Here are my initial set of comments. I'll hopefully have more on Monday:
Here are the rest of my comments:
Sebastien,
Thanks for code reviewing.
We have collated all your comments, and our responses in the included
text file. We need clarifications/suggestions for these:
seb-13, seb-20, seb-22. seb-23, seb-24
Thiru,
Please provide input on seb-8
Sangeeta
*********************************************
usr/src/uts/common/inet/ip/ip.c
*********************************************
seb-1: 6548: This debug statement generates useless noise. dtrace
gives you this information if you want to know it...
RESP: Accept
seb-2: 6555: Printing a debug statement for every packet
successfully sent is a bad idea. Lots of noise, little signal.
Again, people can easily use dtrace to get this information (and more).
RESP: Accept
seb-3: 6984: forwarding a packet
RESP: Accept - will fix comment
seb-4: 7333: might as well rename that local variable to res_mp
and fix the comment above.
RESP: Accept
seb-5: 7336: fix comment
RESP: Accept
seb-6: 7651: should be NULL
RESP: Accept (also flagged in jdc-38)
seb-7: 12661: Has this been addressed?
RESP: Accept. Yes it was flagged by Jim ( jdc-45) will remove
seb-8: 13534-13548: I don't think I agree with moving this out of ip_input(). Is
the idea that Nemo polling can't be enabled today if CGTP is in use, and
therefore CGTP can be optimized out of ip_input? To me, that's an
implementation flaw with TCP fast-path. When that flaw is fixed, someone
is going to have to yank that code back into ip_input()... Plus,
overloading the hdrlen argument to ip_input() in this way is just plain
wrong. When CGTP is not enabled, you're only saving a couple of
instructions by moving this code here, and that's not a significant
saving.
RESP: Will investigate.
NOTE: Thiru, need input
seb-9: 13752: Has this been addressed yet? Clearview is planning on removing
Mobile IP from Solaris, but if you want to just do it now, that's fine
too. :-)
RESP: We will remove the comment. But I am not sure if Surya can rip
out MIP code without getting PSARC paperwork. So Clearview can remove
existing MIP code when its ready.
seb-10: 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.
RESP: Will investigate if removing SEND_PKT macro and calling ip_xmit_v4
at L12763 and 12781 in ip_fast_forward causes significant perf dips.
seb-11; 12677: A local uint8_t * variable pointing to the IP header
would make me feel better.
RESP: Accept
seb-12: 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.
RESP: Accept will fix comment as its inaccurate
seb-13: 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?
RESP: Please clarify as to under what conditions packets should
be dropped?
THe comment Line on L12718 should state:
ip_rput_process_forward->ip_rput_forward->ip_xmit_v4
and the exception case should be explained better.
seb-14: 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()?
RESP: Will investigate if inlining this has any noticible perf beneifits,
If not, will collapse case 1 to case 2 , ( or even better call
ip_xmit_v4()
seb-15: 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?
RESP: Accept. As pointed out by jdc-50, we should get rid of both
L12757 and L23125
seb-16: 12775: Has this been resolved?
RESP: No. Will investigate( as stated above in seb-14)
seb-17: 12809: It would be good to add a comment to this function
explaining what the difference is between it and ip_fast_forward().
RESP: Accept
seb-18: 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"?
RESP: Will investigate and adjust comment.
seb-19: 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.
RESP: Accept, assuming you mean we do this:
12952 q = *qp;
12953 ire = *irep;
+ *irep = NULL
seb-20: 12996: Why was this removed?
RESP: Please clarify. In doing a diff of this code fragment with
its parent onnv , I dont see anything missing.
seb-21: 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.
RESP: Accept - will use dtrace instead
*********************************************
usr/src/uts/common/inet/ip/ip6.c
*********************************************
seb-22: 4858: I don't understand this comment. The IPV6_ADDR_LEN
check in itself doesn't tell the driver anything. What
did you mean to say (and please re-format the comment block)?
RESP: Accept - will reformat.
At one point we had questions on why this code is necessary. I would
still like clarification on this.
NOTE: NEED INPUT
seb-23: 4881: What if ill_phys_addr_length is 0, will this
do the right thing? This code path will also be used for
Ethernet, or any other media that has a MAC address size != 16 bytes...
Passing in NULL as a next-hop address doesn't seem right at all.
It seems like this change would make it impossible for IPv6 to
work with a driver that doesn't support IP fast-path.
RESP: Offer suggestion on what it should be
seb-24: 5799-5819: Same two comments as above.
RESP: Offer suggestion on what it should be
*********************************************
usr/src/uts/common/inet/ip/ip_ftable.c
*********************************************
seb-25: 692: This is also called from ip_fast_forward().
RESP: Accept - will fix comment
*********************************************
usr/src/uts/common/inet/ip.h
*********************************************
seb-26: 326-329: What's the point of putting this under the
protection of ifndef?
RESP: Accept. There is no reason other than being with the other defs
IRE_DB_REQ_TYPE & IRE_DB_TYPE)
seb-27: 327: While I'm no STREAMS expert, this practice of hijacking obsolete
STREAMS message types for ip's internal use seems dangerous and broken.
Maybe someone with more expertise in this area explain why we can't just
add message types in <sys/stream.h> (if a new message type is really the
way to go)?
RESP: We are open to input in alternate schemes.
The need for the IRE_ARPRESOLVE_TYPE came from the fact
that the ARP request sent via the forwarding path is
slightly different than what is done on the hostpath(ie IRE_DB_TYPE).
We needed a way to differentiate between the two at ip_wput_nondata,
so the post-ARP processing can be done differently for each.
seb-28: 1545-1554: From the descriptios, it seems like lots of these could be
simultaneously true. For example SEND_FAILED because
LLHDR_RESLV_FAILED. Or LOOKUP_PASSED and LLHDR_RESLV_PASSED are
almost always true. Could the block comment be expanded to
explain which states are valid when?
RESP: Accept: Will expand comments.
seb-29: 1545-1554: SEND_IN_PROGRESS, LOOKUP_PASSED, LOOKUP_FAILED, and
LLHDR_RESLV_PASSED are never used anywhere.
RESP: Currently we would like to use those values for debugging
purposes as to why it failed. But at a later point
it would be useful as return values:
SEND_IN_PROGRESS, LOOKUP_PASSED, LOOKUP_FAILED LLHDR_RESLV_PASSED
for Crossbow APIs (ie info from the forwarding engine that
Crossbow may want).
seb-30: 1554: status_t sounds too generic a name to me. How about
ipxmit_state_t?
RESP: Accept will change to ipxmit_state_t.
seb-31: 2084: I'd remove the "(ill_1) == (ill_2)" short circuit, as this is an
optimization for a condition that almost never happens. Removing this
check makes the comparison in the more typical case faster by not having
to compare values that are almost always different.
RESP: Will investigate This macro is used in the forwarding fastpath
to see if it needs to be kicked into the slowpath.
_______________________________________________
networking-discuss mailing list
[email protected]
