Re: [PATCH] net: sctp: fix a cacc_saw_newack missetting issue

[Chang]

Thanks, I've got it and will submit a new patch later.
On 10/15/2013 04:34 PM, Vlad Yasevich wrote:
> On 10/15/2013 10:27 AM, Chang wrote:
> > On 10/15/2013 04:11 PM, Vlad Yasevich wrote:
> > > On 10/14/2013 09:33 AM, Chang Xiangzhong wrote:
> > > > For for each TSN t being newly acked (Not only cumulatively,
> > > > but also SELECTIVELY) cacc_saw_newack should be set to 1.
> > > >
> > > > Signed-off-by: Xiangzhong Chang <changxiangzhong@xxxxxxxxx>
> > > > ---
> > > >   net/sctp/outqueue.c |   42 
> > > > +++++++++++++++++++++---------------------
> > > >   1 file changed, 21 insertions(+), 21 deletions(-)
> > > >
> > > > diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
> > > > index 94df758..d86032b 100644
> > > > --- a/net/sctp/outqueue.c
> > > > +++ b/net/sctp/outqueue.c
> > > > @@ -1398,6 +1398,27 @@ static void sctp_check_transmitted(struct
> > > > sctp_outq *q,
> > > >                   forward_progress = true;
> > > >               }
> > > >
> > > > +            if (!tchunk->tsn_gap_acked) {
> > >
> > > You can remove this test since the block just above already performs
> > > it.  Just fold this code into the block above.
> > >
> > > -vlad
> > Sorry, I'm not sure if I fully understand you. There are code blocks
> > which checking the tchunk->tsn_gap_acked. In addition, they check other
> > states as well.
>
> The flow is:
>
>      if (sctp_acked(sack, tsn)) {
>         ...
>         if (transport) {
>             ....
>         }
>
>         if (!tchunk->tsn_gap_acked) {
>             ....
>         }
>
>         if (TSN_lte(tsn, sack_ctsn)) {
>             ....
>             /* SFR-CACC ...
>         }
>
> Since you are moving this up, you can simply re-use
> the if (!tchunk->tsn_gap_acked) immediately above.
>
> > > > +                /*
> > > > +                 * SFR-CACC algorithm:
> > > > +                 * 2) If the SACK contains gap acks
> > > > +                 * and the flag CHANGEOVER_ACTIVE is
> > > > +                 * set the receiver of the SACK MUST
> > > > +                 * take the following action:
> > > > +                 *
> > > > +                 * B) For each TSN t being acked that
> > > > +                 * has not been acked in any SACK so
> > > > +                 * far, set cacc_saw_newack to 1 for
> > > > +                 * the destination that the TSN was
> > > > +                 * sent to.
> > > > +                 */
> > > > +                if (transport &&
> > > > +                    sack->num_gap_ack_blocks &&
> > > > + q->asoc->peer.primary_path->cacc.
> > > > +                    changeover_active)
> > > > +                    transport->cacc.cacc_saw_newack    = 1;
>                             ^^^^
>
> Don't need that many spaces...
>
> -vlad
> > > > +            }
> > > > +
> > > >               if (TSN_lte(tsn, sack_ctsn)) {
> > > >                   /* RFC 2960  6.3.2 Retransmission Timer Rules
> > > >                    *
> > > > @@ -1411,27 +1432,6 @@ static void sctp_check_transmitted(struct
> > > > sctp_outq *q,
> > > >                   restart_timer = 1;
> > > >                   forward_progress = true;
> > > >
> > > > -                if (!tchunk->tsn_gap_acked) {
> > > > -                    /*
> > > > -                     * SFR-CACC algorithm:
> > > > -                     * 2) If the SACK contains gap acks
> > > > -                     * and the flag CHANGEOVER_ACTIVE is
> > > > -                     * set the receiver of the SACK MUST
> > > > -                     * take the following action:
> > > > -                     *
> > > > -                     * B) For each TSN t being acked that
> > > > -                     * has not been acked in any SACK so
> > > > -                     * far, set cacc_saw_newack to 1 for
> > > > -                     * the destination that the TSN was
> > > > -                     * sent to.
> > > > -                     */
> > > > -                    if (transport &&
> > > > -                        sack->num_gap_ack_blocks &&
> > > > - q->asoc->peer.primary_path->cacc.
> > > > -                        changeover_active)
> > > > -                        transport->cacc.cacc_saw_newack
> > > > -                            = 1;
> > > > -                }
> > > >
> > > > list_add_tail(&tchunk->transmitted_list,
> > > >                             &q->sacked);

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/