| Message ID | 20120724113802.GA30142@hmsreliant.think-freely.org |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On 7/24/12, Neil Horman <nhorman@tuxdriver.com> wrote: > On Tue, Jul 24, 2012 at 09:50:18AM +0800, xufeng zhang wrote: >> On 07/23/2012 08:14 PM, Neil Horman wrote: >> >On Mon, Jul 23, 2012 at 10:30:34AM +0800, xufeng zhang wrote: >> >>On 07/23/2012 08:49 AM, Neil Horman wrote: >> >>>Not sure I understand how you came into this error. If we get an >> >>> invalid >> >>>stream, we issue an SCTP_REPORT_TSN side effect, followed by an >> >>> SCTP_CMD_REPLY >> >>>which sends the error chunk. The reply goes through >> >>>sctp_outq_tail->sctp_outq_chunk->sctp_outq_transmit_chunk->sctp_outq_append_chunk. >> >>>That last function checks to see if a sack is already part of the >> >>> packet, and if >> >>>there isn't one, appends one, using the updated tsn map. >> >>Yes, you are right, but consider the invalid stream identifier's >> >>DATA chunk is the first >> >>DATA chunk in the association which will need SACK immediately. >> >>Here is what I thought of the scenario: >> >> sctp_sf_eat_data_6_2() >> >> -->sctp_eat_data() >> >> -->sctp_make_op_error() >> >> -->sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, >> >> SCTP_CHUNK(err)) >> >> -->sctp_outq_tail() /* First enqueue ERROR chunk >> >> */ >> >> -->sctp_add_cmd_sf(commands, SCTP_CMD_GEN_SACK, SCTP_FORCE()) >> >> -->sctp_gen_sack() >> >> -->sctp_make_sack() >> >> -->sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, >> >>SCTP_CHUNK(sack)) >> >> -->sctp_outq_tail() /* Then enqueue SACK chunk >> >> */ >> >> >> >>So SACK chunk is enqueued after ERROR chunk. >> >Ah, I see. Since the ERROR and SACK chunks are both control chunks, and >> > since >> >we explicitly add the SACK to the control queue instead of going through >> > the >> >bundle path in sctp_packet_append_chunk the ordering gets wrong. >> > >> >Ok, so the problem makes sense. I think the soultion could be alot >> > easier >> >though. IIRC SACK chunks always live at the head of a packet, so why not >> > just >> >special case it in sctp_outq_tail? I.e. instead of doing a >> > list_add_tail, in >> >the else clause of sctp_outq_tail check the chunk_hdr->type to see if >> > its >> >SCTP_CID_SACK. If it is, use list_add_head rather than list_add_tail. I >> > think >> >that will fix up both the COOKIE_ECHO and ESTABLISHED cases, won't it? >> > And then >> >you won't have keep track of extra state in the packet configuration. >> Yes, it's a good idea, but I think the premise is not correct: >> RFC 4960 page 57: >> "D) Upon reception of the COOKIE ECHO chunk, endpoint "Z" will reply >> with a COOKIE ACK chunk after building a TCB and moving to the >> ESTABLISHED state. A COOKIE ACK chunk may be bundled with any >> pending DATA chunks (and/or SACK chunks), *but the COOKIE ACK chunk >> MUST be the first chunk in the packet*." >> >> So we can't put SACK chunk always at the head of the packet. >> > Ok, Fair point, but that just changes the ordering a bit to: > COOKIE_ACK > SACK > OTHER CONTROL CHUNKS > > What about something like this? Its completely untested, and I'm sure it > can be > cleaned up a bunch, but this keeps us from having to add additional state to > the > packet structure. Yeah! I like this modification, thank you very much for your work! I'll try to send a V2 patch based on your changes and run some tests. Thanks, Xufeng Zhang > > > diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c > index e7aa177c..eeac32f 100644 > --- a/net/sctp/outqueue.c > +++ b/net/sctp/outqueue.c > @@ -300,7 +300,7 @@ void sctp_outq_free(struct sctp_outq *q) > int sctp_outq_tail(struct sctp_outq *q, struct sctp_chunk *chunk) > { > int error = 0; > - > + struct sctp_chunk *cptr; > SCTP_DEBUG_PRINTK("sctp_outq_tail(%p, %p[%s])\n", > q, chunk, chunk && chunk->chunk_hdr ? > sctp_cname(SCTP_ST_CHUNK(chunk->chunk_hdr->type)) > @@ -344,7 +344,21 @@ int sctp_outq_tail(struct sctp_outq *q, struct > sctp_chunk *chunk) > break; > } > } else { > - list_add_tail(&chunk->list, &q->control_chunk_list); > + list_del_init(&chunk->list); > + if (chunk->chunk_hdr->type == SCTP_CID_COOKIE_ACK) > + list_add_head(&chunk->list, &q->control_chunk_list); > + else if (!list_empty(&q->control_chunk_list) && > + chunk->chunk_hdr->type == SCTP_CID_SACK) { > + list_for_each_entry(cptr, &q->control_chunk_list, list) { > + if (cptr->chunk_hdr->type == SCTP_CID_COOKIE_ACK) > + continue; > + list_add(&chunk->list, &cptr->list); > + break; > + } > + } > + > + if (list_empty(&chunk->list)) > + list_add_tail(&chunk->list, &q->control_chunk_list); > SCTP_INC_STATS(SCTP_MIB_OUTCTRLCHUNKS); > } > >> >> Thanks, >> Xufeng Zhang >> >Regards >> >Neil >> > >> > >> > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 25, 2012 at 10:34:32AM +0800, Xufeng Zhang wrote: > On 7/24/12, Neil Horman <nhorman@tuxdriver.com> wrote: > > On Tue, Jul 24, 2012 at 09:50:18AM +0800, xufeng zhang wrote: > >> On 07/23/2012 08:14 PM, Neil Horman wrote: > >> >On Mon, Jul 23, 2012 at 10:30:34AM +0800, xufeng zhang wrote: > >> >>On 07/23/2012 08:49 AM, Neil Horman wrote: > >> >>>Not sure I understand how you came into this error. If we get an > >> >>> invalid > >> >>>stream, we issue an SCTP_REPORT_TSN side effect, followed by an > >> >>> SCTP_CMD_REPLY > >> >>>which sends the error chunk. The reply goes through > >> >>>sctp_outq_tail->sctp_outq_chunk->sctp_outq_transmit_chunk->sctp_outq_append_chunk. > >> >>>That last function checks to see if a sack is already part of the > >> >>> packet, and if > >> >>>there isn't one, appends one, using the updated tsn map. > >> >>Yes, you are right, but consider the invalid stream identifier's > >> >>DATA chunk is the first > >> >>DATA chunk in the association which will need SACK immediately. > >> >>Here is what I thought of the scenario: > >> >> sctp_sf_eat_data_6_2() > >> >> -->sctp_eat_data() > >> >> -->sctp_make_op_error() > >> >> -->sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, > >> >> SCTP_CHUNK(err)) > >> >> -->sctp_outq_tail() /* First enqueue ERROR chunk > >> >> */ > >> >> -->sctp_add_cmd_sf(commands, SCTP_CMD_GEN_SACK, SCTP_FORCE()) > >> >> -->sctp_gen_sack() > >> >> -->sctp_make_sack() > >> >> -->sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, > >> >>SCTP_CHUNK(sack)) > >> >> -->sctp_outq_tail() /* Then enqueue SACK chunk > >> >> */ > >> >> > >> >>So SACK chunk is enqueued after ERROR chunk. > >> >Ah, I see. Since the ERROR and SACK chunks are both control chunks, and > >> > since > >> >we explicitly add the SACK to the control queue instead of going through > >> > the > >> >bundle path in sctp_packet_append_chunk the ordering gets wrong. > >> > > >> >Ok, so the problem makes sense. I think the soultion could be alot > >> > easier > >> >though. IIRC SACK chunks always live at the head of a packet, so why not > >> > just > >> >special case it in sctp_outq_tail? I.e. instead of doing a > >> > list_add_tail, in > >> >the else clause of sctp_outq_tail check the chunk_hdr->type to see if > >> > its > >> >SCTP_CID_SACK. If it is, use list_add_head rather than list_add_tail. I > >> > think > >> >that will fix up both the COOKIE_ECHO and ESTABLISHED cases, won't it? > >> > And then > >> >you won't have keep track of extra state in the packet configuration. > >> Yes, it's a good idea, but I think the premise is not correct: > >> RFC 4960 page 57: > >> "D) Upon reception of the COOKIE ECHO chunk, endpoint "Z" will reply > >> with a COOKIE ACK chunk after building a TCB and moving to the > >> ESTABLISHED state. A COOKIE ACK chunk may be bundled with any > >> pending DATA chunks (and/or SACK chunks), *but the COOKIE ACK chunk > >> MUST be the first chunk in the packet*." > >> > >> So we can't put SACK chunk always at the head of the packet. > >> > > Ok, Fair point, but that just changes the ordering a bit to: > > COOKIE_ACK > > SACK > > OTHER CONTROL CHUNKS > > > > What about something like this? Its completely untested, and I'm sure it > > can be > > cleaned up a bunch, but this keeps us from having to add additional state to > > the > > packet structure. > Yeah! I like this modification, thank you very much for your work! > I'll try to send a V2 patch based on your changes and run some tests. > Awesome, thank you! Neil -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c index e7aa177c..eeac32f 100644 --- a/net/sctp/outqueue.c +++ b/net/sctp/outqueue.c @@ -300,7 +300,7 @@ void sctp_outq_free(struct sctp_outq *q) int sctp_outq_tail(struct sctp_outq *q, struct sctp_chunk *chunk) { int error = 0; - + struct sctp_chunk *cptr; SCTP_DEBUG_PRINTK("sctp_outq_tail(%p, %p[%s])\n", q, chunk, chunk && chunk->chunk_hdr ? sctp_cname(SCTP_ST_CHUNK(chunk->chunk_hdr->type)) @@ -344,7 +344,21 @@ int sctp_outq_tail(struct sctp_outq *q, struct sctp_chunk *chunk) break; } } else { - list_add_tail(&chunk->list, &q->control_chunk_list); + list_del_init(&chunk->list); + if (chunk->chunk_hdr->type == SCTP_CID_COOKIE_ACK) + list_add_head(&chunk->list, &q->control_chunk_list); + else if (!list_empty(&q->control_chunk_list) && + chunk->chunk_hdr->type == SCTP_CID_SACK) { + list_for_each_entry(cptr, &q->control_chunk_list, list) { + if (cptr->chunk_hdr->type == SCTP_CID_COOKIE_ACK) + continue; + list_add(&chunk->list, &cptr->list); + break; + } + } + + if (list_empty(&chunk->list)) + list_add_tail(&chunk->list, &q->control_chunk_list); SCTP_INC_STATS(SCTP_MIB_OUTCTRLCHUNKS); }