On 9/22/22 14:56, Maxime Coquelin wrote: > Hi Ilya, > > On 1/17/22 19:01, Ilya Maximets wrote: >> On 1/5/22 09:19, Maxime Coquelin wrote: >>> Hash-based Tx steering feature will enable steering Tx >>> packets on transmit queues based on their hashes. In order >>> to test the feature, it is needed to be able to get the >>> per-queue statistics for Vhost-user ports. >>> >>> This patch introduces "bytes", "packets" and "error" >>> per-queue custom statistics for Vhost-user ports. >>> >>> Suggested-by David Marchand <[email protected]> >>> Signed-off-by: Maxime Coquelin <[email protected]> >>> Reviewed-by: David Marchand <[email protected]> >>> --- >>> lib/netdev-dpdk.c | 147 +++++++++++++++++++++++++++++++++++++++++++--- >>> 1 file changed, 138 insertions(+), 9 deletions(-) >> >> Hi, Maxime. >> >> Thanks for the patch; and I really think that it's an important >> feature for debugging performance issues in a real-world setups. >> >> However, it causes a performance drop by about 2-2.5% for me >> with the VM-VM bidirectional traffic with 2 PMD threads. >> >> The reason is the existing stats_lock. Unfortunately, in current >> code, we're taking the same stats_lock on both rx and tx paths, >> and since rx and tx are likely performed by different threads at >> the same time, they are frequently locking each other. >> >> Under this circumstances even a slight increase of a critical >> section causes a visible performance drop. >> >> One of the possible solutions might be to split the stats_lock >> in two (one for rx stats and one for tx stats). We also should >> split or re-align to different cache lines rx and tx fields >> of the generic struct netdev_stats, or count all the stats on the >> per-queue basis. >> Quick prototype of such a solution gives an extra 2-3% performance >> boost over the current master and reduces the impact of extra >> stats in this patch to a minimum. >> >> I'll polish and submit my prototype code sometime later. >> For now, I think, we won't be able to accept this change for 2.17, >> since some more development is needed to avoid regression. > > I'm currently working on supporting the new Vhost per queue stats API in > OVS. Have you posted the prototype you did? I cannot find it, and think > it would be better to be applied before my series.
Hi. I never actually posted it, but here is the commit: https://github.com/igsilya/ovs/commit/cc3b03a8d1eb613bc42c9dc7c491efc42206f824 It's fairly simple. I'm not sure about modifying the public 'netdev_stats' structure though. It might be better to keep 2 instances of that structure. One for rx and one for tx and keep them on separate cache lines along with their locks. Best regards, Ilya Maximets. > > Thanks, > Maxime > >> There is also a memory leak in this code, but that can be easily >> fixed: >> >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >> index 0e1efefe3..f8680a058 100644 >> --- a/lib/netdev-dpdk.c >> +++ b/lib/netdev-dpdk.c >> @@ -1549,6 +1549,9 @@ netdev_dpdk_vhost_destruct(struct netdev *netdev) >> dev->vhost_id = NULL; >> rte_free(dev->vhost_rxq_enabled); >> + free(dev->vhost_rxq_stats); >> + free(dev->vhost_txq_stats); >> + >> common_destruct(dev); >> ovs_mutex_unlock(&dpdk_mutex); >> --- >> >> Best regards, Ilya Maximets. >> > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
