[dpdk-dev] [PATCH v4 1/5] lib: add Port Representor library

[Ferruh Yigit]

On 1/8/2018 2:37 PM, Remy Horton wrote:
> Port Representors provide a logical presentation in DPDK of VF (virtual
> function) ports for the purposes of control and monitoring. Each port
> representor device represents a single VF and is associated with it's
> parent physical function (PF) PMD which provides the back-end hooks for
> the representor device ops and defines the control domain to which that
> port belongs. This allows to use existing DPDK APIs to monitor and control
> the port without the need to create and maintain VF specific APIs.
> 
> The library provides the broker infrastructure to be instantiated by
> base driver and corresponding methods to manage the broker
> infrastructure. The broker keeps records of list of representor PMDs.
> The library also provides methods to manage the representor PMDs by the
> broker.
> 
> Signed-off-by: Declan Doherty <declan.doherty at intel.com>
> Signed-off-by: Mohammad Abdul Awal <mohammad.abdul.awal at intel.com>
> Signed-off-by: Remy Horton <remy.horton at intel.com>

<...>

> @@ -788,7 +795,6 @@ F: doc/guides/sample_app_ug/test_pipeline.rst
>  F: examples/ip_pipeline/
>  F: doc/guides/sample_app_ug/ip_pipeline.rst
>  
> -

Can drop this. Two line break is intentional I think.

<...>

> @@ -820,3 +820,8 @@ CONFIG_RTE_APP_CRYPTO_PERF=y
>  # Compile the eventdev application
>  #
>  CONFIG_RTE_APP_EVENTDEV=y
> +
> +#
> +# Compile representor PMD
> +#
> +CONFIG_RTE_LIBRTE_REPRESENTOR=y

Last section of the config is for apps, can you please group this with libraries?

> diff --git a/doc/api/doxy-api-index.md b/doc/api/doxy-api-index.md
> index 3492702..5020a40 100644
> --- a/doc/api/doxy-api-index.md
> +++ b/doc/api/doxy-api-index.md
> @@ -50,7 +50,8 @@ The public API headers are grouped by topics:
>    [bitrate]            (@ref rte_bitrate.h),
>    [latency]            (@ref rte_latencystats.h),
>    [devargs]            (@ref rte_devargs.h),
> -  [PCI]                (@ref rte_pci.h)
> +  [PCI]                (@ref rte_pci.h),
> +  [Port Representor]   (@ref rte_port_representor.h)

Since this is related to the ethdev, I would put this after rte_mtr.

<....>

> @@ -41,6 +41,15 @@ New Features
>       Also, make sure to start the actual text at the margin.
>       =========================================================
>  
> +* **Added Port Representor functionality.**
> +
> +  Port Representors provide a logical presentation in DPDK of VF (virtual
> +  function) ports for the purposes of control and monitoring. Each port
> +  representor device represents a single VF and is associated with it's
> +  parent PF (physical function) PMD which provides the back-end hooks for
> +  the representor device ops and defines the control domain to which that
> +  port belongs.
> +

Can you please add new library to the "Shared Library Versions" section of the
release notes?

<...>

> @@ -0,0 +1,26 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2017 Intel Corporation. All rights reserved.

Can drop "All rights reserved." part, for all files.

<...>

> +	RTE_LOG(INFO, EAL, "Registered [%s_%s] broker.\n", broker->bus,
> +		broker->device);

Shouldn't use EAL type, this is a new library and it should dynamically register
its new type. Same for all log occurrence.

<...>

> +	TAILQ_FOREACH(broker, &broker_list, next) {
> +		if ((strcmp(broker->bus, bus) == 0) &&
> +				(strcmp(broker->device, device) == 0))
> +			break;
> +	}

Is there any type of synchronization required to protect the list?

<...>

