Some comments inline. Also, I looked at the rest of the SCTP files and the
changes look fine to me, except for a couple of nits.
usr/src/uts/common/inet/sctp/sctp_cookie.c
L814: change printf() before putback?
usr/src/uts/common/inet/sctp/sctp_error.c
L289,444: change printf() before putback?
usr/src/uts/common/inet/sctp/sctp_hash.c
usr/src/uts/common/inet/sctp/sctp_heartbeat.c
usr/src/uts/common/inet/sctp/sctp_init.c
usr/src/uts/common/inet/sctp/sctp_ioc.c
usr/src/uts/common/inet/sctp/sctp_notify.c
usr/src/uts/common/inet/sctp/sctp_output.c
usr/src/uts/common/inet/sctp/sctp_param.c
usr/src/uts/common/inet/sctp/sctp_shutdown.c
usr/src/uts/common/inet/sctp/sctp_snmp.c
usr/src/uts/common/inet/sctp/sctp_stack.h
usr/src/uts/common/inet/sctp/sctp_timer.c
usr/src/uts/common/inet/sctp_ip.h
usr/src/uts/common/inet/sctp_itf.h
No comments
usr/src/uts/common/inet/sctp/sctp_input.c
L357: True, it is not required.
usr/src/uts/common/inet/sctp/sctp_opt_data.c
L600-601: SCTP instead of TCP
L648,653: in theory the max len would be the max number of
logical addresses that can be/is plumbed * struct sockaddr_in6
(for both v4 and v6). In practice, you can get the nladdrs
and * struct sockaddr_in6.
L1061: Is SCTP in secs now after this?
-venu
On Tue, 6 Oct 2009, Erik Nordmark wrote:
venugopal iyer wrote:
Hi, Erik:
I looked at a few SCTP files and here are some comments. Will look at the
rest of the SCTP changes get back by Mon.
Thank you for the review comments.
usr/src/cmd/mdb/common/modules/sctp/sctp.c
L581: is crb_recvdstaddr this really used?
I just changed it from sctp_recvdstaddr to use the conn_t. Looks like
sctp_recvdstaddr isn't used in onnv-gate. Should I remove it?
If it is not used, let's remove it and if it is something SCTP is missing
let's have an RFE.
L710: Yes.
L957-970 : connp instead of sctp->sctp_connp?
Will fix.
usr/src/uts/common/inet/sctp/sctp_impl.h
L900-903 (old code) : could removing this cause any
overflow/deadlock
issues etc. i.e in a loopback case could this thread send the packet
and when it gets to the receiving SCTP can it continue (assuming
sctp_recvq is null) and then get back with the response..
handshake?
could this exchange lead to stack overflow? Also, when we call
sctp_output from sctp_sendmsg we now has sctp_running set, when
the
loopback thread (in the above scenario) comes back we'd probably
queue the packet in the receive queue and ask the task queue to
process it, but will then wait in sctp_process_recvq since
sctp_output
has still not returned to WAKE_SCTP, is that so? Did we check
loopback
tests? Sorry if I am missing something obvious.
We talked about this on the phone, and I don't see how sctp would use any
more stack than tcp; the tcp squeue allows re-entry once which means that
for loopback the same thread can pass the SYN and then carry the SYN-ACK
back. The sctp_running has means that a thread can pass e.g., an INIT and
then the INIT-ACK back.
Yes, per our discussion this is OK.
usr/src/uts/common/inet/sctp/sctp_asconf.h
usr/src/uts/common/inet/sctp/sctp_addr.h
No comments
usr/src/uts/common/inet/sctp/sctp.c
L217-219: Whatis is the reason for this assert here (i.e. in
sctp_create_eager)
Just to verify that sctp_inherit_values set things up. I'll remove the
assert.
L1056: why is this notyet?
Because sctp, unlike tcp, doesn't have a minmss knob.
It probably should have one. Should I file a CR against Nevada?
I think so.
L1070-1072: When sending a packet out we use info. from the
appropriate faddr, right? So, this should be handled correctly?
OK, I'll remove the comment.
L1154,1253: What is this comment for?
I'll remove the comments.
TCP retransmits a packet since it knows a packet left the network. TCP does
this by calling
tcp_rexmit_after_error(tcp);
Presumably sctp could do the same.
usr/src/uts/common/inet/sctp/sctp_addr.c
usr/src/uts/common/inet/sctp/sctp_asconf.c
usr/src/uts/common/inet/sctp/sctp_bind.c
No comments
usr/src/uts/common/inet/sctp/sctp_common.c
L648: We just heard from the fp (i.e. it is alive), so what could
result in fp->state being unreach?
The comment we added says:
644 * Note that if we didn't find a source in
sctp_get_dest
645 * then we'd be unreachable at this point in time.
If you look in onnv-gate or in refactor-review there is a code path where
sctp_get_ire/sctp_get_dest calls sctp_set_saddr and fp->state gets set to
SCTP_FADDRS_UNREACH.
Thus it is possible to hit the assertion that we removed.
I don't know why nobody has tripped on that assertion in onnv-gate; perhaps
it hasn't been tested as much.
L948-951: (related to comments L1070-1072 in sctp.c) should this
be w.r.t the fp we are using to send the packet?
sctp_build_hdrs merely creates a template.
Other places like sctp_make_mp() sets DF based on the faddr/ip_xmit_attr_t
used, and sctp_set_faddr_current sets it based on the sctp_current/
Thus the code in sctp_build_hdrs might be unnecessary; I can try removing it
and check that DF is still being set.
usr/src/uts/common/inet/sctp/sctp_conn.c
L241: will we add an assert here before putback?
I'll check with the trusted folks. But none of the 8 places where we have a
printf and XTX comment about ira_tsl have printed anything on the systems we
use for TX testing. But I don't know how much of SCTP they test.
L580: I think so.
OK
Erik
_______________________________________________
networking-discuss mailing list
[email protected]
