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

[Chang <changxiangzhong@gmail.com>]

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@gmail.com>
>>>> ---
>>>>   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);
>>>>
>>>
>>
>