[dpdk-dev,1/5] i40e: extract non-x86 specific code from vector driver

Message ID 1472032425-16136-2-git-send-email-jianbo.liu@linaro.org (mailing list archive)
State Superseded, archived
Delegated to: Bruce Richardson
Headers

Commit Message

Jianbo Liu Aug. 24, 2016, 9:53 a.m. UTC
  move scalar code which does not use x86 intrinsic functions to new file
"i40e_rxtx_vec_common.h", while keeping x86 code in i40e_rxtx_vec.c.
This allows the scalar code to to be shared among vector drivers for
different platforms.

Signed-off-by: Jianbo Liu <jianbo.liu@linaro.org>
---
 drivers/net/i40e/i40e_rxtx_vec.c        | 184 +-----------------------
 drivers/net/i40e/i40e_rxtx_vec_common.h | 239 ++++++++++++++++++++++++++++++++
 2 files changed, 243 insertions(+), 180 deletions(-)
 create mode 100644 drivers/net/i40e/i40e_rxtx_vec_common.h
  

Comments

Qi Zhang Oct. 12, 2016, 2:55 a.m. UTC | #1
Hi Jianbo

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Jianbo Liu
> Sent: Wednesday, August 24, 2016 5:54 PM
> To: Zhang, Helin <helin.zhang@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; jerin.jacob@caviumnetworks.com; dev@dpdk.org
> Cc: Jianbo Liu <jianbo.liu@linaro.org>
> Subject: [dpdk-dev] [PATCH 1/5] i40e: extract non-x86 specific code from vector
> driver
> 
> move scalar code which does not use x86 intrinsic functions to new file
> "i40e_rxtx_vec_common.h", while keeping x86 code in i40e_rxtx_vec.c.
> This allows the scalar code to to be shared among vector drivers for different
> platforms.
> 
> Signed-off-by: Jianbo Liu <jianbo.liu@linaro.org>
> ---
>  drivers/net/i40e/i40e_rxtx_vec.c        | 184 +-----------------------
>  drivers/net/i40e/i40e_rxtx_vec_common.h | 239
> ++++++++++++++++++++++++++++++++
>  2 files changed, 243 insertions(+), 180 deletions(-)  create mode 100644
> drivers/net/i40e/i40e_rxtx_vec_common.h
> 
> diff --git a/drivers/net/i40e/i40e_rxtx_vec.c b/drivers/net/i40e/i40e_rxtx_vec.c
> index 51fb282..f847469 100644
> --- a/drivers/net/i40e/i40e_rxtx_vec.c
> +++ b/drivers/net/i40e/i40e_rxtx_vec.c
> @@ -39,6 +39,7 @@
>  #include "base/i40e_type.h"
>  #include "i40e_ethdev.h"
>  #include "i40e_rxtx.h"
> +#include "i40e_rxtx_vec_common.h"
> 
>  #include <tmmintrin.h>
> 
> @@ -421,68 +422,6 @@ i40e_recv_pkts_vec(void *rx_queue, struct rte_mbuf
> **rx_pkts,
>  	return _recv_raw_pkts_vec(rx_queue, rx_pkts, nb_pkts, NULL);  }
> 
> -static inline uint16_t
> -reassemble_packets(struct i40e_rx_queue *rxq, struct rte_mbuf **rx_bufs,
> -		   uint16_t nb_bufs, uint8_t *split_flags)
> -{
> -	struct rte_mbuf *pkts[RTE_I40E_VPMD_RX_BURST]; /*finished pkts*/
> -	struct rte_mbuf *start = rxq->pkt_first_seg;
> -	struct rte_mbuf *end =  rxq->pkt_last_seg;
> -	unsigned pkt_idx, buf_idx;
> -
> -	for (buf_idx = 0, pkt_idx = 0; buf_idx < nb_bufs; buf_idx++) {
> -		if (end != NULL) {
> -			/* processing a split packet */
> -			end->next = rx_bufs[buf_idx];
> -			rx_bufs[buf_idx]->data_len += rxq->crc_len;
> -
> -			start->nb_segs++;
> -			start->pkt_len += rx_bufs[buf_idx]->data_len;
> -			end = end->next;
> -
> -			if (!split_flags[buf_idx]) {
> -				/* it's the last packet of the set */
> -				start->hash = end->hash;
> -				start->ol_flags = end->ol_flags;
> -				/* we need to strip crc for the whole packet */
> -				start->pkt_len -= rxq->crc_len;
> -				if (end->data_len > rxq->crc_len) {
> -					end->data_len -= rxq->crc_len;
> -				} else {
> -					/* free up last mbuf */
> -					struct rte_mbuf *secondlast = start;
> -
> -					while (secondlast->next != end)
> -						secondlast = secondlast->next;
> -					secondlast->data_len -= (rxq->crc_len -
> -							end->data_len);
> -					secondlast->next = NULL;
> -					rte_pktmbuf_free_seg(end);
> -					end = secondlast;
> -				}
> -				pkts[pkt_idx++] = start;
> -				start = end = NULL;
> -			}
> -		} else {
> -			/* not processing a split packet */
> -			if (!split_flags[buf_idx]) {
> -				/* not a split packet, save and skip */
> -				pkts[pkt_idx++] = rx_bufs[buf_idx];
> -				continue;
> -			}
> -			end = start = rx_bufs[buf_idx];
> -			rx_bufs[buf_idx]->data_len += rxq->crc_len;
> -			rx_bufs[buf_idx]->pkt_len += rxq->crc_len;
> -		}
> -	}
> -
> -	/* save the partial packet for next time */
> -	rxq->pkt_first_seg = start;
> -	rxq->pkt_last_seg = end;
> -	memcpy(rx_bufs, pkts, pkt_idx * (sizeof(*pkts)));
> -	return pkt_idx;
> -}
> -
>   /* vPMD receive routine that reassembles scattered packets
>   * Notice:
>   * - nb_pkts < RTE_I40E_DESCS_PER_LOOP, just return no packet @@
> -548,73 +487,6 @@ vtx(volatile struct i40e_tx_desc *txdp,
>  		vtx1(txdp, *pkt, flags);
>  }
> 
> -static inline int __attribute__((always_inline)) -i40e_tx_free_bufs(struct
> i40e_tx_queue *txq) -{
> -	struct i40e_tx_entry *txep;
> -	uint32_t n;
> -	uint32_t i;
> -	int nb_free = 0;
> -	struct rte_mbuf *m, *free[RTE_I40E_TX_MAX_FREE_BUF_SZ];
> -
> -	/* check DD bits on threshold descriptor */
> -	if ((txq->tx_ring[txq->tx_next_dd].cmd_type_offset_bsz &
> -			rte_cpu_to_le_64(I40E_TXD_QW1_DTYPE_MASK)) !=
> -			rte_cpu_to_le_64(I40E_TX_DESC_DTYPE_DESC_DONE))
> -		return 0;
> -
> -	n = txq->tx_rs_thresh;
> -
> -	 /* first buffer to free from S/W ring is at index
> -	  * tx_next_dd - (tx_rs_thresh-1)
> -	  */
> -	txep = &txq->sw_ring[txq->tx_next_dd - (n - 1)];
> -	m = __rte_pktmbuf_prefree_seg(txep[0].mbuf);
> -	if (likely(m != NULL)) {
> -		free[0] = m;
> -		nb_free = 1;
> -		for (i = 1; i < n; i++) {
> -			m = __rte_pktmbuf_prefree_seg(txep[i].mbuf);
> -			if (likely(m != NULL)) {
> -				if (likely(m->pool == free[0]->pool)) {
> -					free[nb_free++] = m;
> -				} else {
> -					rte_mempool_put_bulk(free[0]->pool,
> -							     (void *)free,
> -							     nb_free);
> -					free[0] = m;
> -					nb_free = 1;
> -				}
> -			}
> -		}
> -		rte_mempool_put_bulk(free[0]->pool, (void **)free, nb_free);
> -	} else {
> -		for (i = 1; i < n; i++) {
> -			m = __rte_pktmbuf_prefree_seg(txep[i].mbuf);
> -			if (m != NULL)
> -				rte_mempool_put(m->pool, m);
> -		}
> -	}
> -
> -	/* buffers were freed, update counters */
> -	txq->nb_tx_free = (uint16_t)(txq->nb_tx_free + txq->tx_rs_thresh);
> -	txq->tx_next_dd = (uint16_t)(txq->tx_next_dd + txq->tx_rs_thresh);
> -	if (txq->tx_next_dd >= txq->nb_tx_desc)
> -		txq->tx_next_dd = (uint16_t)(txq->tx_rs_thresh - 1);
> -
> -	return txq->tx_rs_thresh;
> -}
> -
> -static inline void __attribute__((always_inline)) -tx_backlog_entry(struct
> i40e_tx_entry *txep,
> -		 struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
> -{
> -	int i;
> -
> -	for (i = 0; i < (int)nb_pkts; ++i)
> -		txep[i].mbuf = tx_pkts[i];
> -}
> -
>  uint16_t
>  i40e_xmit_pkts_vec(void *tx_queue, struct rte_mbuf **tx_pkts,
>  		   uint16_t nb_pkts)
> @@ -685,37 +557,13 @@ i40e_xmit_pkts_vec(void *tx_queue, struct
> rte_mbuf **tx_pkts,  void __attribute__((cold))
> i40e_rx_queue_release_mbufs_vec(struct i40e_rx_queue *rxq)  {
> -	const unsigned mask = rxq->nb_rx_desc - 1;
> -	unsigned i;
> -
> -	if (rxq->sw_ring == NULL || rxq->rxrearm_nb >= rxq->nb_rx_desc)
> -		return;
> -
> -	/* free all mbufs that are valid in the ring */
> -	for (i = rxq->rx_tail; i != rxq->rxrearm_start; i = (i + 1) & mask)
> -		rte_pktmbuf_free_seg(rxq->sw_ring[i].mbuf);
> -	rxq->rxrearm_nb = rxq->nb_rx_desc;
> -
> -	/* set all entries to NULL */
> -	memset(rxq->sw_ring, 0, sizeof(rxq->sw_ring[0]) * rxq->nb_rx_desc);
> +	_i40e_rx_queue_release_mbufs_vec(rxq);
>  }
> 
>  int __attribute__((cold))
>  i40e_rxq_vec_setup(struct i40e_rx_queue *rxq)  {
> -	uintptr_t p;
> -	struct rte_mbuf mb_def = { .buf_addr = 0 }; /* zeroed mbuf */
> -
> -	mb_def.nb_segs = 1;
> -	mb_def.data_off = RTE_PKTMBUF_HEADROOM;
> -	mb_def.port = rxq->port_id;
> -	rte_mbuf_refcnt_set(&mb_def, 1);
> -
> -	/* prevent compiler reordering: rearm_data covers previous fields */
> -	rte_compiler_barrier();
> -	p = (uintptr_t)&mb_def.rearm_data;
> -	rxq->mbuf_initializer = *(uint64_t *)p;
> -	return 0;
> +	return i40e_rxq_vec_setup_default(rxq);
>  }
> 
>  int __attribute__((cold))
> @@ -728,34 +576,10 @@ int __attribute__((cold))
> i40e_rx_vec_dev_conf_condition_check(struct rte_eth_dev *dev)  {  #ifndef
> RTE_LIBRTE_IEEE1588
> -	struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
> -	struct rte_fdir_conf *fconf = &dev->data->dev_conf.fdir_conf;
> -
>  	/* need SSE4.1 support */
>  	if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_SSE4_1))
>  		return -1;
> -
> -#ifndef RTE_LIBRTE_I40E_RX_OLFLAGS_ENABLE
> -	/* whithout rx ol_flags, no VP flag report */
> -	if (rxmode->hw_vlan_strip != 0 ||
> -	    rxmode->hw_vlan_extend != 0)
> -		return -1;
>  #endif
> 
> -	/* no fdir support */
> -	if (fconf->mode != RTE_FDIR_MODE_NONE)
> -		return -1;
> -
> -	 /* - no csum error report support
> -	 * - no header split support
> -	 */
> -	if (rxmode->hw_ip_checksum == 1 ||
> -	    rxmode->header_split == 1)
> -		return -1;
> -
> -	return 0;
> -#else
> -	RTE_SET_USED(dev);
> -	return -1;
> -#endif
> +	return i40e_rx_vec_dev_conf_condition_check_default(dev);
>  }
> diff --git a/drivers/net/i40e/i40e_rxtx_vec_common.h
> b/drivers/net/i40e/i40e_rxtx_vec_common.h
> new file mode 100644
> index 0000000..b31b39e
> --- /dev/null
> +++ b/drivers/net/i40e/i40e_rxtx_vec_common.h
> @@ -0,0 +1,239 @@
> +/*-
> + *   BSD LICENSE
> + *
> + *   Copyright(c) 2010-2015 Intel Corporation. All rights reserved.
> + *   All rights reserved.
> + *
> + *   Redistribution and use in source and binary forms, with or without
> + *   modification, are permitted provided that the following conditions
> + *   are met:
> + *
> + *     * Redistributions of source code must retain the above copyright
> + *       notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above copyright
> + *       notice, this list of conditions and the following disclaimer in
> + *       the documentation and/or other materials provided with the
> + *       distribution.
> + *     * Neither the name of Intel Corporation nor the names of its
> + *       contributors may be used to endorse or promote products derived
> + *       from this software without specific prior written permission.
> + *
> + *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
> CONTRIBUTORS
> + *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT
> NOT
> + *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND
> FITNESS FOR
> + *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
> COPYRIGHT
> + *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
> INCIDENTAL,
> + *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING,
> BUT NOT
> + *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> LOSS OF USE,
> + *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED
> AND ON ANY
> + *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR
> TORT
> + *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
> OF THE USE
> + *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH
> DAMAGE.
> + */
> +
> +#ifndef _I40E_RXTX_VEC_COMMON_H_
> +#define _I40E_RXTX_VEC_COMMON_H_
> +#include <stdint.h>
> +#include <rte_ethdev.h>
> +#include <rte_malloc.h>
> +
> +#include "i40e_ethdev.h"
> +#include "i40e_rxtx.h"
> +
> +static inline uint16_t
> +reassemble_packets(struct i40e_rx_queue *rxq, struct rte_mbuf **rx_bufs,
> +		   uint16_t nb_bufs, uint8_t *split_flags) {
> +	struct rte_mbuf *pkts[RTE_I40E_VPMD_RX_BURST]; /*finished pkts*/
> +	struct rte_mbuf *start = rxq->pkt_first_seg;
> +	struct rte_mbuf *end =  rxq->pkt_last_seg;
> +	unsigned pkt_idx, buf_idx;
> +
> +	for (buf_idx = 0, pkt_idx = 0; buf_idx < nb_bufs; buf_idx++) {
> +		if (end != NULL) {
> +			/* processing a split packet */
> +			end->next = rx_bufs[buf_idx];
> +			rx_bufs[buf_idx]->data_len += rxq->crc_len;
> +
> +			start->nb_segs++;
> +			start->pkt_len += rx_bufs[buf_idx]->data_len;
> +			end = end->next;
> +
> +			if (!split_flags[buf_idx]) {
> +				/* it's the last packet of the set */
> +				start->hash = end->hash;
> +				start->ol_flags = end->ol_flags;
> +				/* we need to strip crc for the whole packet */
> +				start->pkt_len -= rxq->crc_len;
> +				if (end->data_len > rxq->crc_len) {
> +					end->data_len -= rxq->crc_len;
> +				} else {
> +					/* free up last mbuf */
> +					struct rte_mbuf *secondlast = start;
> +
> +					while (secondlast->next != end)
> +						secondlast = secondlast->next;
> +					secondlast->data_len -= (rxq->crc_len -
> +							end->data_len);
> +					secondlast->next = NULL;
> +					rte_pktmbuf_free_seg(end);
> +					end = secondlast;
> +				}
> +				pkts[pkt_idx++] = start;
> +				start = end = NULL;
> +			}
> +		} else {
> +			/* not processing a split packet */
> +			if (!split_flags[buf_idx]) {
> +				/* not a split packet, save and skip */
> +				pkts[pkt_idx++] = rx_bufs[buf_idx];
> +				continue;
> +			}
> +			end = start = rx_bufs[buf_idx];
> +			rx_bufs[buf_idx]->data_len += rxq->crc_len;
> +			rx_bufs[buf_idx]->pkt_len += rxq->crc_len;
> +		}
> +	}
> +
> +	/* save the partial packet for next time */
> +	rxq->pkt_first_seg = start;
> +	rxq->pkt_last_seg = end;
> +	memcpy(rx_bufs, pkts, pkt_idx * (sizeof(*pkts)));
> +	return pkt_idx;
> +}
> +
> +static inline int __attribute__((always_inline))
> +i40e_tx_free_bufs(struct i40e_tx_queue *txq) {
> +	struct i40e_tx_entry *txep;
> +	uint32_t n;
> +	uint32_t i;
> +	int nb_free = 0;
> +	struct rte_mbuf *m, *free[RTE_I40E_TX_MAX_FREE_BUF_SZ];
> +
> +	/* check DD bits on threshold descriptor */
> +	if ((txq->tx_ring[txq->tx_next_dd].cmd_type_offset_bsz &
> +			rte_cpu_to_le_64(I40E_TXD_QW1_DTYPE_MASK)) !=
> +			rte_cpu_to_le_64(I40E_TX_DESC_DTYPE_DESC_DONE))
> +		return 0;
> +
> +	n = txq->tx_rs_thresh;
> +
> +	 /* first buffer to free from S/W ring is at index
> +	  * tx_next_dd - (tx_rs_thresh-1)
> +	  */
> +	txep = &txq->sw_ring[txq->tx_next_dd - (n - 1)];
> +	m = __rte_pktmbuf_prefree_seg(txep[0].mbuf);
> +	if (likely(m != NULL)) {
> +		free[0] = m;
> +		nb_free = 1;
> +		for (i = 1; i < n; i++) {
> +			m = __rte_pktmbuf_prefree_seg(txep[i].mbuf);
> +			if (likely(m != NULL)) {
> +				if (likely(m->pool == free[0]->pool)) {
> +					free[nb_free++] = m;
> +				} else {
> +					rte_mempool_put_bulk(free[0]->pool,
> +							     (void *)free,
> +							     nb_free);
> +					free[0] = m;
> +					nb_free = 1;
> +				}
> +			}
> +		}
> +		rte_mempool_put_bulk(free[0]->pool, (void **)free, nb_free);
> +	} else {
> +		for (i = 1; i < n; i++) {
> +			m = __rte_pktmbuf_prefree_seg(txep[i].mbuf);
> +			if (m != NULL)
> +				rte_mempool_put(m->pool, m);
> +		}
> +	}
> +
> +	/* buffers were freed, update counters */
> +	txq->nb_tx_free = (uint16_t)(txq->nb_tx_free + txq->tx_rs_thresh);
> +	txq->tx_next_dd = (uint16_t)(txq->tx_next_dd + txq->tx_rs_thresh);
> +	if (txq->tx_next_dd >= txq->nb_tx_desc)
> +		txq->tx_next_dd = (uint16_t)(txq->tx_rs_thresh - 1);
> +
> +	return txq->tx_rs_thresh;
> +}
> +
> +static inline void __attribute__((always_inline))
> +tx_backlog_entry(struct i40e_tx_entry *txep,
> +		 struct rte_mbuf **tx_pkts, uint16_t nb_pkts) {
> +	int i;
> +
> +	for (i = 0; i < (int)nb_pkts; ++i)
> +		txep[i].mbuf = tx_pkts[i];
> +}
> +
> +static inline void
> +_i40e_rx_queue_release_mbufs_vec(struct i40e_rx_queue *rxq) {
> +	const unsigned mask = rxq->nb_rx_desc - 1;
> +	unsigned i;
> +
> +	if (rxq->sw_ring == NULL || rxq->rxrearm_nb >= rxq->nb_rx_desc)
> +		return;
> +
> +	/* free all mbufs that are valid in the ring */
> +	for (i = rxq->rx_tail; i != rxq->rxrearm_start; i = (i + 1) & mask)
> +		rte_pktmbuf_free_seg(rxq->sw_ring[i].mbuf);
> +	rxq->rxrearm_nb = rxq->nb_rx_desc;
> +
> +	/* set all entries to NULL */
> +	memset(rxq->sw_ring, 0, sizeof(rxq->sw_ring[0]) * rxq->nb_rx_desc); }
> +
> +static inline int
> +i40e_rxq_vec_setup_default(struct i40e_rx_queue *rxq) {
> +	uintptr_t p;
> +	struct rte_mbuf mb_def = { .buf_addr = 0 }; /* zeroed mbuf */
> +
> +	mb_def.nb_segs = 1;
> +	mb_def.data_off = RTE_PKTMBUF_HEADROOM;
> +	mb_def.port = rxq->port_id;
> +	rte_mbuf_refcnt_set(&mb_def, 1);
> +
> +	/* prevent compiler reordering: rearm_data covers previous fields */
> +	rte_compiler_barrier();
> +	p = (uintptr_t)&mb_def.rearm_data;
> +	rxq->mbuf_initializer = *(uint64_t *)p;
> +	return 0;
> +}
> +
> +static inline int
> +i40e_rx_vec_dev_conf_condition_check_default(struct rte_eth_dev *dev) {
> +#ifndef RTE_LIBRTE_IEEE1588
> +	struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
> +	struct rte_fdir_conf *fconf = &dev->data->dev_conf.fdir_conf;
> +
> +#ifndef RTE_LIBRTE_I40E_RX_OLFLAGS_ENABLE
> +	/* whithout rx ol_flags, no VP flag report */
> +	if (rxmode->hw_vlan_strip != 0 ||
> +	    rxmode->hw_vlan_extend != 0)
> +		return -1;
> +#endif
> +
> +	/* no fdir support */
> +	if (fconf->mode != RTE_FDIR_MODE_NONE)
> +		return -1;
> +
> +	 /* - no csum error report support
> +	 * - no header split support
> +	 */
> +	if (rxmode->hw_ip_checksum == 1 ||
> +	    rxmode->header_split == 1)
> +		return -1;
> +
> +	return 0;
> +#else
> +	RTE_SET_USED(dev);
> +	return -1;
> +#endif
> +}
> +#endif
> --
> 2.4.11

