<snip> > > 25/05/2022 01:24, Honnappa Nagarahalli пишет: > > From: Konstantin Ananyev <[email protected]> > > > > 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 <[email protected]> > >>> Signed-off-by: Feifei Wang <[email protected]> > >>> Reviewed-by: Ruifeng Wang <[email protected]> > >>> Reviewed-by: Honnappa Nagarahalli <[email protected]> > >>> --- > >>> 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,
