Re: [dpdk-dev] [PATCH v2] net/nfb: new Netcope driver

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 2/27/2019 11:43 AM, Rastislav Cernay wrote:
> From: Rastislav Cernay <cernay@netcope.com>
> 
> Added new net driver for Netcope nfb cards
> 
> Signed-off-by: Rastislav Cernay <cernay@netcope.com>
> ---
> v2: remove unnecessary cast
>     remove unnecessary zeroing
>     move declaration to not mix with code
>     restore skeleton example
>  MAINTAINERS                      |   7 +
>  config/common_base               |   4 +
>  devtools/test-build.sh           |   1 +
>  doc/guides/nics/features/nfb.ini |  17 ++
>  doc/guides/nics/nfb.rst          | 141 ++++++++++

Do we need to add this file into index, doc/guides/nics/index.rst? Did you
compiled the doc and verified it is visible in the left side menu?

>  drivers/net/Makefile             |   1 +
>  drivers/net/meson.build          |   1 +
>  drivers/net/nfb/Makefile         |  41 +++
>  drivers/net/nfb/meson.build      |   9 +
>  drivers/net/nfb/nfb.h            |  51 ++++
>  drivers/net/nfb/nfb_ethdev.c     | 583 +++++++++++++++++++++++++++++++++++++++
>  drivers/net/nfb/nfb_rx.c         | 127 +++++++++
>  drivers/net/nfb/nfb_rx.h         | 226 +++++++++++++++
>  drivers/net/nfb/nfb_rxmode.c     | 104 +++++++
>  drivers/net/nfb/nfb_rxmode.h     |  81 ++++++
>  drivers/net/nfb/nfb_stats.c      |  79 ++++++
>  drivers/net/nfb/nfb_stats.h      |  52 ++++
>  drivers/net/nfb/nfb_tx.c         | 112 ++++++++
>  drivers/net/nfb/nfb_tx.h         | 209 ++++++++++++++
>  mk/rte.app.mk                    |   1 +

Can you please update the release notes (release_19_05.rst) to announce the new PMD?

Also I believe you will need a .map file, explicitly "rte_pmd_nfb_version.map"
according makefile, to be able to produce shared library.

<...>

> +Prerequisites
> +-------------
> +
> +This PMD requires kernel modules which are responsible for initialization and
> +allocation of resources needed for nfb layer function.
> +Communication between PMD and kernel modules is mediated by libnfb library.
> +These kernel modules and library are not part of DPDK and must be installed
> +separately:
> +
> +*  **libnfb library**
> +
> +   The library provides API for initialization of nfb transfers, receiving and
> +   transmitting data segments.
> +
> +*  **Kernel modules**
> +
> +   * nfb
> +
> +   Kernel modules manage initialization of hardware, allocation and
> +   sharing of resources for user space applications.
> +
> +Dependencies can be found here:
> +`Netcope common <https://drive.google.com/file/d/13khqS312KzrRSrgGxD0CBSmd1YLJHWp8>`_.

There should be a way to download prerequisites, so thanks for providing this
link, but how reliable this google drive is, is there a way to provide some link
 or information from vendor official site, as you are doing for szedata2?

Also above link gives an rpm, is there any source code option for different distros?

<...>

