Thirumalai Srinivasan wrote:
thiru-10a ipfil_sendpkt: If ipfil_sendpkt -> ire_route_lookup happens for a
destination that is onlink ire_route_lookup() would return an IRE_INTERFACE
and sire would be null. The check at L731 in ire_forward must check whether
supplied_ire is null to determine the caller.
RESP: CLARIFY. You are correct in that L731 in ire_forward is incorrect.
But sire may null ( or onlink host) and a non-null for offlink host. So
should L731 just check for a non-null ire(and not check on sire at all) to
determine that the caller of ire_forward is ipfil_sendpkt instead?
Yes, that is right.
RESP: ACCEPT
thiru-21 ire_get_bucket() assignment at L1047 is not required. This
actually happens in rn_newpair() at L495 radix.c. rdst and
There are 2 parts to my comment. The first part above is needed for
avoiding redundancy and confusion. The 2nd part below is a style issue,
so I will leave it to you. In both cases we don't expect any measurable
performance benefits.
initialization at L1030 - L1034 is not needed, instead
rt->rt_dst can be directly initialized at L1048.
RESP:INVESTIGATE Will see if above suggestion provides any perf benifits
RESP: We dont see any functional flaw in this, and so prefer to keep it
as is. Every other caller of the radix code follows the same steps here;
so if we ever pick up a patch for radix that makes assumptions about
initialization, we would run the risk of creating a bug here
thiru-22 ire_find_best_route() - Should not loop across the dupedkey chain.
That loop happens in rn_match_args().
RESP: ACCEPT
thiru-27. The code in rn_match_args L382 down should be as follows.
/*
* Although we found an exact match on the key, rn_leaf_fn
* is looking for some other criteria as well. Continue
* looking as if the exact match failed.
*/
b = 0;
goto keeplooking;
}
keydiff:
test = (*cp ^ *cp2) & 0xff; /* find first bit that differs */
for (b = 7; (test >>= 1) > 0; )
b--;
keeplooking:;
matched_off = cp - v;
b += matched_off << 3;
rn_bit = -1 - b;
The search key and the route matched completely in all bits upto
the netmask of the route, otherwise we would have gone to
'keydiff'. So the matched offset is really cp - v. The parent's
rn_offset represents the byte in which the search key is being
tested, and that is not relevant.
RESP: ACCEPT
thiru-30 Given the function pointer table rnh_* for the
lookup/add/delete operations, it is not good to also have
direct calls to parallel lookup functions that are not listed
in the table. rn_match is the table driven function. But
rn_match_args is used privately by IP. One solution is to add
another operation say rnh_matchaddr_args to the function table.
RESP: CLARIFY What would the extra level of indirection buy us?
Modularity, abstraction / encapsulation and uniformity.
I believe you want to hide the entire radix tree related code and data
structures as an abstraction to clients and let clients access the radix
tree via well defined entry points which are routed through a function table
pointer mechanism rather than letting them make direct calls into the
radix tree implementation.
RESP: ACCEPT
thiru-31 ire_forward: The block at L876 for the IRE_CACHE is not very
useful. If the interface route is deleted the ire cache will
also be eventually deleted through the ihandle linkage. Instead
we should return much earlier in block L794 down, irrespective
of whether the nce_state is reachable or not.
RESP: CLARIFY.
Are you suggesting that the ire_ihandle_lookup_offlink() at L882 is
useless or that it should be moved up somewhere else?
Iam suggesting it can be removed. It is needed only if you want to create
a long lived IRE_CACHE for the offlink destination based on the IRE_CACHE
of the gateway. But that is not the case.
RESP: ACCEPT
thiru-33 It is good to avoid dropping packets in the outbound side and instead
queue them and enable flow control if canputnext fails. In ip_wput_ire()
we check for canputnext at label blocked: and subsequently just do putnext.
Now the replacement of putnext with ip_xmit_v4, causes another check for
canputnext and then drop the packet. Instead ip_xmit_v4() must be passed
another boolean parameter. If this is true, we do the canputnext check
in ip_xmit_v4 and drop the packet (eg. forwarding path). If it is false
we just call putnext. (eg. ip_wput_ire path).
RESP: CLARIFY
There are two calls to ip_xmit_v4() in ip_wput_ire(0: one is at L27105
before the "blocked" label at 20804, the other is after it at L21118
Furthermore there is canput logic in ip_xmit_v4() itself in L27027
Are you suggesting the following changes?:
o We add another bootlean arg (lets call it "canputnext_check") to
ip_xmit_v4()
and change code at ip_xmit_v4() such that if this new arg is B_TRUE then
we
do the canputnext at L27027 of ip_xmit_v4, if B_FALSE the skip the
caputnext check
and just do a putnext.
o Have all callers of ip_xmit_v4() set it as B_TRUE *except* the two
callers of ip_xmit_v4() from ip_wput_ire(L27105, 21118)
I suppose this way we wont be doing double canput check in:
ip_wput_ire->ip_xmit_v4
Yes, that is correct.
but other than the redundancy, is there a flaw in doing canput check twice?
You would end up dropping packets instead of flow controlling and blocking
the application (unlike onnv) in case of say UDP apps.
RESP: ACCEPT
_______________________________________________
networking-discuss mailing list
[email protected]
