Re: [RFC 2/4] gro: remove use of VLAs

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 5/23/2024 5:26 PM, Konstantin Ananyev wrote:
> From: Konstantin Ananyev <konstantin.ananyev@huawei.com>
> 
> ../lib/gro/rte_gro.c:182:34: warning: variable length array used [-Wvla]
> ../lib/gro/rte_gro.c:363:34: warning: variable length array used [-Wvla]
> 
> In both cases the pattern is the same: we use unprocess_pkts[nb_pkts] to
> collect un-used by GRO packets, and then copy them to the start of
> input/output pkts[] array.
> In both cases, we can safely copy pkts[i] into already
> processed entry at the same array, i.e. into pkts[unprocess_num].
> Such change eliminates need of temporary VLA: unprocess_pkts[nb_pkts].
> 
> Signed-off-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>
> ---
>  lib/gro/rte_gro.c | 40 ++++++++++++++--------------------------
>  1 file changed, 14 insertions(+), 26 deletions(-)
> 
> diff --git a/lib/gro/rte_gro.c b/lib/gro/rte_gro.c
> index db86117609..6d5aadf32a 100644
> --- a/lib/gro/rte_gro.c
> +++ b/lib/gro/rte_gro.c
> @@ -179,7 +179,6 @@ rte_gro_reassemble_burst(struct rte_mbuf **pkts,
>  	struct gro_vxlan_udp4_item vxlan_udp_items[RTE_GRO_MAX_BURST_ITEM_NUM]
>  			= {{{0}} };
>  
> -	struct rte_mbuf *unprocess_pkts[nb_pkts];
>  	uint32_t item_num;
>  	int32_t ret;
>  	uint16_t i, unprocess_num = 0, nb_after_gro = nb_pkts;
> @@ -275,7 +274,7 @@ rte_gro_reassemble_burst(struct rte_mbuf **pkts,
>  				/* Merge successfully */
>  				nb_after_gro--;
>  			else if (ret < 0)
> -				unprocess_pkts[unprocess_num++] = pkts[i];
> +				pkts[unprocess_num++] = pkts[i];
>  		} else if (IS_IPV4_VXLAN_UDP4_PKT(pkts[i]->packet_type) &&
>  				do_vxlan_udp_gro) {
>  			ret = gro_vxlan_udp4_reassemble(pkts[i],
> @@ -284,7 +283,7 @@ rte_gro_reassemble_burst(struct rte_mbuf **pkts,
>  				/* Merge successfully */
>  				nb_after_gro--;
>  			else if (ret < 0)
> -				unprocess_pkts[unprocess_num++] = pkts[i];
> +				pkts[unprocess_num++] = pkts[i];
>  		} else if (IS_IPV4_TCP_PKT(pkts[i]->packet_type) &&
>  				do_tcp4_gro) {
>  			ret = gro_tcp4_reassemble(pkts[i], &tcp_tbl, 0);
> @@ -292,7 +291,7 @@ rte_gro_reassemble_burst(struct rte_mbuf **pkts,
>  				/* merge successfully */
>  				nb_after_gro--;
>  			else if (ret < 0)
> -				unprocess_pkts[unprocess_num++] = pkts[i];
> +				pkts[unprocess_num++] = pkts[i];
>  		} else if (IS_IPV4_UDP_PKT(pkts[i]->packet_type) &&
>  				do_udp4_gro) {
>  			ret = gro_udp4_reassemble(pkts[i], &udp_tbl, 0);
> @@ -300,7 +299,7 @@ rte_gro_reassemble_burst(struct rte_mbuf **pkts,
>  				/* merge successfully */
>  				nb_after_gro--;
>  			else if (ret < 0)
> -				unprocess_pkts[unprocess_num++] = pkts[i];
> +				pkts[unprocess_num++] = pkts[i];
>  		} else if (IS_IPV6_TCP_PKT(pkts[i]->packet_type) &&
>  				do_tcp6_gro) {
>  			ret = gro_tcp6_reassemble(pkts[i], &tcp6_tbl, 0);
> @@ -308,21 +307,15 @@ rte_gro_reassemble_burst(struct rte_mbuf **pkts,
>  				/* merge successfully */
>  				nb_after_gro--;
>  			else if (ret < 0)
> -				unprocess_pkts[unprocess_num++] = pkts[i];
> +				pkts[unprocess_num++] = pkts[i];
>  		} else
> -			unprocess_pkts[unprocess_num++] = pkts[i];
> +			pkts[unprocess_num++] = pkts[i];
>  	}
>  
>  	if ((nb_after_gro < nb_pkts)
>  		 || (unprocess_num < nb_pkts)) {
> -		i = 0;
> -		/* Copy unprocessed packets */
> -		if (unprocess_num > 0) {
> -			memcpy(&pkts[i], unprocess_pkts,
> -					sizeof(struct rte_mbuf *) *
> -					unprocess_num);
> -			i = unprocess_num;
> -		}
> +
> +		i = unprocess_num;
>  
>  		/* Flush all packets from the tables */
>  		if (do_vxlan_tcp_gro) {
>

ack to re-use 'pkts[]' buffer for unprocessed packets, that should work.

But as a more general GRO question, above 'rte_gro_reassemble_burst()'
functions seems returns 'nb_after_gro' and as far as I can see that
amount of mbufs sits in the 'pkts[]'.
When packets flushed from tables, flushed packets are replaced to
'pkts[]' but still 'nb_after_gro' returned, there is no way for
application to know that more than 'nb_after_gro' mbufs available in the
'pkts[]'. Shouldn't return value increased per flushed packet?

Ahh, I can see it was the case before, but it is updated (perhaps
broken) in commit:
74080d7dcf31 ("gro: support IPv6 for TCP")


I wonder when GRO last tested!
@Jiayu, did you have a chance to test GRO recently?


> @@ -360,7 +353,6 @@ rte_gro_reassemble(struct rte_mbuf **pkts,
>  		uint16_t nb_pkts,
>  		void *ctx)
>  {
> -	struct rte_mbuf *unprocess_pkts[nb_pkts];
>  	struct gro_ctx *gro_ctx = ctx;
>  	void *tcp_tbl, *udp_tbl, *vxlan_tcp_tbl, *vxlan_udp_tbl, *tcp6_tbl;
>  	uint64_t current_time;
> @@ -396,33 +388,29 @@ rte_gro_reassemble(struct rte_mbuf **pkts,
>  				do_vxlan_tcp_gro) {
>  			if (gro_vxlan_tcp4_reassemble(pkts[i], vxlan_tcp_tbl,
>  						current_time) < 0)
> -				unprocess_pkts[unprocess_num++] = pkts[i];
> +				pkts[unprocess_num++] = pkts[i];
>  		} else if (IS_IPV4_VXLAN_UDP4_PKT(pkts[i]->packet_type) &&
>  				do_vxlan_udp_gro) {
>  			if (gro_vxlan_udp4_reassemble(pkts[i], vxlan_udp_tbl,
>  						current_time) < 0)
> -				unprocess_pkts[unprocess_num++] = pkts[i];
> +				pkts[unprocess_num++] = pkts[i];
>  		} else if (IS_IPV4_TCP_PKT(pkts[i]->packet_type) &&
>  				do_tcp4_gro) {
>  			if (gro_tcp4_reassemble(pkts[i], tcp_tbl,
>  						current_time) < 0)
> -				unprocess_pkts[unprocess_num++] = pkts[i];
> +				pkts[unprocess_num++] = pkts[i];
>  		} else if (IS_IPV4_UDP_PKT(pkts[i]->packet_type) &&
>  				do_udp4_gro) {
>  			if (gro_udp4_reassemble(pkts[i], udp_tbl,
>  						current_time) < 0)
> -				unprocess_pkts[unprocess_num++] = pkts[i];
> +				pkts[unprocess_num++] = pkts[i];
>  		} else if (IS_IPV6_TCP_PKT(pkts[i]->packet_type) &&
>  				do_tcp6_gro) {
>  			if (gro_tcp6_reassemble(pkts[i], tcp6_tbl,
>  						current_time) < 0)
> -				unprocess_pkts[unprocess_num++] = pkts[i];
> +				pkts[unprocess_num++] = pkts[i];
>  		} else
> -			unprocess_pkts[unprocess_num++] = pkts[i];
> -	}
> -	if (unprocess_num > 0) {
> -		memcpy(pkts, unprocess_pkts, sizeof(struct rte_mbuf *) *
> -				unprocess_num);
> +			pkts[unprocess_num++] = pkts[i];
> 

ack