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

Ferruh Yigit ferruh.yigit at intel.com
Thu Nov 23 00:15:20 CET 2017


On 11/21/2017 4:57 PM, Stephen Hemminger wrote:
> On Tue, 21 Nov 2017 16:06:24 -0800
> Ferruh Yigit <ferruh.yigit at intel.com> wrote:
> 
>> 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?
>>
>> <...>
>>
>>> @@ -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?
>>
>>> +#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?
>>
>> <...>
>>
>>> +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.
>>
>> <...>
>>
>>> +/* 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.
>>
>> <...>
> 
> Most drivers base code of one of the legacy Intel drivers.
> Why not fix ixgbe (or similar) to be a "follow this model" reference?
> 
> It is unreasonable to expect new drivers to follow a different pattern.

You are right, updating existing drivers will increase the chance of new drivers
being correct at first time.

After above said, it is harder to get community driven updates for existing
drivers, but easier to ask new drivers to comply with latest libraries since
there is already a resource working on developing the driver.


More information about the dev mailing list