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?
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.
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?
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]
