On 6/28/2024 1:57 PM, Konstantin Ananyev 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")
>>>
>>> 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.
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.
>
>
>
