On 8/1/2024 11:53 AM, Hemant Agrawal wrote:
> This patch enhances the received packet debugging capability.
> This help displaying the full packet parsing output.
>
> Signed-off-by: Jun Yang <jun.yang@nxp.com>
> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> ---
> doc/guides/nics/dpaa.rst | 5 ++
> drivers/net/dpaa/dpaa_ethdev.c | 6 ++
> drivers/net/dpaa/dpaa_rxtx.c | 138 +++++++++++++++++++++++++++------
> drivers/net/dpaa/dpaa_rxtx.h | 5 ++
> 4 files changed, 130 insertions(+), 24 deletions(-)
>
> diff --git a/doc/guides/nics/dpaa.rst b/doc/guides/nics/dpaa.rst
> index 580edd9327..448607e9ac 100644
> --- a/doc/guides/nics/dpaa.rst
> +++ b/doc/guides/nics/dpaa.rst
> @@ -227,6 +227,11 @@ state during application initialization:
> application want to use eventdev with DPAA device.
> Currently these queues are not used for LS1023/LS1043 platform by default.
>
> +- ``DPAA_DISPLAY_FRAME_AND_PARSER_RESULT`` (default 0)
> +
> + This defines the debug flag, whether to dump the detailed frame and packet
> + parsing result for the incoming packets.
> +
>
If this is for debug, why not implement it as devarg.
Environment variables are supported by a few drivers, and as DPDK
drivers are userspace applications we can benefit from them, true.
But that is yet another way for configuration, I am feeling it is more
suitable way of configuration for applications. For drivers we already
have a devarg way, sticking to same configuration way provides more
consistency for users.
>
> Driver compilation and testing
> ------------------------------
> diff --git a/drivers/net/dpaa/dpaa_ethdev.c b/drivers/net/dpaa/dpaa_ethdev.c
> index e92f1c25b2..979220a700 100644
> --- a/drivers/net/dpaa/dpaa_ethdev.c
> +++ b/drivers/net/dpaa/dpaa_ethdev.c
> @@ -2094,6 +2094,12 @@ dpaa_dev_init(struct rte_eth_dev *eth_dev)
> td_tx_threshold = CGR_RX_PERFQ_THRESH;
> }
>
> +#ifdef RTE_LIBRTE_DPAA_DEBUG_DRIVER
> + penv = getenv("DPAA_DISPLAY_FRAME_AND_PARSER_RESULT");
> + if (penv)
> + dpaa_force_display_frame_set(atoi(penv));
> +#endif
>
'penv' is not defined and build fails when macro enabled.
It seems even yourself not testing with the macro, this is why using
compile time macros is bad for long term, it is hard to detect when they
are broken.
Can't you convert this macro to some runtime configuration, that is
enabled/disable via devargs, so easier to test, ensuring it is not broken.
Or other option can be to switch ETHDEV debug macro, which is used more,
by multiple cases, so more likely to be tested better.
<...>
On 07-08-2024 21:09, Ferruh Yigit wrote:
> On 8/1/2024 11:53 AM, Hemant Agrawal wrote:
>> This patch enhances the received packet debugging capability.
>> This help displaying the full packet parsing output.
>>
>> Signed-off-by: Jun Yang <jun.yang@nxp.com>
>> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
>> ---
>> doc/guides/nics/dpaa.rst | 5 ++
>> drivers/net/dpaa/dpaa_ethdev.c | 6 ++
>> drivers/net/dpaa/dpaa_rxtx.c | 138 +++++++++++++++++++++++++++------
>> drivers/net/dpaa/dpaa_rxtx.h | 5 ++
>> 4 files changed, 130 insertions(+), 24 deletions(-)
>>
>> diff --git a/doc/guides/nics/dpaa.rst b/doc/guides/nics/dpaa.rst
>> index 580edd9327..448607e9ac 100644
>> --- a/doc/guides/nics/dpaa.rst
>> +++ b/doc/guides/nics/dpaa.rst
>> @@ -227,6 +227,11 @@ state during application initialization:
>> application want to use eventdev with DPAA device.
>> Currently these queues are not used for LS1023/LS1043 platform by default.
>>
>> +- ``DPAA_DISPLAY_FRAME_AND_PARSER_RESULT`` (default 0)
>> +
>> + This defines the debug flag, whether to dump the detailed frame and packet
>> + parsing result for the incoming packets.
>> +
>>
> If this is for debug, why not implement it as devarg.
>
> Environment variables are supported by a few drivers, and as DPDK
> drivers are userspace applications we can benefit from them, true.
>
> But that is yet another way for configuration, I am feeling it is more
> suitable way of configuration for applications. For drivers we already
> have a devarg way, sticking to same configuration way provides more
> consistency for users.
For the debugging flags, devargs don't work well, at least for us.
Customers are using EAL argument embedded in their application code.
Changing devargs means they recompile their code.
In the production, the env variables are working better for us. They
just need to set it and they can re-use the existing production builds
to collect more logs.
>>
>> Driver compilation and testing
>> ------------------------------
>> diff --git a/drivers/net/dpaa/dpaa_ethdev.c b/drivers/net/dpaa/dpaa_ethdev.c
>> index e92f1c25b2..979220a700 100644
>> --- a/drivers/net/dpaa/dpaa_ethdev.c
>> +++ b/drivers/net/dpaa/dpaa_ethdev.c
>> @@ -2094,6 +2094,12 @@ dpaa_dev_init(struct rte_eth_dev *eth_dev)
>> td_tx_threshold = CGR_RX_PERFQ_THRESH;
>> }
>>
>> +#ifdef RTE_LIBRTE_DPAA_DEBUG_DRIVER
>> + penv = getenv("DPAA_DISPLAY_FRAME_AND_PARSER_RESULT");
>> + if (penv)
>> + dpaa_force_display_frame_set(atoi(penv));
>> +#endif
>>
> 'penv' is not defined and build fails when macro enabled.
fixed
>
> It seems even yourself not testing with the macro, this is why using
> compile time macros is bad for long term, it is hard to detect when they
> are broken.
>
> Can't you convert this macro to some runtime configuration, that is
> enabled/disable via devargs, so easier to test, ensuring it is not broken.
> Or other option can be to switch ETHDEV debug macro, which is used more,
> by multiple cases, so more likely to be tested better.
>
run time checks are effecting datapath. We usually ask customers to
re-compile only the net_dpaa libarry and replace it in their test for
debugging purpose.
> <...>
@@ -227,6 +227,11 @@ state during application initialization:
application want to use eventdev with DPAA device.
Currently these queues are not used for LS1023/LS1043 platform by default.
+- ``DPAA_DISPLAY_FRAME_AND_PARSER_RESULT`` (default 0)
+
+ This defines the debug flag, whether to dump the detailed frame and packet
+ parsing result for the incoming packets.
+
Driver compilation and testing
------------------------------
@@ -2094,6 +2094,12 @@ dpaa_dev_init(struct rte_eth_dev *eth_dev)
td_tx_threshold = CGR_RX_PERFQ_THRESH;
}
+#ifdef RTE_LIBRTE_DPAA_DEBUG_DRIVER
+ penv = getenv("DPAA_DISPLAY_FRAME_AND_PARSER_RESULT");
+ if (penv)
+ dpaa_force_display_frame_set(atoi(penv));
+#endif
+
/* If congestion control is enabled globally*/
if (num_rx_fqs > 0 && td_threshold) {
dpaa_intf->cgr_rx = rte_zmalloc(NULL,
@@ -47,6 +47,10 @@
#include <dpaa_of.h>
#include <netcfg.h>
+#ifdef RTE_LIBRTE_DPAA_DEBUG_DRIVER
+static int s_force_display_frm;
+#endif
+
#define DPAA_MBUF_TO_CONTIG_FD(_mbuf, _fd, _bpid) \
do { \
(_fd)->opaque_addr = 0; \
@@ -58,37 +62,122 @@
} while (0)
#ifdef RTE_LIBRTE_DPAA_DEBUG_DRIVER
+void
+dpaa_force_display_frame_set(int set)
+{
+ s_force_display_frm = set;
+}
+
#define DISPLAY_PRINT printf
-static void dpaa_display_frame_info(const struct qm_fd *fd,
- uint32_t fqid, bool rx)
+static void
+dpaa_display_frame_info(const struct qm_fd *fd,
+ uint32_t fqid, bool rx)
{
- int ii;
- char *ptr;
+ int pos, offset = 0;
+ char *ptr, info[1024];
struct annotations_t *annot = rte_dpaa_mem_ptov(fd->addr);
uint8_t format;
+ const struct dpaa_eth_parse_results_t *psr;
- if (!fd->status) {
- /* Do not display correct packets.*/
+ if (!fd->status && !s_force_display_frm) {
+ /* Do not display correct packets unless force display.*/
return;
}
+ psr = &annot->parse;
- format = (fd->opaque & DPAA_FD_FORMAT_MASK) >>
- DPAA_FD_FORMAT_SHIFT;
-
- DISPLAY_PRINT("fqid %d bpid %d addr 0x%lx, format %d\r\n",
- fqid, fd->bpid, (unsigned long)fd->addr, fd->format);
- DISPLAY_PRINT("off %d, len %d stat 0x%x\r\n",
- fd->offset, fd->length20, fd->status);
+ format = (fd->opaque & DPAA_FD_FORMAT_MASK) >> DPAA_FD_FORMAT_SHIFT;
+ if (format == qm_fd_contig)
+ sprintf(info, "simple");
+ else if (format == qm_fd_sg)
+ sprintf(info, "sg");
+ else
+ sprintf(info, "unknown format(%d)", format);
+
+ DISPLAY_PRINT("%s: fqid=%08x, bpid=%d, phy addr=0x%lx ",
+ rx ? "RX" : "TX", fqid, fd->bpid, (unsigned long)fd->addr);
+ DISPLAY_PRINT("format=%s offset=%d, len=%d, stat=0x%x\r\n",
+ info, fd->offset, fd->length20, fd->status);
if (rx) {
- ptr = (char *)&annot->parse;
- DISPLAY_PRINT("RX parser result:\r\n");
- for (ii = 0; ii < (int)sizeof(struct dpaa_eth_parse_results_t);
- ii++) {
- DISPLAY_PRINT("%02x ", ptr[ii]);
- if (((ii + 1) % 16) == 0)
- DISPLAY_PRINT("\n");
+ DISPLAY_PRINT("Display usual RX parser result:\r\n");
+ if (psr->eth_frame_type == 0)
+ offset += sprintf(&info[offset], "unicast");
+ else if (psr->eth_frame_type == 1)
+ offset += sprintf(&info[offset], "multicast");
+ else if (psr->eth_frame_type == 3)
+ offset += sprintf(&info[offset], "broadcast");
+ else
+ offset += sprintf(&info[offset], "unknown eth type(%d)",
+ psr->eth_frame_type);
+ if (psr->l2r_err) {
+ offset += sprintf(&info[offset], " L2 error(%d)",
+ psr->l2r_err);
+ } else {
+ offset += sprintf(&info[offset], " L2 non error");
}
- DISPLAY_PRINT("\n");
+ DISPLAY_PRINT("L2: %s, %s, ethernet type:%s\r\n",
+ psr->ethernet ? "is ethernet" : "non ethernet",
+ psr->vlan ? "is vlan" : "non vlan", info);
+
+ offset = 0;
+ DISPLAY_PRINT("L3: %s/%s, %s/%s, %s, %s\r\n",
+ psr->first_ipv4 ? "first IPv4" : "non first IPv4",
+ psr->last_ipv4 ? "last IPv4" : "non last IPv4",
+ psr->first_ipv6 ? "first IPv6" : "non first IPv6",
+ psr->last_ipv6 ? "last IPv6" : "non last IPv6",
+ psr->gre ? "GRE" : "non GRE",
+ psr->l3_err ? "L3 has error" : "L3 non error");
+
+ if (psr->l4_type == DPAA_PR_L4_TCP_TYPE) {
+ offset += sprintf(&info[offset], "tcp");
+ } else if (psr->l4_type == DPAA_PR_L4_UDP_TYPE) {
+ offset += sprintf(&info[offset], "udp");
+ } else if (psr->l4_type == DPAA_PR_L4_IPSEC_TYPE) {
+ offset += sprintf(&info[offset], "IPSec ");
+ if (psr->esp_sum)
+ offset += sprintf(&info[offset], "ESP");
+ if (psr->ah)
+ offset += sprintf(&info[offset], "AH");
+ } else if (psr->l4_type == DPAA_PR_L4_SCTP_TYPE) {
+ offset += sprintf(&info[offset], "sctp");
+ } else if (psr->l4_type == DPAA_PR_L4_DCCP_TYPE) {
+ offset += sprintf(&info[offset], "dccp");
+ } else {
+ offset += sprintf(&info[offset], "unknown l4 type(%d)",
+ psr->l4_type);
+ }
+ DISPLAY_PRINT("L4: type:%s, L4 validation %s\r\n",
+ info, psr->l4cv ? "Performed" : "NOT performed");
+
+ offset = 0;
+ if (psr->ethernet) {
+ offset += sprintf(&info[offset],
+ "Eth offset=%d, ethtype offset=%d, ",
+ psr->eth_off, psr->etype_off);
+ }
+ if (psr->vlan) {
+ offset += sprintf(&info[offset], "vLAN offset=%d, ",
+ psr->vlan_off[0]);
+ }
+ if (psr->first_ipv4 || psr->first_ipv6) {
+ offset += sprintf(&info[offset], "first IP offset=%d, ",
+ psr->ip_off[0]);
+ }
+ if (psr->last_ipv4 || psr->last_ipv6) {
+ offset += sprintf(&info[offset], "last IP offset=%d, ",
+ psr->ip_off[1]);
+ }
+ if (psr->gre) {
+ offset += sprintf(&info[offset], "GRE offset=%d, ",
+ psr->gre_off);
+ }
+ if (psr->l4_type >= DPAA_PR_L4_TCP_TYPE) {
+ offset += sprintf(&info[offset], "L4 offset=%d, ",
+ psr->l4_off);
+ }
+ offset += sprintf(&info[offset], "Next HDR(0x%04x) offset=%d.",
+ rte_be_to_cpu_16(psr->nxthdr), psr->nxthdr_off);
+
+ DISPLAY_PRINT("%s\r\n", info);
}
if (unlikely(format == qm_fd_sg)) {
@@ -99,13 +188,14 @@ static void dpaa_display_frame_info(const struct qm_fd *fd,
DISPLAY_PRINT("Frame payload:\r\n");
ptr = (char *)annot;
ptr += fd->offset;
- for (ii = 0; ii < fd->length20; ii++) {
- DISPLAY_PRINT("%02x ", ptr[ii]);
- if (((ii + 1) % 16) == 0)
+ for (pos = 0; pos < fd->length20; pos++) {
+ DISPLAY_PRINT("%02x ", ptr[pos]);
+ if (((pos + 1) % 16) == 0)
DISPLAY_PRINT("\n");
}
DISPLAY_PRINT("\n");
}
+
#else
#define dpaa_display_frame_info(a, b, c)
#endif
@@ -274,4 +274,9 @@ void dpaa_rx_cb_prepare(struct qm_dqrr_entry *dq, void **bufs);
void dpaa_rx_cb_no_prefetch(struct qman_fq **fq,
struct qm_dqrr_entry **dqrr, void **bufs, int num_bufs);
+#ifdef RTE_LIBRTE_DPAA_DEBUG_DRIVER
+void
+dpaa_force_display_frame_set(int set);
+#endif
+
#endif