Should we rename the function "_40e_rx_queue_release_mbufs_vec" to
"i40e_rx_queue_release_mbufs_vec_default", so functions be wrapped can follow a consistent rule?  
Thanks!
Qi
  
Jianbo Liu Oct. 13, 2016, 3 a.m. UTC | #2
On 12 October 2016 at 10:55, Zhang, Qi Z <qi.z.zhang@intel.com> wrote:
> Hi Jianbo
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Jianbo Liu
>> Sent: Wednesday, August 24, 2016 5:54 PM
>> To: Zhang, Helin <helin.zhang@intel.com>; Wu, Jingjing
>> <jingjing.wu@intel.com>; jerin.jacob@caviumnetworks.com; dev@dpdk.org
>> Cc: Jianbo Liu <jianbo.liu@linaro.org>
>> Subject: [dpdk-dev] [PATCH 1/5] i40e: extract non-x86 specific code from vector
>> driver
>>
>> move scalar code which does not use x86 intrinsic functions to new file
>> "i40e_rxtx_vec_common.h", while keeping x86 code in i40e_rxtx_vec.c.
>> This allows the scalar code to to be shared among vector drivers for different
>> platforms.
>>
>> Signed-off-by: Jianbo Liu <jianbo.liu@linaro.org>
>> ---
...
>
> Should we rename the function "_40e_rx_queue_release_mbufs_vec" to
> "i40e_rx_queue_release_mbufs_vec_default", so functions be wrapped can follow a consistent rule?

