On 12/13/21 08:37, Sriharsha Basavapatna via dev wrote: > The hw_miss_packet_recover() API results in performance degradation, for > ports that are either not offload capable or do not support this specific > offload API. > > For example, in the test configuration shown below, the vhost-user port > does not support offloads and the VF port doesn't support hw_miss offload > API. But because tunnel offload needs to be configured in other bridges > (br-vxlan and br-phy), OVS has been built with -DALLOW_EXPERIMENTAL_API. > > br-vhost br-vxlan br-phy > vhost-user<-->VF VF-Rep<-->VxLAN uplink-port > > For every packet between the VF and the vhost-user ports, hw_miss API is > called even though it is not supported by the ports involved. This leads > to significant performance drop (~3x in some cases; both cycles and pps). > > Return EOPNOTSUPP when this API fails for a device that doesn't support it > and avoid this API on that port for subsequent packets. > > Signed-off-by: Sriharsha Basavapatna <[email protected]> > --- > > v2: > Rebased changes to commit: 20a4f546f7db; updated the logic to > save 'hw_miss_api_supported' variable in struct dp_netdev_rxq.
Thanks for the update! The change looks good to me and I also tested it with dummy rte_flow and it seem to work as expected. I'm planning to apply this change once I get back from my PTO. For now, Acked-by: Ilya Maximets <[email protected]> On a side note: I noticed that dev->mutex inside the netdev_dpdk_rte_flow_get_restore_info() eats a lot of performance. Some thread-safety analysis is needed, but I hope we can remove that mutex in the future from a hot path. But that is not related to the current patch. Best regards, Ilya Maximets. > > --- > lib/dpif-netdev.c | 18 +++++++++++++----- > lib/netdev-offload-dpdk.c | 8 ++++++-- > 2 files changed, 19 insertions(+), 7 deletions(-) > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > index a790df5fd..dbe3f427b 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -377,6 +377,7 @@ struct dp_netdev_rxq { > unsigned intrvl_idx; /* Write index for 'cycles_intrvl'. */ > struct dp_netdev_pmd_thread *pmd; /* pmd thread that polls this queue. > */ > bool is_vhost; /* Is rxq of a vhost port. */ > + bool hw_miss_api_supported; /* hw_miss_packet_recover() supported > */ > > /* Counters of cycles spent successfully polling and processing pkts. */ > atomic_ullong cycles[RXQ_N_CYCLES]; > @@ -4790,6 +4791,7 @@ port_reconfigure(struct dp_netdev_port *port) > > port->rxqs[i].port = port; > port->rxqs[i].is_vhost = !strncmp(port->type, "dpdkvhost", 9); > + port->rxqs[i].hw_miss_api_supported = true; > > err = netdev_rxq_open(netdev, &port->rxqs[i].rx, i); > if (err) { > @@ -7332,12 +7334,18 @@ dp_netdev_hw_flow(const struct dp_netdev_pmd_thread > *pmd, > #ifdef ALLOW_EXPERIMENTAL_API /* Packet restoration API required. */ > /* Restore the packet if HW processing was terminated before completion. > */ > struct dp_netdev_rxq *rxq = pmd->ctx.last_rxq; > - int err; > > - err = netdev_hw_miss_packet_recover(rxq->port->netdev, packet); > - if (err && err != EOPNOTSUPP) { > - COVERAGE_INC(datapath_drop_hw_miss_recover); > - return -1; > + if (rxq->hw_miss_api_supported) { > + int err = netdev_hw_miss_packet_recover(rxq->port->netdev, packet); > + if (err) { > + if (err != EOPNOTSUPP) { > + COVERAGE_INC(datapath_drop_hw_miss_recover); > + return -1; > + } else { > + /* API unsupported by the port; avoid subsequent calls. */ > + rxq->hw_miss_api_supported = false; > + } > + } > } > #endif > > diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c > index 9fee7570a..b335572cd 100644 > --- a/lib/netdev-offload-dpdk.c > +++ b/lib/netdev-offload-dpdk.c > @@ -2292,11 +2292,15 @@ netdev_offload_dpdk_hw_miss_packet_recover(struct > netdev *netdev, > odp_port_t vport_odp; > int ret = 0; > > - if (netdev_dpdk_rte_flow_get_restore_info(netdev, packet, > - &rte_restore_info, NULL)) { > + ret = netdev_dpdk_rte_flow_get_restore_info(netdev, packet, > + &rte_restore_info, NULL); > + if (ret) { > /* This function is called for every packet, and in most cases there > * will be no restore info from the HW, thus error is expected. > */ > + if (ret == -EOPNOTSUPP) { > + return -ret; > + } > return 0; > } > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
