[RFC patch] sctp: sctp_generate_fwdtsn: Initialize sctp_fwdtsn_skip array, neatening

[RFC patch] sctp: sctp_generate_fwdtsn: Initialize sctp_fwdtsn_skip array, neatening

[Joe Perches]

It's not clear to me that the sctp_fwdtsn_skip array is
always initialized when used.

It is appropriate to initialize the array to 0?

This patch initializes the array too 0 and moves the
local variables into the blocks where used.

It also does some miscellaneous neatening by using
continue; and unindenting the following block and
using ARRAY_SIZE rather than 10 to decouple the
array declaration size from a constant.
---
 net/sctp/outqueue.c | 90 ++++++++++++++++++++++++++---------------------------
 1 file changed, 44 insertions(+), 46 deletions(-)

diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
index 7e8f0a1..4c80d7b 100644
--- a/net/sctp/outqueue.c
+++ b/net/sctp/outqueue.c
@@ -1684,13 +1684,9 @@ static inline int sctp_get_skip_pos(struct sctp_fwdtsn_skip *skiplist,
 static void sctp_generate_fwdtsn(struct sctp_outq *q, __u32 ctsn)
 {
 	struct sctp_association *asoc = q->asoc;
-	struct sctp_chunk *ftsn_chunk = NULL;
-	struct sctp_fwdtsn_skip ftsn_skip_arr[10];
-	int nskips = 0;
-	int skip_pos = 0;
-	__u32 tsn;
-	struct sctp_chunk *chunk;
 	struct list_head *lchunk, *temp;
+	struct sctp_fwdtsn_skip ftsn_skip_arr[10] = {};
+	int nskips = 0;
 
 	if (!asoc->peer.prsctp_capable)
 		return;
@@ -1726,9 +1722,11 @@ static void sctp_generate_fwdtsn(struct sctp_outq *q, __u32 ctsn)
 	 * "Advanced.Peer.Ack.Point" from 102 to 104 locally.
 	 */
 	list_for_each_safe(lchunk, temp, &q->abandoned) {
-		chunk = list_entry(lchunk, struct sctp_chunk,
-					transmitted_list);
-		tsn = ntohl(chunk->subh.data_hdr->tsn);
+		struct sctp_chunk *chunk = list_entry(lchunk, struct sctp_chunk,
+						      transmitted_list);
+		sctp_datahdr_t *data_hdr = chunk->subh.data_hdr;
+		__u32 tsn = ntohl(data_hdr->tsn);
+		int skip_pos;
 
 		/* Remove any chunks in the abandoned queue that are acked by
 		 * the ctsn.
@@ -1736,52 +1734,52 @@ static void sctp_generate_fwdtsn(struct sctp_outq *q, __u32 ctsn)
 		if (TSN_lte(tsn, ctsn)) {
 			list_del_init(lchunk);
 			sctp_chunk_free(chunk);
-		} else {
-			if (TSN_lte(tsn, asoc->adv_peer_ack_point+1)) {
-				asoc->adv_peer_ack_point = tsn;
-				if (chunk->chunk_hdr->flags &
-					 SCTP_DATA_UNORDERED)
-					continue;
-				skip_pos = sctp_get_skip_pos(&ftsn_skip_arr[0],
-						nskips,
-						chunk->subh.data_hdr->stream);
-				ftsn_skip_arr[skip_pos].stream =
-					chunk->subh.data_hdr->stream;
-				ftsn_skip_arr[skip_pos].ssn =
-					 chunk->subh.data_hdr->ssn;
-				if (skip_pos == nskips)
-					nskips++;
-				if (nskips == 10)
-					break;
-			} else
-				break;
+			continue;
 		}
+
+		if (!TSN_lte(tsn, asoc->adv_peer_ack_point + 1))
+			break;
+
+		asoc->adv_peer_ack_point = tsn;
+		if (chunk->chunk_hdr->flags & SCTP_DATA_UNORDERED)
+			continue;
+
+		skip_pos = sctp_get_skip_pos(ftsn_skip_arr, nskips,
+					     data_hdr->stream);
+		ftsn_skip_arr[skip_pos].stream = data_hdr->stream;
+		ftsn_skip_arr[skip_pos].ssn = data_hdr->ssn;
+		if (skip_pos == nskips)
+			nskips++;
+		if (nskips == ARRAY_SIZE(ftsn_skip_arr))
+			break;
 	}
 
 	/* PR-SCTP C3) If, after step C1 and C2, the "Advanced.Peer.Ack.Point"
-	 * is greater than the Cumulative TSN ACK carried in the received
-	 * SACK, the data sender MUST send the data receiver a FORWARD TSN
-	 * chunk containing the latest value of the
-	 * "Advanced.Peer.Ack.Point".
+	 * is greater than the Cumulative TSN ACK carried in the received SACK,
+	 * the data sender MUST send the data receiver a FORWARD TSN chunk
+	 * containing the latest value of the "Advanced.Peer.Ack.Point".
 	 *
 	 * C4) For each "abandoned" TSN the sender of the FORWARD TSN SHOULD
 	 * list each stream and sequence number in the forwarded TSN. This
-	 * information will enable the receiver to easily find any
-	 * stranded TSN's waiting on stream reorder queues. Each stream
-	 * SHOULD only be reported once; this means that if multiple
-	 * abandoned messages occur in the same stream then only the
-	 * highest abandoned stream sequence number is reported. If the
-	 * total size of the FORWARD TSN does NOT fit in a single MTU then
-	 * the sender of the FORWARD TSN SHOULD lower the
-	 * Advanced.Peer.Ack.Point to the last TSN that will fit in a
+	 * information will enable the receiver to easily find any stranded
+	 * TSN's waiting on stream reorder queues. Each stream SHOULD only be
+	 * reported once; this means that if multiple abandoned messages occur
+	 * in the same stream then only the highest abandoned stream sequence
+	 * number is reported. If the total size of the FORWARD TSN does NOT
+	 * fit in a single MTU then the sender of the FORWARD TSN SHOULD lower
+	 * the Advanced.Peer.Ack.Point to the last TSN that will fit in a
 	 * single MTU.
 	 */
-	if (asoc->adv_peer_ack_point > ctsn)
-		ftsn_chunk = sctp_make_fwdtsn(asoc, asoc->adv_peer_ack_point,
-					      nskips, &ftsn_skip_arr[0]);
+	if (asoc->adv_peer_ack_point > ctsn) {
+		struct sctp_chunk *ftsn_chunk;
 
-	if (ftsn_chunk) {
-		list_add_tail(&ftsn_chunk->list, &q->control_chunk_list);
-		SCTP_INC_STATS(sock_net(asoc->base.sk), SCTP_MIB_OUTCTRLCHUNKS);
+		ftsn_chunk = sctp_make_fwdtsn(asoc, asoc->adv_peer_ack_point,
+					      nskips, ftsn_skip_arr);
+		if (ftsn_chunk) {
+			list_add_tail(&ftsn_chunk->list,
+				      &q->control_chunk_list);
+			SCTP_INC_STATS(sock_net(asoc->base.sk),
+				       SCTP_MIB_OUTCTRLCHUNKS);
+		}
 	}
 }

Re: [RFC patch] sctp: sctp_generate_fwdtsn: Initialize sctp_fwdtsn_skip array, neatening

[Neil Horman]

On Thu, Jul 02, 2015 at 02:54:56PM -0700, Joe Perches wrote:
> It's not clear to me that the sctp_fwdtsn_skip array is
> always initialized when used.
> 
> It is appropriate to initialize the array to 0?
> 
> This patch initializes the array too 0 and moves the
> local variables into the blocks where used.
> 
> It also does some miscellaneous neatening by using
> continue; and unindenting the following block and
> using ARRAY_SIZE rather than 10 to decouple the
> array declaration size from a constant.
> ---
We don't set ftsn_skip_arr to a known value because we limit the amount of
elements that get read from it prior to those elements being set.  That is to
say, in our first use (the call to sctp_get_skip_pos), we pass the uninitialized
array, and the nskips value, which is initalized to 0.  Looking at the
definition of sctp_get_skip_pos, the for loop there becomes a nop, meaning the
uninitalized array is irrelevant, as we never visit any of its elements.
element zero is returned, and thats what the for_each loop in
sctp_generate_fwdtsn sets in that element of the array.  On the next iteration
of the for_each loop, we call sctp_get_skip_pos with nskips = 1, so only the
first element is visited, whcih was set by the previous loop iteration.


The rest of the cleanups look ok I think.  Can you tell me what you did to test
it?

Regards
Neil

Re: [RFC patch] sctp: sctp_generate_fwdtsn: Initialize sctp_fwdtsn_skip array, neatening

[Joe Perches]

On Fri, 2015-07-03 at 07:51 -0400, Neil Horman wrote:
> On Thu, Jul 02, 2015 at 02:54:56PM -0700, Joe Perches wrote:
> > It's not clear to me that the sctp_fwdtsn_skip array is
> > always initialized when used.
> > 
> > It is appropriate to initialize the array to 0?
> > 
> > This patch initializes the array too 0 and moves the
> > local variables into the blocks where used.
> > 
> > It also does some miscellaneous neatening by using
> > continue; and unindenting the following block and
> > using ARRAY_SIZE rather than 10 to decouple the
> > array declaration size from a constant.
> > ---
> We don't set ftsn_skip_arr to a known value because we limit the amount of
> elements that get read from it prior to those elements being set.  That is to
> say, in our first use (the call to sctp_get_skip_pos), we pass the uninitialized
> array, and the nskips value, which is initalized to 0.  Looking at the
> definition of sctp_get_skip_pos, the for loop there becomes a nop, meaning the
> uninitalized array is irrelevant, as we never visit any of its elements.
> element zero is returned, and thats what the for_each loop in
> sctp_generate_fwdtsn sets in that element of the array.  On the next iteration
> of the for_each loop, we call sctp_get_skip_pos with nskips = 1, so only the
> first element is visited, whcih was set by the previous loop iteration.

Alright.

I might have chosen a while loop to limit the # of
returns but it likely compiles to the same code.

static inline int sctp_get_skip_pos(struct sctp_fwdtsn_skip *skiplist,
				    int nskips, __be16 stream)
{
	int i;

	for (i = 0; i < nskips; i++) {
		if (skiplist[i].stream == stream)
			return i;
	}
	return i;
}

to:

{
	int i = 0;

	while (i < nskips && skiplist[i].stream != stream)
		i++;

	return i;
}

> The rest of the cleanups look ok I think.  Can you tell me what you did to test
> it?

Just code inspection.

Re: [RFC patch] sctp: sctp_generate_fwdtsn: Initialize sctp_fwdtsn_skip array, neatening

[Neil Horman]

On Fri, Jul 03, 2015 at 09:41:41AM -0700, Joe Perches wrote:
> On Fri, 2015-07-03 at 07:51 -0400, Neil Horman wrote:
> > On Thu, Jul 02, 2015 at 02:54:56PM -0700, Joe Perches wrote:
> > > It's not clear to me that the sctp_fwdtsn_skip array is
> > > always initialized when used.
> > > 
> > > It is appropriate to initialize the array to 0?
> > > 
> > > This patch initializes the array too 0 and moves the
> > > local variables into the blocks where used.
> > > 
> > > It also does some miscellaneous neatening by using
> > > continue; and unindenting the following block and
> > > using ARRAY_SIZE rather than 10 to decouple the
> > > array declaration size from a constant.
> > > ---
> > We don't set ftsn_skip_arr to a known value because we limit the amount of
> > elements that get read from it prior to those elements being set.  That is to
> > say, in our first use (the call to sctp_get_skip_pos), we pass the uninitialized
> > array, and the nskips value, which is initalized to 0.  Looking at the
> > definition of sctp_get_skip_pos, the for loop there becomes a nop, meaning the
> > uninitalized array is irrelevant, as we never visit any of its elements.
> > element zero is returned, and thats what the for_each loop in
> > sctp_generate_fwdtsn sets in that element of the array.  On the next iteration
> > of the for_each loop, we call sctp_get_skip_pos with nskips = 1, so only the
> > first element is visited, whcih was set by the previous loop iteration.
> 
> Alright.
> 
> I might have chosen a while loop to limit the # of
> returns but it likely compiles to the same code.
> 
> static inline int sctp_get_skip_pos(struct sctp_fwdtsn_skip *skiplist,
> 				    int nskips, __be16 stream)
> {
> 	int i;
> 
> 	for (i = 0; i < nskips; i++) {
> 		if (skiplist[i].stream == stream)
> 			return i;
> 	}
> 	return i;
> }
> 
> to:
> 
> {
> 	int i = 0;
> 
> 	while (i < nskips && skiplist[i].stream != stream)
> 		i++;
> 
> 	return i;
> }
> 
> > The rest of the cleanups look ok I think.  Can you tell me what you did to test
> > it?
> 
> Just code inspection.

I'd like something more than that for this amount of code change.  at least run
some lksctp tests to exercise the gap ack code.
Neil

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