I think these two ways are different.
For func/_func, _func implements what func needs to do, they are same.
We needs _func inline, to be called in different ARCHs.
But for func/func_default, func_default is the default behavior, but
you can use or not-use it in func.
  
Qi Zhang Oct. 13, 2016, 11:58 a.m. UTC | #3
Hi Jianbo

> -----Original Message-----

> From: Jianbo Liu [mailto:jianbo.liu@linaro.org]

> Sent: Thursday, October 13, 2016 11:00 AM

> To: Zhang, Qi Z <qi.z.zhang@intel.com>

> Cc: Zhang, Helin <helin.zhang@intel.com>; Wu, Jingjing

> <jingjing.wu@intel.com>; jerin.jacob@caviumnetworks.com; dev@dpdk.org

> Subject: Re: [dpdk-dev] [PATCH 1/5] i40e: extract non-x86 specific code from

> vector driver

> 

> On 12 October 2016 at 10:55, Zhang, Qi Z <qi.z.zhang@intel.com> wrote:

> > Hi Jianbo

> >

> >> -----Original Message-----

> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Jianbo Liu

> >> Sent: Wednesday, August 24, 2016 5:54 PM

> >> To: Zhang, Helin <helin.zhang@intel.com>; Wu, Jingjing

> >> <jingjing.wu@intel.com>; jerin.jacob@caviumnetworks.com;