> +/**
> + * DPDK callback for Ethernet device configuration.
> + *
> + * @param dev
> + *   Pointer to Ethernet device structure.
> + *
> + * @return
> + *   0 on success, a negative errno value otherwise and rte_errno is set.
> + */
> +static int
> +nfb_eth_dev_configure(struct rte_eth_dev *dev)
> +{
> +	dev->rx_pkt_burst = nfb_eth_ndp_rx;

This looks like fixed value, can be possible to move to device init instead of
having in configure()?

> +	return 0;
> +}
> +
> +/**
> + * DPDK callback to get information about the device.
> + *
> + * @param dev
> + *   Pointer to Ethernet device structure.
> + * @param[out] info
> + *   Info structure output buffer.
> + */
> +static void
> +nfb_eth_dev_info(struct rte_eth_dev *dev,
> +	struct rte_eth_dev_info *dev_info)
> +{
> +	dev_info->max_mac_addrs = 1;
> +	dev_info->max_rx_pktlen = (uint32_t)-1;
> +	dev_info->max_rx_queues = dev->data->nb_rx_queues;
> +	dev_info->max_tx_queues = dev->data->nb_tx_queues;

This prevents you increasing number of queues later, since these values are
coming from your library, most probably you already can't increase them, but a
reminder in case.

> +	dev_info->speed_capa = ETH_LINK_SPEED_10
Just to highlight that this is not setting any offload capability, any
application that requires some offload capabilities will fail to configure the
device.

<...>

> +nfb_eth_link_update(struct rte_eth_dev *dev,
> +	int wait_to_complete __rte_unused)
> +{
> +	uint16_t i;
> +	struct nc_rxmac_status status;
> +	struct rte_eth_link link;
> +	memset(&link, 0, sizeof(link));
> +
> +	struct pmd_internals *internals = dev->data->dev_private;
> +
> +	status.speed = MAC_SPEED_UNKNOWN;
> +
> +	link.link_speed = ETH_SPEED_NUM_NONE;
> +	link.link_status = ETH_LINK_DOWN;
> +	link.link_duplex = ETH_LINK_FULL_DUPLEX;
> +	link.link_autoneg = ETH_LINK_SPEED_FIXED;
> +
> +	if (internals->rxmac[0] != NULL) {
> +		nc_rxmac_read_status(internals->rxmac[0], &status);
> +
> +		switch (status.speed) {
> +		case MAC_SPEED_10G:
> +			link.link_speed = ETH_SPEED_NUM_10G;
> +			break;
> +		case MAC_SPEED_40G:
> +			link.link_speed = ETH_SPEED_NUM_40G;
> +			break;
> +		case MAC_SPEED_100G:
> +			link.link_speed = ETH_SPEED_NUM_100G;
> +			break;
> +		default:
> +			link.link_speed = ETH_SPEED_NUM_NONE;
> +			break;
> +		}
> +	}
> +
> +	i = 0;

Can drop this line.

<...>

> +/**
> + * DPDK callback to set primary MAC address.
> + *
> + * @param dev
> + *   Pointer to Ethernet device structure.
> + * @param mac_addr
> + *   MAC address to register.
> + *
> + * @return
> + *   0 on success, a negative errno value otherwise and rte_errno is set.
> + */
> +static int
> +nfb_eth_mac_addr_set(struct rte_eth_dev *dev __rte_unused,
> +	struct ether_addr *mac_addr __rte_unused)
> +{
> +	return 0;

This returns success but doesn't set the provided MAC address at all, shouldn't
send a fail insted?
Will it break any code flow if this ops is missing or returning error?

<...>

> +static int
> +nfb_eth_dev_init(struct rte_eth_dev *dev)
> +{
> +	struct rte_eth_dev_data *data = dev->data;
> +	struct pmd_internals *internals = (struct pmd_internals *)
> +		data->dev_private;
> +	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
> +	struct rte_pci_addr *pci_addr = &pci_dev->addr;
> +
> +	RTE_LOG(INFO, PMD, "Initializing NFB device (" PCI_PRI_FMT ")\n",
> +		pci_addr->domain, pci_addr->bus, pci_addr->devid,
> +		pci_addr->function);

This is using PMD log type, I agree it is simpler to use it but having own log
type gives more flexibility chaning log type dynamiccally with better
granularity, in case you are intetested in adding own logtype.

> +
> +	snprintf(internals->nfb_dev, PATH_MAX,
> +		"/dev/nfb/by-pci-slot/" PCI_PRI_FMT,
> +		pci_addr->domain, pci_addr->bus, pci_addr->devid,
> +		pci_addr->function);

Does this fixed path mean it is only supported by Linux, but not FreeBSD? If so
it can be better to document this as the supported architecture documented.

> +
> +	/*
> +	 * Get number of available DMA RX and TX queues, which is maximum
> +	 * number of queues that can be created and store it in private device
> +	 * data structure.
> +	 */
> +	internals->nfb = nfb_open(internals->nfb_dev);
> +	if (internals->nfb == NULL) {
> +		RTE_LOG(ERR, PMD, "nfb_open(): failed to open %s",
> +			internals->nfb_dev);
> +		return -EINVAL;
> +	}
> +	data->nb_rx_queues = ndp_get_rx_queue_available_count(internals->nfb);
> +	data->nb_tx_queues = ndp_get_tx_queue_available_count(internals->nfb);
> +
> +	RTE_LOG(INFO, PMD, "Available NDP queues RX: %u TX: %u\n",
> +		data->nb_rx_queues, data->nb_tx_queues);
> +
> +	nfb_nc_rxmac_init(internals->nfb,
> +		internals->rxmac,
> +		&internals->max_rxmac);
> +	nfb_nc_txmac_init(internals->nfb,
> +		internals->txmac,
> +		&internals->max_txmac);
> +
> +	/* Set rx, tx burst functions */
> +	dev->rx_pkt_burst = nfb_eth_ndp_rx;
> +	dev->tx_pkt_burst = nfb_eth_ndp_tx;
> +
> +	/* Set function callbacks for Ethernet API */
> +	dev->dev_ops = &ops;
> +
> +	rte_eth_copy_pci_info(dev, pci_dev);

This may be redundant, I guess rte_eth_dev_pci_generic_probe() does this
already, can you please check?

> +
> +	/* Get link state */
> +	nfb_eth_link_update(dev, 0);
> +
> +	/* Allocate space for one mac address */
> +	data->mac_addrs = rte_zmalloc(data->name, sizeof(struct ether_addr),
> +		RTE_CACHE_LINE_SIZE);
> +	if (data->mac_addrs == NULL) {
> +		RTE_LOG(ERR, PMD, "Could not alloc space for MAC address!\n");
> +		nfb_close(internals->nfb);
> +		return -EINVAL;
> +	}
> +
> +	ether_addr_copy(&eth_addr, data->mac_addrs);

This copies same MAC address for all instances of the device, if there are multi
ports or multi device all we have same MAC. Do you want to diffrenciate them?

<...>

> +/**
> + * DPDK callback to uninitialize an ethernet device
> + *
> + * @param dev
> + *   Pointer to ethernet device structure
> + *
> + * @return
> + *   0 on success, a negative errno value otherwise and rte_errno is set.
> + */
> +static int
> +nfb_eth_dev_uninit(struct rte_eth_dev *dev)
> +{
> +	struct rte_eth_dev_data *data = dev->data;
> +	struct pmd_internals *internals = (struct pmd_internals *)
> +		data->dev_private;
> +
> +	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
> +	struct rte_pci_addr *pci_addr = &pci_dev->addr;
> +
> +	rte_free(dev->data->mac_addrs);
> +	dev->data->mac_addrs = NULL;

rte_eth_dev_pci_generic_remove() will cause the mac_address to be freed, you may
drop above free:

rte_eth_dev_pci_generic_remove
  rte_eth_dev_pci_release
    rte_eth_dev_release_port
      rte_free(eth_dev->data->mac_addrs);

<...>

> +static const struct rte_pci_id nfb_pci_id_table[] = {
> +	{
> +		RTE_PCI_DEVICE(PCI_VENDOR_ID_NETCOPE, PCI_DEVICE_ID_NFB_40G2)
> +	},

It is matter of taste perhaps but single line looks better to me, if you like
them as it is, I am OK with that too:
	{ RTE_PCI_DEVICE(PCI_VENDOR_ID_NETCOPE, PCI_DEVICE_ID_NFB_40G2) },

<...>

> +static struct rte_pci_driver nfb_eth_driver = {
> +	.id_table = nfb_pci_id_table,
> +	.probe = nfb_eth_pci_probe,
> +	.remove = nfb_eth_pci_remove,

just to double check if you need any drv_flag set here, like
RTE_PCI_DRV_INTR_LSC or RTE_PCI_DRV_NEED_MAPPING?

<...>

> diff --git a/drivers/net/nfb/nfb_rx.h b/drivers/net/nfb/nfb_rx.h
> new file mode 100644
> index 0000000..b356f5d
> --- /dev/null
> +++ b/drivers/net/nfb/nfb_rx.h
> @@ -0,0 +1,226 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2018 Cesnet
> + * Copyright(c) 2018 Netcope Technologies, a.s. <info@netcope.com>
> + * All rights reserved.
> + */
> +
> +
> +

multiple blank lines, please clean if not intentional.

<...>

> +/**
> + * Initialize ndp_rx_queue structure
> + *
> + * @param nfb
> + *   Pointer to nfb device structure.
> + * @param rx_queue_id
> + *   RX queue index.
> + * @param port_id
> + *   Device [external] port identifier.
> + * @param mb_pool
> + *   Memory pool for buffer allocations.
> + * @param[out] rxq
> + *   Pointer to ndp_rx_queue output structure
> + * @return
> + *   0 on success, a negative errno value otherwise and rte_errno is set.

Does it really sets 'rte_errno'? I guess only copy/paste artifact, looks like
there are more in other function comments.

<...>

> +static __rte_always_inline uint16_t
> +nfb_eth_ndp_rx(void *queue,
> +	struct rte_mbuf **bufs,
> +	uint16_t nb_pkts)
> +{
> +	struct ndp_rx_queue *ndp = queue;
> +
> +	if (unlikely(ndp->queue == NULL || nb_pkts == 0)) {
> +		RTE_LOG(ERR, PMD, "RX invalid arguments!\n");
> +		return 0;
> +	}
> +
> +	struct rte_mbuf *mbufs[nb_pkts];
> +
> +	unsigned int i;
> +	// returns either all or nothing

Please use c89 style comments, there are more below.

> +	i = rte_pktmbuf_alloc_bulk(ndp->mb_pool, mbufs, nb_pkts);
> +	if (unlikely(i != 0))
> +		return 0;
> +
> +	uint16_t packet_size;
> +	uint64_t num_bytes = 0;
> +	const uint16_t buf_size = ndp->buf_size;

And please have definitions at the beggining of the function.

> +
> +	struct rte_mbuf *mbuf;
> +	struct ndp_packet packets[nb_pkts];
> +
> +
> +	uint16_t num_rx = ndp_rx_burst_get(ndp->queue, packets, nb_pkts);
> +
> +	if (unlikely(num_rx != nb_pkts)) {
> +		for (i = num_rx; i < nb_pkts; i++)
> +			rte_pktmbuf_free(mbufs[i]);
> +	}
> +
> +	nb_pkts = num_rx;
> +
> +	num_rx = 0;
> +	/*
> +	 * Reads the given number of packets from NDP queue given
> +	 * by queue and copies the packet data into a newly allocated mbuf
> +	 * to return.
> +	 */
> +	for (i = 0; i < nb_pkts; ++i) {
> +		mbuf = mbufs[i];
> +
> +		/* get the space available for data in the mbuf */
> +		packet_size = packets[i].data_length;
> +
> +		if (likely(packet_size <= buf_size)) {
> +			/* NDP packet will fit in one mbuf, go ahead and copy */
> +			rte_memcpy(rte_pktmbuf_mtod(mbuf, void *),
> +				packets[i].data, packet_size);
> +
> +			mbuf->data_len = (uint16_t)packet_size;
> +
> +			mbuf->pkt_len = packet_size;
> +			mbuf->port = ndp->in_port;
> +			bufs[num_rx++] = mbuf;
> +			num_bytes += packet_size;
> +		} else {
> +			/*
> +			 * NDP packet will not fit in one mbuf,
> +			 * scattered mode is not enabled, drop packet
> +			 */
> +			RTE_LOG(ERR, PMD,
> +				"NDP segment %d bytes will not fit in one mbuf "
> +				"(%d bytes), scattered mode is not enabled, "
> +				"drop packet!!\n",
> +				packet_size, buf_size);

It may not be good idea to add log into data path, we may have tens of millions
packet per second.
And there are some logging macros that can be removed based on compile time
config options, it can be better idea to use them for data path,
CONFIG_RTE_LOG_DP_LEVEL.

<...>

> +int
> +nfb_eth_stats_get(struct rte_eth_dev *dev,
> +	struct rte_eth_stats *stats)
> +{
> +	uint16_t i;
> +	uint16_t nb_rx = dev->data->nb_rx_queues;
> +	uint16_t nb_tx = dev->data->nb_tx_queues;
> +	uint64_t rx_total = 0;
> +	uint64_t tx_total = 0;
> +	uint64_t tx_err_total = 0;
> +	uint64_t rx_total_bytes = 0;
> +	uint64_t tx_total_bytes = 0;
> +
> +	struct ndp_rx_queue *rx_queue = *((struct ndp_rx_queue **)
> +		dev->data->rx_queues);
> +	struct ndp_tx_queue *tx_queue = *((struct ndp_tx_queue **)
> +		dev->data->tx_queues);
> +
> +	for (i = 0; i < nb_rx; i++) {
> +		if (i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
> +			stats->q_ipackets[i] = rx_queue[i].rx_pkts;
> +			stats->q_ibytes[i] = rx_queue[i].rx_bytes;
> +		}
> +		rx_total += stats->q_ipackets[i];
> +		rx_total_bytes += stats->q_ibytes[i];
> +	}
> +
> +	for (i = 0; i < nb_tx; i++) {
> +		if (i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
> +			stats->q_opackets[i] = tx_queue[i].tx_pkts;
> +			stats->q_obytes[i] = tx_queue[i].tx_bytes;
> +			stats->q_errors[i] = tx_queue[i].err_pkts;

This is what David mentioned, q_errors seems like for Rx queue errors, there is
a general inconsistancey here, perhaps you can leave the code as it is but
please aware of this, it may needs to be changed soon.

<...>

> +	for (i = 0; i < nb_pkts; ++i) {
> +		mbuf = bufs[i];
> +
> +		pkt_len = mbuf->pkt_len;
> +		mbuf_segs = mbuf->nb_segs;
> +
> +		num_bytes += pkt_len;
> +		if (mbuf_segs == 1) {
> +			/*
> +			 * non-scattered packet,
> +			 * transmit from one mbuf
> +			 */
> +			rte_memcpy(packets[i].data,
> +				rte_pktmbuf_mtod(mbuf, const void *),
> +				pkt_len);
> +		} else {
> +			/* scattered packet, transmit from more mbufs */
> +			struct rte_mbuf *m = mbuf;
> +			while (m) {
> +				dst = packets[i].data;
> +
> +				rte_memcpy(dst,
> +					rte_pktmbuf_mtod(m,
> +					const void *),
> +					m->data_len);
> +				dst = ((uint8_t *)(dst)) +
> +					m->data_len;
> +				m = m->next;
> +			}
> +		}

This code looks like supports "DEV_TX_OFFLOAD_MULTI_SEGS", better to report his
capability in dev_info.