> +#define RTE_REPRESENTOR_PORT_VALID_ETHDEV_OR_RET_ERR(ethdev, retval) do { \
> +	if (!strstr(ethdev->data->name, RTE_PORT_REPRESENTOR_DEVICE_NAME)) { \
> +		RTE_PMD_DEBUG_TRACE("port %d is not a representor port", \

RTE_PMD_DEBUG_TRACE prints in PMD type, please don't use it in this library.

<...>

> +	RTE_LOG(INFO, PMD, "Creating representor for VF %i from %s\n",
> +		vport_id, pf_addr_str);

Please don't use PMD type logs in library.

<...>

> +	/* Allocate private ethdev data for Port Representor info */
> +	rep_ethdev->data->dev_private = rte_malloc("rte_representor_port",
> +			sizeof(struct rte_representor_port), 0);
> +	if (rep_ethdev == NULL) {
> +		RTE_LOG(ERR, PMD, "Unable to allocate Port Representor"
> +			" private ethdev data\n");
> +		rte_eth_dev_release_port(rep_ethdev);

free rep_ethdev->data->mac_addrs ?

> +		return -ENOMEM;
> +	}
> +
> +	/* Allocate an rte_device & rte_driver for the ethdev. */
> +	rep_port->rep_pcidev = rte_zmalloc_socket("rte_device",
> +		sizeof(struct rte_device), 0, rte_socket_id());
> +	if (rep_port->rep_pcidev == NULL) {
> +		RTE_LOG(ERR, PMD, "Unable to allocate Port Representor"
> +			" devive\n");
> +		return -ENOMEM;
> +	}
> +	rep_port->rep_pcidrv = rte_zmalloc_socket("rte_driver",
> +		sizeof(struct rte_driver), 0, rte_socket_id());
> +	if (rep_port->rep_pcidrv == NULL) {
> +		RTE_LOG(ERR, PMD, "Unable to allocate Port Representor"
> +			" driver\n");
> +		return -ENOMEM;
> +	}
> +	rep_port->rep_pcidrv->name = RTE_PORT_REPRESENTOR_DEVICE_NAME;
> +	rep_port->rep_pcidev->driver = rep_port->rep_pcidrv;
> +	rep_ethdev->device = rep_port->rep_pcidev;
> +
> +	/* Register representor with broker */
> +	RTE_REPRESENTOR_PORT_VALID_ETHDEV_OR_RET_ERR(rep_ethdev, -EINVAL);
> +	rep_port->vport_id = vport_id;
> +	rep_port->ethdev = rep_ethdev;
> +	rep_port->broker = broker;
> +	rep_port->state = RTE_REPRESENTOR_PORT_VALID;
> +	rep_ethdev->data->dev_private = rep_port;

If so why allocated rep_ethdev->data->dev_private a few lines above? Am I
missing something?

> +
> +	RTE_FUNC_PTR_OR_ERR_RET(broker->ops->port_init, -ENOTSUP);

Do we need to free anything in case error?

> +	retval = broker->ops->port_init(broker, rep_ethdev);
> +	if (retval) {
> +		rte_eth_dev_release_port(rep_ethdev);

Need to free allocated mem.

<...>

> +	/* Free up representor */
> +	RTE_FUNC_PTR_OR_ERR_RET(rep_port->broker->ops->port_uninit, -ENOTSUP);
> +	rep_port->broker->ops->port_uninit(rep_port->broker, rep_port->ethdev);
> +	rep_port->state = RTE_REPRESENTOR_PORT_INVALID;
> +	rte_eth_dev_release_port(rep_port->ethdev);

I guess need to free data->mac_addrs too

<...>

> +/**
> + * Unregister a representor port, called during destruction of representor
> + * ethdev context. Freeing any allocated memory for the representor port.
> + *
> + * @param	pf_addr_str	Address of parent PF in Bus_DomBDF format
> + * @param	vport_id	Virtual port ID to deregister
> + *
> + * @return
> + * - 0 on success
> + * - errno on failure
> + */
> +int
> +rte_representor_port_unregister(char *pf_addr_str, uint32_t vport_id);

There was discussion about having new APIs as EXPERIMENTAL for a release, is it
agreed on. John, Luca do you remember?

<...>

> +struct rte_representor_broker {
> +	TAILQ_ENTRY(rte_representor_broker) next;
> +	/**< Next broker object in linked list */
> +
> +	const char *bus;	/**< bus name */
> +	const char *device;	/**< device name */
> +
> +	uint16_t nb_virtual_ports;
> +	/**< number of virtual ports supported by device */
> +
> +	struct rte_representor_port *vports;
> +	/**< Representor port contexts */
> +
> +	struct rte_representor_broker_port_ops *ops;
> +	/**< broker port operations functions */
> +	void *private_data;
> +	/**< broker private data */

Is this private_data used?

> +};
> +
> +/** Port Representor */
> +struct rte_representor_port {
> +	uint16_t vport_id;
> +	/**< Virtual Port Identifier */
> +	struct rte_eth_dev *ethdev;
> +	/**< ethdev handle of representor port */
> +	struct rte_representor_broker *broker;
> +	/**< Broker handle to allow reverse lookup */
> +	enum {
> +		RTE_REPRESENTOR_PORT_INVALID,
> +		/**< No ethdev instantiated for virtual port */
> +		RTE_REPRESENTOR_PORT_VALID
> +		/**< ethdev active for virtual port */
> +	} state;

What do you think moving enum decleration out of struct, for simplicity?

> +	/**< port state */
> +	void *priv_data;
> +	/**<  port private data */
> +	/**< Representor ethdev device */
> +	struct rte_device *rep_pcidev;
> +	/**< Representor ethdev driver */
> +	struct rte_driver *rep_pcidrv;

Is this only for pci bus? If not it can be good to rename.