Re: [PATCH] sctp: Make "Invalid Stream Identifier" ERROR follows SACK when bundling

[Vlad Yasevich <vyasevich@gmail.com>]

On 07/31/2012 02:51 AM, xufeng zhang wrote:
> Sorry, please ignore the above patch, there was an paste error.
> Please check the following patch.
> ============================================
> I'm wondering if the below solution is fine to you which is based on
> your changes.
> BTW, I have verified this patch and it works ok for all the situation,
> but only one problem persists:
> there is a potential that commands will exceeds SCTP_MAX_NUM_COMMANDS
> which happens during sending lots of small error DATA chunks.
>

I started thinking along the same vein, but was thinking that maybe it 
makes sense to make error list more generic. I need to check the spec on 
the ordering of ERROR chunks.  If they are always after other control 
chunks, then maybe make an error list and queue all errors there.  Then 
when sending control chunks, drain the control queue first, then the 
error queue, and finally the data queue.

BTW, the patch below doesn't include the code to queue the error chunk 
onto the new error queue.

-vlad

> Thanks,
> Xufeng Zhang
>
> ---
>   include/net/sctp/command.h |    1 +
>   include/net/sctp/structs.h |    3 +++
>   net/sctp/outqueue.c        |    7 +++++++
>   net/sctp/sm_sideeffect.c   |   16 ++++++++++++++++
>   net/sctp/sm_statefuns.c    |   17 ++++++++++++++---
>   5 files changed, 41 insertions(+), 3 deletions(-)
>
> diff --git a/include/net/sctp/command.h b/include/net/sctp/command.h
> index 712b3be..62c34f5 100644
> --- a/include/net/sctp/command.h
> +++ b/include/net/sctp/command.h
> @@ -110,6 +110,7 @@ typedef enum {
>          SCTP_CMD_SEND_NEXT_ASCONF, /* Send the next ASCONF after ACK */
>          SCTP_CMD_PURGE_ASCONF_QUEUE, /* Purge all asconf queues.*/
>          SCTP_CMD_SET_ASOC,       /* Restore association context */
> +       SCTP_CMD_GEN_BAD_STREAM, /* Invalid Stream errors happened
> command */
>          SCTP_CMD_LAST
>   } sctp_verb_t;
>
> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
> index fc5e600..3d218e0 100644
> --- a/include/net/sctp/structs.h
> +++ b/include/net/sctp/structs.h
> @@ -1183,6 +1183,9 @@ struct sctp_outq {
>           */
>          struct list_head abandoned;
>
> +       /* Put Invalid Stream error chunks on this list */
> +       struct list_head bad_stream_err;
> +
>          /* How many unackd bytes do we have in-flight?  */
>          __u32 outstanding_bytes;
>
> diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
> index e7aa177..1e87b0b 100644
> --- a/net/sctp/outqueue.c
> +++ b/net/sctp/outqueue.c
> @@ -211,6 +211,7 @@ void sctp_outq_init(struct sctp_association *asoc,
> struct sctp_outq *q)
>          INIT_LIST_HEAD(&q->retransmit);
>          INIT_LIST_HEAD(&q->sacked);
>          INIT_LIST_HEAD(&q->abandoned);
> +       INIT_LIST_HEAD(&q->bad_stream_err);
>
>          q->fast_rtx = 0;
>          q->outstanding_bytes = 0;
> @@ -283,6 +284,12 @@ void sctp_outq_teardown(struct sctp_outq *q)
>                  list_del_init(&chunk->list);
>                  sctp_chunk_free(chunk);
>          }
> +
> +       /* Throw away any pending Invalid Stream error chunks */
> +       list_for_each_entry_safe(chunk, tmp,&q->bad_stream_err, list) {
> +               list_del_init(&chunk->list);
> +               sctp_chunk_free(chunk);
> +       }
>   }
>
>   /* Free the outqueue structure and any related pending chunks.  */
> diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
> index fe99628..4698593 100644
> --- a/net/sctp/sm_sideeffect.c
> +++ b/net/sctp/sm_sideeffect.c
> @@ -1060,6 +1060,18 @@ static void sctp_cmd_send_asconf(struct
> sctp_association *asoc)
>          }
>   }
>
> +static void sctp_cmd_make_inv_stream_err(sctp_cmd_seq_t *commands,
> +               struct sctp_association *asoc)
> +{
> +       struct sctp_chunk *err, *tmp;
> +       struct sctp_outq *q =&asoc->outqueue;
> +
> +       list_for_each_entry_safe(err, tmp,&q->bad_stream_err, list) {
> +               list_del_init(&err->list);
> +               sctp_add_cmd_sf(commands, SCTP_CMD_REPLY,
> +                               SCTP_CHUNK(err));
> +       }
> +}
>
>   /* These three macros allow us to pull the debugging code out of the
>    * main flow of sctp_do_sm() to keep attention focused on the real
> @@ -1724,6 +1736,10 @@ static int sctp_cmd_interpreter(sctp_event_t
> event_type,
>                          asoc = cmd->obj.asoc;
>                          break;
>
> +               case SCTP_CMD_GEN_BAD_STREAM:
> +                       sctp_cmd_make_inv_stream_err(commands, asoc);
> +                       break;
> +
>                  default:
>                          pr_warn("Impossible command: %u, %p\n",
>                                  cmd->verb, cmd->obj.ptr);
> diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
> index 9fca103..1c1bcd9 100644
> --- a/net/sctp/sm_statefuns.c
> +++ b/net/sctp/sm_statefuns.c
> @@ -2967,8 +2967,14 @@ discard_force:
>          return SCTP_DISPOSITION_DISCARD;
>
>   discard_noforce:
> -       if (chunk->end_of_packet)
> +       if (chunk->end_of_packet) {
> +               struct sctp_outq *q =&asoc->outqueue;
>                  sctp_add_cmd_sf(commands, SCTP_CMD_GEN_SACK, force);
> +               /* Queue the INVALID STREAM error after the SACK if one
> is needed. */
> +               if (!list_empty(&q->bad_stream_err))
> +                       sctp_add_cmd_sf(commands, SCTP_CMD_GEN_BAD_STREAM,
> +                                       SCTP_NULL());
> +       }
>
>          return SCTP_DISPOSITION_DISCARD;
>   consume:
> @@ -3037,11 +3043,16 @@ sctp_disposition_t
> sctp_sf_eat_data_fast_4_4(const struct sctp_endpoint *ep,
>           * with a SACK, a SHUTDOWN chunk, and restart the T2-shutdown
> timer
>           */
>          if (chunk->end_of_packet) {
> +               struct sctp_outq *q =&asoc->outqueue;
>                  /* We must delay the chunk creation since the cumulative
>                   * TSN has not been updated yet.
>                   */
>                  sctp_add_cmd_sf(commands, SCTP_CMD_GEN_SHUTDOWN,
> SCTP_NULL());
>                  sctp_add_cmd_sf(commands, SCTP_CMD_GEN_SACK,
> SCTP_FORCE());
> +               /* Queue the INVALID STREAM error after the SACK if one
> is needed. */
> +               if (!list_empty(&q->bad_stream_err))
> +                       sctp_add_cmd_sf(commands, SCTP_CMD_GEN_BAD_STREAM,
> +                                       SCTP_NULL());
>                  sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
>                                  SCTP_TO(SCTP_EVENT_TIMEOUT_T2_SHUTDOWN));
>          }
> @@ -6136,6 +6147,7 @@ static int sctp_eat_data(const struct
> sctp_association *asoc,
>           */
>          sid = ntohs(data_hdr->stream);
>          if (sid>= asoc->c.sinit_max_instreams) {
> +               struct sctp_outq *q =&asoc->outqueue;
>                  /* Mark tsn as received even though we drop it */
>                  sctp_add_cmd_sf(commands, SCTP_CMD_REPORT_TSN,
> SCTP_U32(tsn));
>
> @@ -6144,8 +6156,7 @@ static int sctp_eat_data(const struct
> sctp_association *asoc,
>                                           sizeof(data_hdr->stream),
>                                           sizeof(u16));
>                  if (err)
> -                       sctp_add_cmd_sf(commands, SCTP_CMD_REPLY,
> -                                       SCTP_CHUNK(err));
> +                       list_add_tail(&err->list,&q->bad_stream_err);
>                  return SCTP_IERROR_BAD_STREAM;
>          }
>