Sangeeta Misra wrote:
*********************************************
usr/src/uts/common/inet/ip/ip.c
*********************************************
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.
Right, that side comment was tongue in cheek. :-)
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.
Great.
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?
If ILLF_ROUTER isn't set on either ill's, or if the ttl has expired, for
example. In those cases, I don't see the benefit to deferring dropping
the packet until we're in ip_rput_process_forward(). In addition to
that, I'm not exactly asking that you duplicate the code from
ip_rput_process forward, that would be bad. It would be better to have
a single piece of code that verifies these conditions, instead of having
duplicate blocks of code living in both ip_fast_forward() and
ip_rput_process_forward().
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.
Ok.
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()
Ok.
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.
Ok.
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
Yes.
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.
I had the line numbers mixed up. Line 12996 in the parent was the comment:
/* Copy b_next - used in M_BREAK messages */
That should be after line 13275 in the new file, but the comment was
removed. What is b_next used for if not for M_BREAK? Could the comment
be updated instead of removed?
*********************************************
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
I've always believed that the IPV6_ADDR_LEN check is (and always has
been) dead and incorrect code. It's been there since Solaris 8, and
it's always been wrong. Looking through tun.c, automatic and 6to4
tunnels think they're being clever by advertising a physical address
length of IPV6_ADDR_LEN (line 970), but this never results in anything
useful. The first thing that tun does when getting a DL_UNITDATA_REQ
from ip is throw away the first mblk_t that contains the clever gateway
inserted by ip_newroute_v6()!!! This block can just be removed, leaving
just the second block that you have changed. In addition to that, the
tun module does IP fast-path, so this funky dlureq_mp mblk is never used
anyway.
In any case, this will be re-written for Clearview, when tunnels will
behave more like normal link-layers that have real physical addresses of
the proper lengths. I wouldn't sweat it if you decide to leave that in.
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
I was wrong about this, as this is IRE_IF_NORESOLVER. Your code is
correct here. Reject this comment.
seb-24: 5799-5819: Same two comments as above.
RESP: Offer suggestion on what it should be
Since these are for IRE_IF_NORESOLVER, seb-23 was incorrect, and should
be rejected.
*********************************************
usr/src/uts/common/inet/ip.h
*********************************************
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.
I don't have any bright ideas other than to re-design the entire arp/ip
interface. Feel free to "Defer" until that can be done. ;-)
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).
I don't understand. You mean that you're defining these now, but
they'll be used later? Why not just leave them out, and have more
states added later when they're needed? I don't think you'll be running
into any binary compatibility issues here since this is a private interface.
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.
Right. Removing the first clause makes the macro faster for the
fast-path case, which is what you want, and it would still be logically
correct.
-Seb
_______________________________________________
networking-discuss mailing list
[email protected]