> dev@dpdk.org

> >> Cc: Jianbo Liu <jianbo.liu@linaro.org>

> >> Subject: [dpdk-dev] [PATCH 1/5] i40e: extract non-x86 specific code

> >> from vector driver

> >>

> >> move scalar code which does not use x86 intrinsic functions to new

> >> file "i40e_rxtx_vec_common.h", while keeping x86 code in

> i40e_rxtx_vec.c.

> >> This allows the scalar code to to be shared among vector drivers for

> >> different platforms.

> >>

> >> Signed-off-by: Jianbo Liu <jianbo.liu@linaro.org>

> >> ---

> ...

> >

> > Should we rename the function "_40e_rx_queue_release_mbufs_vec" to

> > "i40e_rx_queue_release_mbufs_vec_default", so functions be wrapped

> can follow a consistent rule?

> 

> I think these two ways are different.

> For func/_func, _func implements what func needs to do, they are same.

> We needs _func inline, to be called in different ARCHs.

> But for func/func_default, func_default is the default behavior, but you can

> use or not-use it in func.


Got your point, I also saw ixgbe is implemented in the same way.
Thanks!
Qi
  

Patch

diff --git a/drivers/net/i40e/i40e_rxtx_vec.c b/drivers/net/i40e/i40e_rxtx_vec.c
index 51fb282..f847469 100644
--- a/drivers/net/i40e/i40e_rxtx_vec.c
+++ b/drivers/net/i40e/i40e_rxtx_vec.c
@@ -39,6 +39,7 @@ 
 #include "base/i40e_type.h"
 #include "i40e_ethdev.h"
 #include "i40e_rxtx.h"
