Re: [PATCH v11] net/iavf: add diagnostic support in TX path

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

On 1/10/2024 3:25 AM, Mingjin Ye wrote:
> Implemented a Tx wrapper to perform a thorough check on mbufs,
> categorizing and counting invalid cases by types for diagnostic
> purposes. The count of invalid cases is accessible through xstats_get.
> 
> Also, the devarg option "mbuf_check" was introduced to configure the
> diagnostic parameters to enable the appropriate diagnostic features.
> 
> supported cases: mbuf, size, segment, offload.
>   1. mbuf: check for corrupted mbuf.
>   2. size: check min/max packet length according to hw spec.
>   3. segment: check number of mbuf segments not exceed hw limitation.
>   4. offload: check any unsupported offload flag.
> 
> parameter format: "mbuf_check=<case>" or "mbuf_check=[<case1>,<case2>]"
> eg: dpdk-testpmd -a 0000:81:01.0,mbuf_check=[mbuf,size] -- -i
> 
> Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
> ---

<snip>

> 
> diff --git a/doc/guides/nics/intel_vf.rst b/doc/guides/nics/intel_vf.rst
> index ce96c2e1f8..f62bb4233c 100644
> --- a/doc/guides/nics/intel_vf.rst
> +++ b/doc/guides/nics/intel_vf.rst
> @@ -111,6 +111,17 @@ For more detail on SR-IOV, please refer to the following documents:
>       by setting the ``devargs`` parameter like ``-a 18:01.0,no-poll-on-link-down=1``
>       when IAVF is backed by an Intel\ |reg| E810 device or an Intel\ |reg| 700 Series Ethernet device.
>   
> +    When IAVF is backed by an Intel\ |reg| E810 device or an Intel\ |reg| 700 series Ethernet devices.

This looks like a duplicate line of the one above it, so probably needs 
to be removed?

> +    Set the ``devargs`` parameter ``mbuf_check`` to enable TX diagnostics. For example,
> +    ``-a 18:01.0,mbuf_check=<case>`` or ``-a 18:01.0,mbuf_check=[<case1>,<case2>...]``. Also,
> +    ``xstats_get`` can be used to get the error counts, which are collected in ``tx_mbuf_error_packets``
> +    xstats. For example, ``testpmd> show port xstats all``. Supported cases:
> +
> +    *   mbuf: Check for corrupted mbuf.
> +    *   size: Check min/max packet length according to hw spec.
> +    *   segment: Check number of mbuf segments not exceed hw limitation.
> +    *   offload: Check any unsupported offload flag.
> +

<snip>

> +
> +	int ret = 0;
> +	uint64_t *mc_flags = args;
> +	char *str2 = strdup(value);
> +	if (str2 == NULL)
> +		return -1;
> +
> +	str_len = strlen(str2);
> +	if (str2[0] == '[' && str2[str_len - 1] == ']') {
> +		if (str_len < 3) {
> +			ret = -1;
> +			goto mdd_end;
> +		}
> +		valid_len = str_len - 2;
> +		memmove(str2, str2 + 1, valid_len);
> +		memset(str2 + valid_len, '\0', 2);
> +	}

I would suggest adding a comment mentioning that we're removing outer 
square brackets from the value. Even better would be to factor it out 
into a separate function, so that code for this function is not polluted 
with implementation details of brackets removal.

> +	cur = strtok_r(str2, ",", &tmp);
> +	while (cur != NULL) {
> +		if (!strcmp(cur, "mbuf"))
> +			*mc_flags |= IAVF_MBUF_CHECK_F_TX_MBUF;
> +		else if (!strcmp(cur, "size"))
> +			*mc_flags |= IAVF_MBUF_CHECK_F_TX_SIZE;
> +		else if (!strcmp(cur, "segment"))
> +			*mc_flags |= IAVF_MBUF_CHECK_F_TX_SEGMENT;
> +		else if (!strcmp(cur, "offload"))
> +			*mc_flags |= IAVF_MBUF_CHECK_F_TX_OFFLOAD;
> +		else
> +			PMD_DRV_LOG(ERR, "Unsupported mdd check type: %s", cur);

I suspect 'mdd' is an artifact of earlier versions? What does it mean, 
and can the error message be made more meaningful?

> +		cur = strtok_r(NULL, ",", &tmp);
> +	}
> +
> +mdd_end:
> +	free(str2);
> +	return ret;
> +}
> +
>   static int iavf_parse_devargs(struct rte_eth_dev *dev)
>   {
>   	struct iavf_adapter *ad =
> @@ -2344,6 +2411,11 @@ static int iavf_parse_devargs(struct rte_eth_dev *dev)
>   		goto bail;
>   	}
>   

<snip>

> +		}
> +	}
> +
> +	if (pkt_error) {
> +		txq->mbuf_errors++;
> +		good_pkts = idx;
> +		if (good_pkts == 0)
> +			return 0;
> +	}
> +
> +	return iavf_tx_pkt_burst_ops[tx_burst_type](tx_queue,
> +								tx_pkts, good_pkts);

The indentation here is a bit odd.


The above suggestions are not blocking, so overall

Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
-- 
Thanks,
Anatoly