[ovs-dev] [PATCH V7 06/13] dpif-netdev: Add HW miss packet state recover logic.
Ferriter, Cian
cian.ferriter at intel.com
Fri Jun 25 11:04:06 UTC 2021
Hi Eli,
I have some concerns about how we plug the packet state recover logic into dfc_processing() here. My concerns are inline below.
I'm concerned that we are hurting performance in the partial HWOL case, as this patchset is introducing new direct (non-inline) function calls per packet to the software datapath. We can reduce performance impact in this area, see specific suggestions below.
Thanks,
Cian
> -----Original Message-----
> From: Eli Britstein <elibr at nvidia.com>
> Sent: Wednesday 23 June 2021 16:53
> To: dev at openvswitch.org; Ilya Maximets <i.maximets at ovn.org>
> Cc: Gaetan Rivet <gaetanr at nvidia.com>; Majd Dibbiny <majd at nvidia.com>; Sriharsha Basavapatna
> <sriharsha.basavapatna at broadcom.com>; Hemal Shah <hemal.shah at broadcom.com>; Ivan Malov
> <Ivan.Malov at oktetlabs.ru>; Ferriter, Cian <cian.ferriter at intel.com>; Eli Britstein <elibr at nvidia.com>;
> Finn, Emma <emma.finn at intel.com>; Kovacevic, Marko <marko.kovacevic at intel.com>
> Subject: [PATCH V7 06/13] dpif-netdev: Add HW miss packet state recover logic.
>
> Recover the packet if it was partially processed by the HW. Fallback to
> lookup flow by mark association.
>
> Signed-off-by: Eli Britstein <elibr at nvidia.com>
> Reviewed-by: Gaetan Rivet <gaetanr at nvidia.com>
> Acked-by: Sriharsha Basavapatna <sriharsha.basavapatna at broadcom.com>
> Tested-by: Emma Finn <emma.finn at intel.com>
> Tested-by: Marko Kovacevic <marko.kovacevic at intel.com>
> Signed-off-by: Ilya Maximets <i.maximets at ovn.org>
> ---
> lib/dpif-netdev.c | 45 +++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 41 insertions(+), 4 deletions(-)
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 8fa7eb6d4..d5b7d5b73 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -114,6 +114,7 @@ COVERAGE_DEFINE(datapath_drop_invalid_port);
> COVERAGE_DEFINE(datapath_drop_invalid_bond);
> COVERAGE_DEFINE(datapath_drop_invalid_tnl_port);
> COVERAGE_DEFINE(datapath_drop_rx_invalid_packet);
> +COVERAGE_DEFINE(datapath_drop_hw_miss_recover);
>
> /* Protects against changes to 'dp_netdevs'. */
> static struct ovs_mutex dp_netdev_mutex = OVS_MUTEX_INITIALIZER;
> @@ -7062,6 +7063,39 @@ smc_lookup_batch(struct dp_netdev_pmd_thread *pmd,
> pmd_perf_update_counter(&pmd->perf_stats, PMD_STAT_SMC_HIT, n_smc_hit);
> }
>
> +static struct tx_port * pmd_send_port_cache_lookup(
> + const struct dp_netdev_pmd_thread *pmd, odp_port_t port_no);
> +
> +static inline int
> +dp_netdev_hw_flow(const struct dp_netdev_pmd_thread *pmd,
> + odp_port_t port_no,
> + struct dp_packet *packet,
> + struct dp_netdev_flow **flow)
> +{
> + struct tx_port *p;
> + uint32_t mark;
> +
Start of full HWOL recovery block
> + /* Restore the packet if HW processing was terminated before completion. */
> + p = pmd_send_port_cache_lookup(pmd, port_no);
> + if (OVS_LIKELY(p)) {
> + int err = netdev_hw_miss_packet_recover(p->port->netdev, packet);
> +
> + if (err && err != EOPNOTSUPP) {
> + COVERAGE_INC(datapath_drop_hw_miss_recover);
> + return -1;
> + }
> + }
End of full HWOL recovery block
IIUC, the above is only relevant to full HWOL where the packet is partially processed by the HW. In a partial HWOL testcase, we see a performance drop as a result of the above code. The performance after the patchset is applied is 0.94x what the performance was before.
We should branch over this code in the partial HWOL scenario, where we don't need to get the call to pmd_send_port_cache_lookup() and netdev_hw_miss_packet_recover(). We want this branch to be transparent to the user. Since both full and partial HWOL falls under the other_config:hw-offload=true switch, we might need a configure time check NIC capabilities solution or something similar to branch over full HWOL packet recovery code. Does this make sense?
perf top showing cycles spent per function in my partial HWOL scenario. We can see netdev_hw_miss_packet_recover(), netdev_offload_dpdk_hw_miss_packet_recover() and netdev_is_flow_api_enabled() taking cycles:
28.79% pmd-c01/id:8 ovs-vswitchd [.] dp_netdev_input__
13.72% pmd-c01/id:8 ovs-vswitchd [.] parse_tcp_flags
11.07% pmd-c01/id:8 ovs-vswitchd [.] i40e_recv_pkts_vec_avx2
10.94% pmd-c01/id:8 ovs-vswitchd [.] i40e_xmit_fixed_burst_vec_avx2
7.48% pmd-c01/id:8 ovs-vswitchd [.] cmap_find
4.94% pmd-c01/id:8 ovs-vswitchd [.] netdev_hw_miss_packet_recover
4.77% pmd-c01/id:8 ovs-vswitchd [.] dp_execute_output_action
3.81% pmd-c01/id:8 ovs-vswitchd [.] dp_netdev_pmd_flush_output_on_port
3.40% pmd-c01/id:8 ovs-vswitchd [.] netdev_send
2.49% pmd-c01/id:8 ovs-vswitchd [.] netdev_dpdk_eth_send
1.16% pmd-c01/id:8 ovs-vswitchd [.] netdev_dpdk_rxq_recv
0.90% pmd-c01/id:8 ovs-vswitchd [.] pmd_perf_end_iteration
0.75% pmd-c01/id:8 ovs-vswitchd [.] dp_netdev_process_rxq_port
0.68% pmd-c01/id:8 ovs-vswitchd [.] netdev_is_flow_api_enabled
0.55% pmd-c01/id:8 ovs-vswitchd [.] netdev_offload_dpdk_hw_miss_packet_recover
> +
> + /* If no mark, no flow to find. */
> + if (!dp_packet_has_flow_mark(packet, &mark)) {
> + *flow = NULL;
> + return 0;
> + }
> +
> + *flow = mark_to_flow_find(pmd, mark);
> + return 0;
> +}
> +
> /* Try to process all ('cnt') the 'packets' using only the datapath flow cache
> * 'pmd->flow_cache'. If a flow is not found for a packet 'packets[i]', the
> * miniflow is copied into 'keys' and the packet pointer is moved at the
> @@ -7106,7 +7140,6 @@ dfc_processing(struct dp_netdev_pmd_thread *pmd,
>
> DP_PACKET_BATCH_REFILL_FOR_EACH (i, cnt, packet, packets_) {
> struct dp_netdev_flow *flow;
> - uint32_t mark;
>
> if (OVS_UNLIKELY(dp_packet_size(packet) < ETH_HEADER_LEN)) {
> dp_packet_delete(packet);
> @@ -7125,9 +7158,13 @@ dfc_processing(struct dp_netdev_pmd_thread *pmd,
> pkt_metadata_init(&packet->md, port_no);
> }
>
> - if ((*recirc_depth_get() == 0) &&
> - dp_packet_has_flow_mark(packet, &mark)) {
> - flow = mark_to_flow_find(pmd, mark);
> + if (netdev_is_flow_api_enabled() && *recirc_depth_get() == 0) {
Here we have a per packet call to netdev_is_flow_api_enabled(). I think that netdev_is_flow_api_enabled() should be inlined if it's going to be called per packet. We can see from the above "perf top" that it isn't inlined since it shows up as a separate function.
> + if (OVS_UNLIKELY(dp_netdev_hw_flow(pmd, port_no, packet, &flow))) {
> + /* Packet restoration failed and it was dropped, do not
> + * continue processing.
> + */
> + continue;
> + }
> if (OVS_LIKELY(flow)) {
> tcp_flags = parse_tcp_flags(packet);
> if (OVS_LIKELY(batch_enable)) {
> --
> 2.28.0.2311.g225365fb51
More information about the dev
mailing list