[dpdk-dev] [RFC 4/9] net/avf: enable basic Rx Tx func

Ferruh Yigit ferruh.yigit at intel.com
Wed Nov 22 23:38:36 CET 2017


On 11/21/2017 11:55 PM, Wu, Jingjing wrote:
> 
> 
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Wednesday, November 22, 2017 8:06 AM
>> To: Wu, Jingjing <jingjing.wu at intel.com>; dev at dpdk.org
>> Cc: Lu, Wenzhuo <wenzhuo.lu at intel.com>
>> Subject: Re: [dpdk-dev] [RFC 4/9] net/avf: enable basic Rx Tx func
>>
>> On 10/20/2017 1:26 AM, Jingjing Wu wrote:
>>> Signed-off-by: Wenzhuo Lu <wenzhuo.lu at intel.com>
>>
>> <...>
>>
>>> @@ -214,6 +214,9 @@ CONFIG_RTE_LIBRTE_FM10K_INC_VECTOR=y
>>>  # Compile burst-oriented AVF PMD driver  #
>>> CONFIG_RTE_LIBRTE_AVF_PMD=y
>>> +CONFIG_RTE_LIBRTE_AVF_RX_DUMP=n
>>> +CONFIG_RTE_LIBRTE_AVF_TX_DUMP=n
>>
>> Are these config options used?
>>
> Yes, some macros are defined in avf_rxtx.h for dump descriptors. Will merge them with AVF_DEBUG_TX/RX.
> 
>> <...>
>>
>>> @@ -49,4 +49,18 @@ extern int avf_logtype_driver;
>>>  	PMD_DRV_LOG_RAW(level, fmt "\n", ## args)  #define
>>> PMD_DRV_FUNC_TRACE() PMD_DRV_LOG(DEBUG, " >>")
>>>
>>> +#ifdef RTE_LIBRTE_AVF_DEBUG_TX
>>
>> Is this defined anywhere?
> Will merge it with AVF_TX_DUMP.
> 
>>
>>> +#define PMD_TX_LOG(level, fmt, args...) \
>>> +	RTE_LOG(level, PMD, "%s(): " fmt "\n", __func__, ## args)
>>
>> Instead should RTE_LOG_DP used?
>> And since other macros uses dynamic log functions, why here use static method,
>> what do you think using new method for data path logs as well?
>>
> This is used for fast path debug, so static macro will benefit performance.

How it will benefit?

The PMD_TX_LOG macro controlled by a specific compile time option,
RTE_LIBRTE_AVF_DEBUG_TX. If this config is disabled the logging won't be part of
all binary at all.

When that config option enabled, what is the difference with macro and dynamic
debug call? Eventually both are rte_log calls. Only macro has dependency to
RTE_LOGTYPE_xxx static definitions.

> 
>> <...>
>>
>>> +static inline void
>>> +avf_rxd_to_vlan_tci(struct rte_mbuf *mb, volatile union avf_rx_desc
>>> +*rxdp) {
>>> +	if (rte_le_to_cpu_64(rxdp->wb.qword1.status_error_len) &
>>> +		(1 << AVF_RX_DESC_STATUS_L2TAG1P_SHIFT)) {
>>> +		mb->ol_flags |= PKT_RX_VLAN_PKT | PKT_RX_VLAN_STRIPPED;
>>
>> Please new flag instead of PKT_RX_VLAN_PKT and please be sure flag is
>> correctly used with its new meaning.

Just reminder of this one, new flag is "PKT_RX_VLAN" which means mbuf contains
vlan information.

>>
>> <...>
>>
>>> +/* TX prep functions */
>>> +uint16_t
>>> +avf_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
>>> +	      uint16_t nb_pkts)
>>> +{
>>> +	int i, ret;
>>> +	uint64_t ol_flags;
>>> +	struct rte_mbuf *m;
>>> +
>>> +	for (i = 0; i < nb_pkts; i++) {
>>> +		m = tx_pkts[i];
>>> +		ol_flags = m->ol_flags;
>>> +
>>> +		/* m->nb_segs is uint8_t, so nb_segs is always less than
>>> +		 * AVF_TX_MAX_SEG.
>>> +		 * We check only a condition for nb_segs >
>> AVF_TX_MAX_MTU_SEG.
>>> +		 */
>>
>> This is wrong, nb_segs is 16bits now, this check has been updated in i40e
>> already.
>>
> Will change, Thanks
> 
>> <...>



More information about the dev mailing list