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

[Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>]

<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.

> 
> 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,