On Tue, 2009-09-29 at 14:35 -0700, Erik Nordmark wrote:
> 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.)
I was thinking of ASSERT(connp->conn_fanout == NULL) in iptun_bind(),
but probably not strictly necessary.
> >>> * 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.
Okay, that makes sense.
> >>> * 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.
Yep, looks good.
> >>> * 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
...
> >> 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.)
I believe that there is no pullupmsg() done at all in the input data
path (only in the ICMP input path and in the output path). I'm not
sure, in practice, how often multi-block messages come down in the
output path though.
> >>> * 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.
Okay.
>
> >>> * 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.
Indeed. I'm not sure that it's worth doing.
Thanks,
-seb
_______________________________________________
networking-discuss mailing list
[email protected]
