pktgen / Pktgen-DPDK

DPDK based packet generator
Other
377 stars 117 forks source link

tx_over run not recorded #81

Closed trajs closed 2 years ago

trajs commented 3 years ago

Hello Keith, I noticed that when rte_eth_tx_burst fails(tx_overrun), pktgen doesn't seem to record it on any of the counters. I thought it would make sense to record it as one of the variables in info struct such as info->stats.tx_failed, Please let me know your thoughts so that I can submit a patch.

pktgen commented 3 years ago

In Pktgen the code spins calling tx_burst() until all of the requested packets are sent, so the tx_failed counter would record possible multiple fails per burst. As long as all of the packets are sent do you care how many times you overrun the TX ring?

What is the need for the counter is what I am asking?

trajs commented 3 years ago

The purpose of the counter is to detect when there is tx overrun so that I can reduce the rate at which I send or to know if am overwhelming the underlying NIC and adjust my TX rate accodingly.

To give some more context, I am running pktgen on top of a VIRTIO interface and trying to run a modified rfc_2544 test Lua script, In a virtualized environment, if I just rely on the dropped packet counts to determine the rate, the results seem to vary by a bigger margin every time I run the tests on the same machine. Whereas if I get to poke into the tx_failed counter and adjust my tx rate, I seem to get stable results for multiple runs.

So I was thinking if I could just do the below, by that way dbg tx_dbg would populate "Tx Overrun" field, as well as I, could query this counter from Lua script.

diff --git a/app/pktgen.c b/app/pktgen.c
index 66cc344..250b5f9 100644
--- a/app/pktgen.c
+++ b/app/pktgen.c
@@ -332,6 +332,10 @@ pktgen_send_burst(port_info_t *info, uint16_t qid)
                if (tap)
                        pktgen_do_tx_tap(info, pkts, ret);

+    if (cnt - ret) {
+      info->stats.tx_failed += (cnt-ret);
+    }
+
                pkts += ret;
                cnt -= ret;
        }

Please let me know your thoughts on this.

pktgen commented 3 years ago

OK, that is reasonable. Just create a patch and merge request or pull request. Please do not use {} around single line 'if, for, while, do, ...' statments.

Thanks

thilakrajs-nutanix commented 3 years ago

Hi, Please find the patch below.

From 6e1303bced5a888db230088ac23d5a75d1a82c28 Mon Sep 17 00:00:00 2001
From: Thilak Raj Surendra Babu <thilakraj.sb@nutanix.com>
Date: Wed, 8 Sep 2021 17:40:41 +0000
Subject: [PATCH] fix: Use info->stats.tx_failed to record number of entries
 tx_queue was over run which is used to display "tx overrun" on "dbg tx_dbg"
 command

---
 app/pktgen.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/app/pktgen.c b/app/pktgen.c
index 66cc344..db83d04 100644
--- a/app/pktgen.c
+++ b/app/pktgen.c
@@ -332,6 +332,9 @@ pktgen_send_burst(port_info_t *info, uint16_t qid)
                if (tap)
                        pktgen_do_tx_tap(info, pkts, ret);

+               if (cnt - ret)
+                       info->stats.tx_failed += (cnt-ret);
+
                pkts += ret;
                cnt -= ret;
        }
--
2.9.3
pktgen commented 3 years ago

Can you create a pull request, it is much easier to add your code with the correct attributions.