Sebastien Roy wrote:
* 862: This can't happen because we call iptun_unbind() before
iptun_bind() if the tunnel is already bound. I suppose you could
optimize things to make the iptun_unbind() unnecessary if you want.
I'll remove 862.
Perhaps an ASSERT() as well?
Where would that belong? in ipcl_iptun_hash_insert_*? (I checked and
there isn't one - it removes it is it already inserted.)
* 870,872: We should just have an ipcl_conn_insert() that calls
ipcl_conn_insert_v[4|6]() depending on conn_ipversion.
tcp_conn_request() and conn_connect() both have the same little
if/else dance.
OK, but for uniformity it makes sense to do the same for ipcl_bind_insert.
Yes.
Done
* 1795: There are some places in the code where ip_get_pmtu() is
called with an argument of connp->conn_ixa directly without first
getting a safe reference using conn_get_ixa() (in icmp_icmp_input()
for example). Is that deliberate?
We only call ip_get_pmtu() in one place, but there are places where we
use conn_ixa directly. However, currently that is only in iptun_create().
Where did you see it be used?
The question was more about the usage in icmp_icmp_input(), where I had
noticed conn_ixa directly. Is the usage there safe?
It (and udp_icmp_error) should use conn_get_ixa.
* 1976: What is this call to ip_attr_connect() about?
If ixa_ire was NULL we got a safe copy.
But I think I'll change the ixa_notify upcall to pass up the
ip_xmit_attr_t; that way we will have the currently used ip_xmit_attr_t
and can modify it directly which avoids the conn_get_ixa in
iptun_get_maxmtu.
Good idea.
Done
* 2093: The ICMP error doesn't need to have the same cred as the
original packet?
I read RFC 791 and there aren't credentials for IP packets ;-)
What is the purpose of the cred on the packet?
We use them for getpeerucred() and SCM_UCRED. They have also been
overloaded to carry a label, but with refactoring we have the explicit
ixa_tsl for the label.
I'm simply concerned that the TX label associated with the ICMP error
will be correct. If your code ensures that, then that's fine.
I've changed it to carry the ira_tsl into the iptun_icmp routines and
use that for the ixa_tsl for the generated ICMP error.
* 2781: Can we use ira_ip_hdr_len to make this faster and simple?
It will be a bit faster at the expense of duplicated code. The
duplication would be to have
outer_hlen = ira->ira_ip_hdr_length;
if (ira->ira_flags & IRAF_IS_IPV4) {
outer4 = (ipha_t *)data_mp->b_rptr;
outer6 = NULL;
} else {
outer4 = NULL;
outer6 = (ip6_t *)data_mp->b_rptr;
}
if (MBLKL(data_mp) == outer_hlen) {
inner_mp = data_mp->b_cont;
ipha = (ipha_t *)inner_mp->b_rptr;
} else {
inner_mp = data_mp;
ipha = (ipha_t *)(data_mp->b_rptr + outer_hlen);
}
switch (IPH_HDR_VERSION(ipha)) {
case IPV4_VERSION:
if (inner_mp->b_wptr - (uint8_t *)ipha < sizeof (ipha_t))
goto drop;
inner4 = ipha;
inner6 = NULL;
break;
case IPV6_VERSION:
if (inner_mp->b_wptr - (uint8_t *)ipha < sizeof (ip6_t))
goto drop;
*inner4 = NULL;
*inner6 = (ip6_t *)ipha;
break;
default:
goto drop;
}
thus a subset of what iptun_find_headers does.
Is that what you want?
Not quite, I was thinking that iptun_find_headers() could take an
outer_hlen as input if that was already a known quantity. That would
make the IPv6 input path a tad faster in the face of extension headers,
which there are by default (the encapsulation limit option).
I can do that. (But I suspect the N pullupmsg being done for tunnels is
more a a performance issue than walking the headers.)
* 3183,3276: We call iptun_find_headers() twice in a row for 6to4
output processing.
The alternative would be to pass 5 more arguments (outer{4,6},
inner{4,6} and outer_hlen) to iptun_output_common(). Is that better?
I suppose not. I thought that perhaps this could be fixed by
reorganizing the calling chain, but it doesn't look that simple.
Things need to be a bit different to handle the ixa correctly,
unfortunately.
* 3246-3259: Can this be done before calling iptun_output_common() so
that iptun_output_common() becomes a tail-call?
ixa_refrele() can't be done until we are done with the ixa.
If you think the extra stack frame makes a big difference I can apply a
pragma inline.
Not a big deal; if it's not a trivial fix, I wouldn't go out of my way
for the stack frame savings.
#pgragma inline(iptun_output_common)
is just one line.
But has an impact if you want to use fbt with dtrace since the inlined
function doesn't appear as an fbt probe AFAICT.
But again, there are pullupmsg calls which drag down performance.
Erik
_______________________________________________
networking-discuss mailing list
[email protected]
