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

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

On 6/13/2024 11:20 AM, Konstantin Ananyev wrote:
> 
> 
>>
>> 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")
> 
> Actually my first thought was - we should return 'I' here.
> but then looking at the code more carefully, I realized that it is correct:
> nb_after_gro - would contain valid number of packets
> (at least I wasn't able to find a case when it wouldn't). 
> Though yeh, it wasn't very obvious for me at first place, so might be
> extra comment wouldn't hurt here.
> 

In first half of the function, 'nb_after_gro' is number of packets not
assembled and decided to pass back to user via 'pkts' buffer.

In second half, timed out packets are decided to turn back to user
(flushed), as they are not reassembled, and these packets are added to
'pkts' array for user, but 'nb_after_gro' not increased. So how user can
know about it?

Basically, I think we should return 'i', what am I missing, can you
please detail?