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

[Vlad Yasevich <vyasevich@gmail.com>]

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);
>>>
>>
>