+#include "i40e_rxtx_vec_common.h"
 
 #include <tmmintrin.h>
 
@@ -421,68 +422,6 @@  i40e_recv_pkts_vec(void *rx_queue, struct rte_mbuf **rx_pkts,
 	return _recv_raw_pkts_vec(rx_queue, rx_pkts, nb_pkts, NULL);
 }
 
-static inline uint16_t
-reassemble_packets(struct i40e_rx_queue *rxq, struct rte_mbuf **rx_bufs,
-		   uint16_t nb_bufs, uint8_t *split_flags)
-{
-	struct rte_mbuf *pkts[RTE_I40E_VPMD_RX_BURST]; /*finished pkts*/
-	struct rte_mbuf *start = rxq->pkt_first_seg;
-	struct rte_mbuf *end =  rxq->pkt_last_seg;
-	unsigned pkt_idx, buf_idx;
-
-	for (buf_idx = 0, pkt_idx = 0; buf_idx < nb_bufs; buf_idx++) {
-		if (end != NULL) {
-			/* processing a split packet */
-			end->next = rx_bufs[buf_idx];
-			rx_bufs[buf_idx]->data_len += rxq->crc_len;
-
-			start->nb_segs++;
-			start->pkt_len += rx_bufs[buf_idx]->data_len;
-			end = end->next;
-
-			if (!split_flags[buf_idx]) {
-				/* it's the last packet of the set */
-				start->hash = end->hash;
-				start->ol_flags = end->ol_flags;
-				/* we need to strip crc for the whole packet */
-				start->pkt_len -= rxq->crc_len;
-				if (end->data_len > rxq->crc_len) {
-					end->data_len -= rxq->crc_len;
-				} else {
-					/* free up last mbuf */
-					struct rte_mbuf *secondlast = start;
-
-					while (secondlast->next != end)
-						secondlast = secondlast->next;
-					secondlast->data_len -= (rxq->crc_len -
-							end->data_len);
-					secondlast->next = NULL;
-					rte_pktmbuf_free_seg(end);
-					end = secondlast;
-				}
-				pkts[pkt_idx++] = start;
-				start = end = NULL;
-			}
-		} else {
-			/* not processing a split packet */
-			if (!split_flags[buf_idx]) {
-				/* not a split packet, save and skip */
-				pkts[pkt_idx++] = rx_bufs[buf_idx];
-				continue;
-			}
-			end = start = rx_bufs[buf_idx];
-			rx_bufs[buf_idx]->data_len += rxq->crc_len;
-			rx_bufs[buf_idx]->pkt_len += rxq->crc_len;
-		}
-	}
-
-	/* save the partial packet for next time */
-	rxq->pkt_first_seg = start;
-	rxq->pkt_last_seg = end;
-	memcpy(rx_bufs, pkts, pkt_idx * (sizeof(*pkts)));
-	return pkt_idx;
-}
-
  /* vPMD receive routine that reassembles scattered packets
  * Notice:
  * - nb_pkts < RTE_I40E_DESCS_PER_LOOP, just return no packet
@@ -548,73 +487,6 @@  vtx(volatile struct i40e_tx_desc *txdp,
 		vtx1(txdp, *pkt, flags);
 }
 
-static inline int __attribute__((always_inline))
-i40e_tx_free_bufs(struct i40e_tx_queue *txq)
-{
-	struct i40e_tx_entry *txep;
-	uint32_t n;
-	uint32_t i;
-	int nb_free = 0;
-	struct rte_mbuf *m, *free[RTE_I40E_TX_MAX_FREE_BUF_SZ];
-
-	/* check DD bits on threshold descriptor */
-	if ((txq->tx_ring[txq->tx_next_dd].cmd_type_offset_bsz &
-			rte_cpu_to_le_64(I40E_TXD_QW1_DTYPE_MASK)) !=
-			rte_cpu_to_le_64(I40E_TX_DESC_DTYPE_DESC_DONE))
-		return 0;
-
-	n = txq->tx_rs_thresh;
-
-	 /* first buffer to free from S/W ring is at index
-	  * tx_next_dd - (tx_rs_thresh-1)
-	  */
-	txep = &txq->sw_ring[txq->tx_next_dd - (n - 1)];
-	m = __rte_pktmbuf_prefree_seg(txep[0].mbuf);
-	if (likely(m != NULL)) {
-		free[0] = m;
-		nb_free = 1;
-		for (i = 1; i < n; i++) {
-			m = __rte_pktmbuf_prefree_seg(txep[i].mbuf);
-			if (likely(m != NULL)) {
-				if (likely(m->pool == free[0]->pool)) {
-					free[nb_free++] = m;
-				} else {
-					rte_mempool_put_bulk(free[0]->pool,
-							     (void *)free,
-							     nb_free);
-					free[0] = m;
-					nb_free = 1;
-				}
-			}
-		}
-		rte_mempool_put_bulk(free[0]->pool, (void **)free, nb_free);
-	} else {
-		for (i = 1; i < n; i++) {
-			m = __rte_pktmbuf_prefree_seg(txep[i].mbuf);
-			if (m != NULL)
-				rte_mempool_put(m->pool, m);
-		}
-	}
-
-	/* buffers were freed, update counters */
-	txq->nb_tx_free = (uint16_t)(txq->nb_tx_free + txq->tx_rs_thresh);
-	txq->tx_next_dd = (uint16_t)(txq->tx_next_dd + txq->tx_rs_thresh);
-	if (txq->tx_next_dd >= txq->nb_tx_desc)
-		txq->tx_next_dd = (uint16_t)(txq->tx_rs_thresh - 1);
-
-	return txq->tx_rs_thresh;
-}
-
-static inline void __attribute__((always_inline))
-tx_backlog_entry(struct i40e_tx_entry *txep,
-		 struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
-{
-	int i;
-
-	for (i = 0; i < (int)nb_pkts; ++i)
-		txep[i].mbuf = tx_pkts[i];
-}
-
 uint16_t
 i40e_xmit_pkts_vec(void *tx_queue, struct rte_mbuf **tx_pkts,
 		   uint16_t nb_pkts)
@@ -685,37 +557,13 @@  i40e_xmit_pkts_vec(void *tx_queue, struct rte_mbuf **tx_pkts,
 void __attribute__((cold))
 i40e_rx_queue_release_mbufs_vec(struct i40e_rx_queue *rxq)
 {
-	const unsigned mask = rxq->nb_rx_desc - 1;
-	unsigned i;
-
-	if (rxq->sw_ring == NULL || rxq->rxrearm_nb >= rxq->nb_rx_desc)
-		return;
-
-	/* free all mbufs that are valid in the ring */
-	for (i = rxq->rx_tail; i != rxq->rxrearm_start; i = (i + 1) & mask)
-		rte_pktmbuf_free_seg(rxq->sw_ring[i].mbuf);
-	rxq->rxrearm_nb = rxq->nb_rx_desc;
-
-	/* set all entries to NULL */
-	memset(rxq->sw_ring, 0, sizeof(rxq->sw_ring[0]) * rxq->nb_rx_desc);
+	_i40e_rx_queue_release_mbufs_vec(rxq);
 }
 
 int __attribute__((cold))
 i40e_rxq_vec_setup(struct i40e_rx_queue *rxq)
 {
-	uintptr_t p;
-	struct rte_mbuf mb_def = { .buf_addr = 0 }; /* zeroed mbuf */
-
-	mb_def.nb_segs = 1;
-	mb_def.data_off = RTE_PKTMBUF_HEADROOM;
-	mb_def.port = rxq->port_id;
-	rte_mbuf_refcnt_set(&mb_def, 1);
-
-	/* prevent compiler reordering: rearm_data covers previous fields */
-	rte_compiler_barrier();
-	p = (uintptr_t)&mb_def.rearm_data;
-	rxq->mbuf_initializer = *(uint64_t *)p;
-	return 0;
+	return i40e_rxq_vec_setup_default(rxq);
 }
 
 int __attribute__((cold))
@@ -728,34 +576,10 @@  int __attribute__((cold))
 i40e_rx_vec_dev_conf_condition_check(struct rte_eth_dev *dev)
 {
 #ifndef RTE_LIBRTE_IEEE1588
-	struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
-	struct rte_fdir_conf *fconf = &dev->data->dev_conf.fdir_conf;
-
 	/* need SSE4.1 support */
 	if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_SSE4_1))
 		return -1;
-
-#ifndef RTE_LIBRTE_I40E_RX_OLFLAGS_ENABLE
-	/* whithout rx ol_flags, no VP flag report */
-	if (rxmode->hw_vlan_strip != 0 ||
-	    rxmode->hw_vlan_extend != 0)
-		return -1;
 #endif
 
-	/* no fdir support */
-	if (fconf->mode != RTE_FDIR_MODE_NONE)
-		return -1;
-
-	 /* - no csum error report support
-	 * - no header split support
-	 */
-	if (rxmode->hw_ip_checksum == 1 ||
-	    rxmode->header_split == 1)
-		return -1;
-
-	return 0;
-#else
-	RTE_SET_USED(dev);
-	return -1;
-#endif
+	return i40e_rx_vec_dev_conf_condition_check_default(dev);
 }
