On 7/4/2024 11:05 AM, Konstantin Ananyev wrote:
>
>>>>>>>
>>>>>>> ../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")
>>>>>
>>>>> 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?
>>>
>>> Actually, as I understand the logic is different from what you described
>>> above.
>>> At the start nb_after_gro equals to total number of input packets:
>>> nb_after_gro = nb_pkts;
>>> Then later, for each packet that was merged with some other packet it
>>> decrements:
>>> ret = gro_..._reassemble(pkts[i], ...);
>>> if (ret > 0)
>>> /* Merge successfully */
>>> nb_after_gro--;
>>>
>>> So at the end nb_after_gro contains number of input packets minus number
>>> of packets that were merged.
>>>
>>
>> We have same understanding up to this point, this is what I described as
>> 'first half of the function' above.
>>
>> My concern is about the flushing timed out packets. They are copied back
>> to 'pkts' array, but 'nb_after_gro' is not updated for these packets.
>
> It is probably easier to discuss it on some example.
> Let say we have 4 input packets:
> <p0, p1, p2, p3>
> both p0 and p1 belongs to the same TCP flow and can be merged.
> p2 belongs to different tcp flow
> p3 is raw ip packet (not subject fro GRO).
> Then after first half we'll have:
>
> tcp_tbl={..., merged(p0, p1), ... p2}
> unprocess_pkts[]={p3} /* I am talking about original code for now
> */
> unprocess_num=1
> nb_after_gro==3 /* correct return value */
>
> Now it starts to re-assemble pkts[]
> First copy all unprocessed packets.
> Then as all our GRO tables are temporal (local vars) we have to flush
> all their contents back into pkts[].
> So it becomes:
> pkts[]={p3, merged(p0,p1), p2}
> return value: 3
>
>
Above is clear, thanks for explanation.
My mistake was assuming 'gro_..._timeout_flush()' can return timed out
packets from previous bursts, but this seems not the case.
These functions works only in the scope of current burst and timeout is
_not_ really used at all.
So, returning 'nb_after_gro' is OK, and it should be same with 'i' anyway.
>> What is the purpose of copying packets back to 'pkts' array?
>>
>>
>>> Which, as I undersrand should be equal to 'I' value.
>>> So, no change here is necessary, I think.
>>> Except probably some extra comment to avoid confusion.
>>>
>>>
>>>
>
