On 6/12/2024 1:48 AM, Ferruh Yigit wrote:
> On 5/23/2024 5:26 PM, Konstantin Ananyev wrote:
>> From: Konstantin Ananyev <konstantin.anan...@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.anan...@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?
>
>
As above GRO specific return value concerns clarified,
for this patch:
Acked-by: Ferruh Yigit <ferruh.yi...@amd.com>
