Re: [PATCH v1 5/5] examples/l3fwd: enable direct rearm mode

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

On 5/31/22 20:14, Honnappa Nagarahalli wrote:
> <snip>
>>
>> 25/05/2022 01:24, Honnappa Nagarahalli пишет:
>>> From: Konstantin Ananyev <konstantin.v.ananyev@yandex.ru>
>>>
>>> 20/04/2022 09:16, Feifei Wang пишет:
>>>>> Enable direct rearm mode. The mapping is decided in the data plane
>>>>> based on the first packet received.
>>>>>
>>>>> Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
>>>>> Signed-off-by: Feifei Wang <feifei.wang2@arm.com>
>>>>> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
>>>>> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
>>>>> ---
>>>>>    examples/l3fwd/l3fwd_lpm.c | 16 +++++++++++++++-
>>>>>    1 file changed, 15 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/examples/l3fwd/l3fwd_lpm.c b/examples/l3fwd/l3fwd_lpm.c
>>>>> index bec22c44cd..38ffdf4636 100644
>>>>> --- a/examples/l3fwd/l3fwd_lpm.c
>>>>> +++ b/examples/l3fwd/l3fwd_lpm.c
>>>>> @@ -147,7 +147,7 @@ lpm_main_loop(__rte_unused void *dummy)
>>>>>        unsigned lcore_id;
>>>>>        uint64_t prev_tsc, diff_tsc, cur_tsc;
>>>>>        int i, nb_rx;
>>>>> -    uint16_t portid;
>>>>> +    uint16_t portid, tx_portid;
>>>>>        uint8_t queueid;
>>>>>        struct lcore_conf *qconf;
>>>>>        const uint64_t drain_tsc = (rte_get_tsc_hz() + US_PER_S - 1) /
>>>>> @@ -158,6 +158,8 @@ lpm_main_loop(__rte_unused void *dummy)
>>>>>        const uint16_t n_rx_q = qconf->n_rx_queue;
>>>>>        const uint16_t n_tx_p = qconf->n_tx_port;
>>>>> +    int direct_rearm_map[n_rx_q];
>>>>> +
>>>>>        if (n_rx_q == 0) {
>>>>>            RTE_LOG(INFO, L3FWD, "lcore %u has nothing to do\n",
>>>>> lcore_id);
>>>>>            return 0;
>>>>> @@ -169,6 +171,7 @@ lpm_main_loop(__rte_unused void *dummy)
>>>>>            portid = qconf->rx_queue_list[i].port_id;
>>>>>            queueid = qconf->rx_queue_list[i].queue_id;
>>>>> +        direct_rearm_map[i] = 0;
>>>>>            RTE_LOG(INFO, L3FWD,
>>>>>                " -- lcoreid=%u portid=%u rxqueueid=%hhu\n",
>>>>>                lcore_id, portid, queueid); @@ -209,6 +212,17 @@
>>>>> lpm_main_loop(__rte_unused void *dummy)
>>>>>                if (nb_rx == 0)
>>>>>                    continue;
>>>>> +            /* Determine the direct rearm mapping based on the
>>>>> +first
>>>>> +             * packet received on the rx queue
>>>>> +             */
>>>>> +            if (direct_rearm_map[i] == 0) {
>>>>> +                tx_portid = lpm_get_dst_port(qconf, pkts_burst[0],
>>>>> +                            portid);
>>>>> +                rte_eth_direct_rxrearm_map(portid, queueid,
>>>>> +                                tx_portid, queueid);
>>>>> +                direct_rearm_map[i] = 1;
>>>>> +            }
>>>>> +
>>>
>>>> That just doesn't look right to me: why to make decision based on the
>>>> first packet?
>>> The TX queue depends on the incoming packet. So, this method covers
>>> more scenarios than doing it in the control plane where the outgoing
>>> queue is not known.
>>>
>>>
>>>> What would happen if second and all other packets have to be routed
>>>> to different ports?
>>> This is an example application and it should be fine to make this
>>> assumption.
>>> More over, it does not cause any problems if packets change in between.
>>> When
>>> the packets change back, the feature works again.
>>>
>>>> In fact, this direct-rearm mode seems suitable only for hard-coded
>>>> one to one mapped forwarding (examples/l2fwd, testpmd).
>>>> For l3fwd it can be used safely only when we have one port in use.
>>> Can you elaborate more on the safety issue when more than one port is
>> used?
>>>
>>>> Also I think it should be selected at init-time and it shouldn't be
>>>> on by default.
>>>> To summarize, my opinion:
>>>> special cmd-line parameter to enable it.
>>> Can you please elaborate why a command line parameter is required?
>>> Other similar features like RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE are
>>> enabled without a command line parameter. IMO, this is how it should
>>> ber. Essentially we are trying to measure how different PMDs perform,
>>> the ones that have implemented performance improvement features
>> would
>>> show better performance (i.e. the PMDs implementing the features
>>> should not be penalized by asking for additional user input).
>>
>>   From my perspective, main purpose of l3fwd application is to demonstrate
>> DPDK ability to do packet routing based on input packet contents.
>> Making guesses about packet contents is a change in expected behavior.
>> For some cases it might improve performance, for many others - will most
>> likely cause performance drop.
>> I think that performance drop as default behavior (running the same
>> parameters as before) should not be allowed.
>> Plus you did not provided ability to switch off that behavior, if undesired.
> There is no drop in L3fwd performance due to this patch.

It is a questionable statement. Sorry. The patch adds code on
data path. So, I'm almost confident that it is possible to find
a case when performance slightly drops. May be it is always
minor and acceptable.

>>
>> About comparison with RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE default
>> enablement - I don't think it is correct.
>> Within l3fwd app we can safely guarantee that all
>> RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE pre-requirements are met:
>> in each TX queue all mbufs will belong to the same mempool and their refcnt
>> will always equal to one.
>> Here you are making guesses about contents of input packets, without any
>> guarantee that you guess will always be valid.
> This is not a guess. The code understands the incoming traffic and configures accordingly. So, it should be correct. Since it is a sample application, we do not expect the traffic to be complex. If it is complex, the performance will be the same as before or better.
> 
>>
>> BTW, what's wrong with using l2fwd to demonstrate that feature?
>> Seems like a natural choice to me.
> The performance of L3fwd application in DPDK has become a industry standard, hence we need to showcase the performance in L3fwd application.
> 
>>
>>>> allowable only when we run l3fwd over one port.
>>>
>>>
>>>>>    #if defined RTE_ARCH_X86 || defined __ARM_NEON \
>>>>>                 || defined RTE_ARCH_PPC_64
>>>>>                l3fwd_lpm_send_packets(nb_rx, pkts_burst,
>