diff --git a/drivers/net/i40e/i40e_rxtx_vec_common.h b/drivers/net/i40e/i40e_rxtx_vec_common.h
new file mode 100644
index 0000000..b31b39e
--- /dev/null
+++ b/drivers/net/i40e/i40e_rxtx_vec_common.h
@@ -0,0 +1,239 @@ 
+/*-
+ *   BSD LICENSE
+ *
+ *   Copyright(c) 2010-2015 Intel Corporation. All rights reserved.
+ *   All rights reserved.
+ *
+ *   Redistribution and use in source and binary forms, with or without
+ *   modification, are permitted provided that the following conditions
+ *   are met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in
+ *       the documentation and/or other materials provided with the
+ *       distribution.
+ *     * Neither the name of Intel Corporation nor the names of its
+ *       contributors may be used to endorse or promote products derived
+ *       from this software without specific prior written permission.
+ *
+ *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#ifndef _I40E_RXTX_VEC_COMMON_H_
+#define _I40E_RXTX_VEC_COMMON_H_
+#include <stdint.h>
+#include <rte_ethdev.h>
+#include <rte_malloc.h>
+
+#include "i40e_ethdev.h"
+#include "i40e_rxtx.h"
+
+static inline uint16_t
+reassemble_packets(struct i40e_rx_queue *rxq, struct rte_mbuf **rx_bufs,
+		   uint16_t nb_bufs, uint8_t *split_flags)
+{
+	struct rte_mbuf *pkts[RTE_I40E_VPMD_RX_BURST]; /*finished pkts*/
+	struct rte_mbuf *start = rxq->pkt_first_seg;
+	struct rte_mbuf *end =  rxq->pkt_last_seg;
+	unsigned pkt_idx, buf_idx;
+
+	for (buf_idx = 0, pkt_idx = 0; buf_idx < nb_bufs; buf_idx++) {
+		if (end != NULL) {
+			/* processing a split packet */
+			end->next = rx_bufs[buf_idx];
+			rx_bufs[buf_idx]->data_len += rxq->crc_len;
+
+			start->nb_segs++;
+			start->pkt_len += rx_bufs[buf_idx]->data_len;
+			end = end->next;
+
+			if (!split_flags[buf_idx]) {
+				/* it's the last packet of the set */
+				start->hash = end->hash;
+				start->ol_flags = end->ol_flags;
+				/* we need to strip crc for the whole packet */
+				start->pkt_len -= rxq->crc_len;
+				if (end->data_len > rxq->crc_len) {
+					end->data_len -= rxq->crc_len;
+				} else {
+					/* free up last mbuf */
+					struct rte_mbuf *secondlast = start;
+
+					while (secondlast->next != end)
+						secondlast = secondlast->next;
+					secondlast->data_len -= (rxq->crc_len -
+							end->data_len);
+					secondlast->next = NULL;
+					rte_pktmbuf_free_seg(end);
+					end = secondlast;
+				}
+				pkts[pkt_idx++] = start;
+				start = end = NULL;
+			}
+		} else {
+			/* not processing a split packet */
+			if (!split_flags[buf_idx]) {
+				/* not a split packet, save and skip */
+				pkts[pkt_idx++] = rx_bufs[buf_idx];
+				continue;
+			}
+			end = start = rx_bufs[buf_idx];
+			rx_bufs[buf_idx]->data_len += rxq->crc_len;
+			rx_bufs[buf_idx]->pkt_len += rxq->crc_len;
+		}
+	}
+
+	/* save the partial packet for next time */
+	rxq->pkt_first_seg = start;
+	rxq->pkt_last_seg = end;
+	memcpy(rx_bufs, pkts, pkt_idx * (sizeof(*pkts)));
+	return pkt_idx;
+}
+
+static inline int __attribute__((always_inline))
+i40e_tx_free_bufs(struct i40e_tx_queue *txq)
+{
+	struct i40e_tx_entry *txep;
+	uint32_t n;
+	uint32_t i;
+	int nb_free = 0;
+	struct rte_mbuf *m, *free[RTE_I40E_TX_MAX_FREE_BUF_SZ];
+
+	/* check DD bits on threshold descriptor */
+	if ((txq->tx_ring[txq->tx_next_dd].cmd_type_offset_bsz &
+			rte_cpu_to_le_64(I40E_TXD_QW1_DTYPE_MASK)) !=
+			rte_cpu_to_le_64(I40E_TX_DESC_DTYPE_DESC_DONE))
+		return 0;
+
+	n = txq->tx_rs_thresh;
+
+	 /* first buffer to free from S/W ring is at index
+	  * tx_next_dd - (tx_rs_thresh-1)
+	  */
+	txep = &txq->sw_ring[txq->tx_next_dd - (n - 1)];
+	m = __rte_pktmbuf_prefree_seg(txep[0].mbuf);
+	if (likely(m != NULL)) {
+		free[0] = m;
+		nb_free = 1;
+		for (i = 1; i < n; i++) {
+			m = __rte_pktmbuf_prefree_seg(txep[i].mbuf);
+			if (likely(m != NULL)) {
+				if (likely(m->pool == free[0]->pool)) {
+					free[nb_free++] = m;
+				} else {
+					rte_mempool_put_bulk(free[0]->pool,
+							     (void *)free,
+							     nb_free);
+					free[0] = m;
+					nb_free = 1;
+				}
+			}
+		}
+		rte_mempool_put_bulk(free[0]->pool, (void **)free, nb_free);
+	} else {
+		for (i = 1; i < n; i++) {
+			m = __rte_pktmbuf_prefree_seg(txep[i].mbuf);
+			if (m != NULL)
+				rte_mempool_put(m->pool, m);
+		}
+	}
+
+	/* buffers were freed, update counters */
+	txq->nb_tx_free = (uint16_t)(txq->nb_tx_free + txq->tx_rs_thresh);
+	txq->tx_next_dd = (uint16_t)(txq->tx_next_dd + txq->tx_rs_thresh);
+	if (txq->tx_next_dd >= txq->nb_tx_desc)
+		txq->tx_next_dd = (uint16_t)(txq->tx_rs_thresh - 1);
+
+	return txq->tx_rs_thresh;
+}
+
+static inline void __attribute__((always_inline))
+tx_backlog_entry(struct i40e_tx_entry *txep,
+		 struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
+{
+	int i;
+
+	for (i = 0; i < (int)nb_pkts; ++i)
+		txep[i].mbuf = tx_pkts[i];
+}
+
+static inline void
+_i40e_rx_queue_release_mbufs_vec(struct i40e_rx_queue *rxq)
+{
+	const unsigned mask = rxq->nb_rx_desc - 1;
+	unsigned i;
+
+	if (rxq->sw_ring == NULL || rxq->rxrearm_nb >= rxq->nb_rx_desc)
+		return;
+
+	/* free all mbufs that are valid in the ring */
+	for (i = rxq->rx_tail; i != rxq->rxrearm_start; i = (i + 1) & mask)
+		rte_pktmbuf_free_seg(rxq->sw_ring[i].mbuf);
+	rxq->rxrearm_nb = rxq->nb_rx_desc;
+
+	/* set all entries to NULL */
+	memset(rxq->sw_ring, 0, sizeof(rxq->sw_ring[0]) * rxq->nb_rx_desc);
+}
+
+static inline int
+i40e_rxq_vec_setup_default(struct i40e_rx_queue *rxq)
+{
+	uintptr_t p;
+	struct rte_mbuf mb_def = { .buf_addr = 0 }; /* zeroed mbuf */
+
+	mb_def.nb_segs = 1;
+	mb_def.data_off = RTE_PKTMBUF_HEADROOM;
+	mb_def.port = rxq->port_id;
+	rte_mbuf_refcnt_set(&mb_def, 1);
+
+	/* prevent compiler reordering: rearm_data covers previous fields */
+	rte_compiler_barrier();
+	p = (uintptr_t)&mb_def.rearm_data;
+	rxq->mbuf_initializer = *(uint64_t *)p;
+	return 0;
+}
+
+static inline int
+i40e_rx_vec_dev_conf_condition_check_default(struct rte_eth_dev *dev)
+{
+#ifndef RTE_LIBRTE_IEEE1588
+	struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
+	struct rte_fdir_conf *fconf = &dev->data->dev_conf.fdir_conf;
+
+#ifndef RTE_LIBRTE_I40E_RX_OLFLAGS_ENABLE
+	/* whithout rx ol_flags, no VP flag report */
+	if (rxmode->hw_vlan_strip != 0 ||
+	    rxmode->hw_vlan_extend != 0)
+		return -1;
+#endif
+
+	/* no fdir support */
+	if (fconf->mode != RTE_FDIR_MODE_NONE)
+		return -1;
+
+	 /* - no csum error report support
+	 * - no header split support
+	 */
+	if (rxmode->hw_ip_checksum == 1 ||
+	    rxmode->header_split == 1)
+		return -1;
+
+	return 0;
+#else
+	RTE_SET_USED(dev);
+	return -1;
+#endif
+}
+#endif