Re: [PATCH v11] gro: fix reordering of packets in GRO layer

[Stephen Hemminger <stephen@networkplumber.org>]

On Sun,  7 Jan 2024 16:59:20 +0530
Kumara Parameshwaran <kumaraparamesh92@gmail.com> wrote:

> +	/* Return early if the TCP flags are not handled in GRO layer */
> +	if (tcp_hdr->tcp_flags & (~(VALID_GRO_TCP_FLAGS)))

Nit, lots of extra paren here. Could be:
	if (tcp_hdr->tcp_flags & ~VALID_GRO_TCP_FLAGS)

> +	if (find == 1) {
> +		/*
> +		 * Any packet with additional flags like PSH,FIN should be processed
> +		 * and flushed immediately.
> +		 * Hence marking the start time to 0, so that the packets will be flushed
> +		 * immediately in timer mode.
> +		 */
> +		if (tcp_hdr->tcp_flags & (RTE_TCP_ACK_FLAG | RTE_TCP_PSH_FLAG | RTE_TCP_FIN_FLAG)) {
> +			if (tcp_hdr->tcp_flags != RTE_TCP_ACK_FLAG)
> +				tbl->items[item_start_idx].start_time = 0;
> +			return process_tcp_item(pkt, tcp_hdr, tcp_dl, tbl->items,
> +						tbl->flows[i].start_index,
> +						&tbl->item_num, tbl->max_item_num,
> +						ip_id, is_atomic, start_time);
> +		} else {
> +			return -1;
> +		}
> +	}

Reordering this conditional would keep code from being so indented.

> -			delete_tcp_item(tbl->items, item_idx, &tbl->item_num, INVALID_ARRAY_INDEX);
> +			delete_tcp_item(tbl->items, item_idx, &tbl->item_num,
> +				INVALID_ARRAY_INDEX);
>  			return -1;

This change is unnecessary, max line length in DPDK is 100 characters for readability.

>  		return 0;
> +	} else {
> +		return -1;
>  	}
>  
> -	return process_tcp_item(pkt, tcp_hdr, tcp_dl, tbl->items, tbl->flows[i].start_index,
> -						&tbl->item_num, tbl->max_item_num,
> -						ip_id, is_atomic, start_time);
> +	return -1;
>  }

Since end of else and end of function both return -1, the else clause is unnecessary.