From: Ferruh Yigit <ferruh.yigit@intel.com>
To: Remy Horton <remy.horton@intel.com>, dev@dpdk.org
Cc: John McNamara <john.mcnamara@intel.com>,
Wenzhuo Lu <wenzhuo.lu@intel.com>,
Jingjing Wu <jingjing.wu@intel.com>,
Declan Doherty <declan.doherty@intel.com>,
Mohammad Abdul Awal <mohammad.abdul.awal@intel.com>,
Luca Boccassi <bluca@debian.org>
Subject: Re: [dpdk-dev] [PATCH v4 1/5] lib: add Port Representor library
Date: Tue, 9 Jan 2018 22:06:31 +0000 [thread overview]
Message-ID: <f83d9741-0924-609b-77d1-ff9a80a43eac@intel.com> (raw)
In-Reply-To: <20180108143720.7994-2-remy.horton@intel.com>
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@intel.com>
> Signed-off-by: Mohammad Abdul Awal <mohammad.abdul.awal@intel.com>
> Signed-off-by: Remy Horton <remy.horton@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.
next prev parent reply other threads:[~2018-01-09 22:06 UTC|newest]
Thread overview: 75+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-11-17 14:42 [dpdk-dev] [PATCH v2 1/6] ethdev: added switch_domain and representor port flag Mohammad Abdul Awal
2017-11-17 14:42 ` [dpdk-dev] [PATCH v2 2/6] lib/representor: port representor library to manage broker infrastructure and representor PMDs Mohammad Abdul Awal
2017-11-17 23:03 ` Ferruh Yigit
2017-12-08 14:57 ` Mohammad Abdul Awal
2017-11-17 14:42 ` [dpdk-dev] [PATCH v2 3/6] eal: added --enable-representor command line argument in EAL to load representor broker infrastructure Mohammad Abdul Awal
2017-11-17 14:42 ` [dpdk-dev] [PATCH v2 4/6] net/representor: Implement port representor PMD Mohammad Abdul Awal
2017-11-20 7:46 ` Ferruh Yigit
2017-12-08 15:02 ` Remy Horton
2017-12-08 16:56 ` Mohammad Abdul Awal
2017-12-11 10:08 ` Remy Horton
2017-12-11 15:11 ` Mohammad Abdul Awal
2017-12-11 15:29 ` Remy Horton
2017-11-17 14:42 ` [dpdk-dev] [PATCH v2 5/6] net/i40e: Enable port representor PMD and broker for fortville PMD driver Mohammad Abdul Awal
2017-11-20 7:48 ` Ferruh Yigit
2017-11-17 14:42 ` [dpdk-dev] [PATCH v2 6/6] net/ixgbe: Enable port representor PMD and broker for ixgbe " Mohammad Abdul Awal
2017-11-20 19:57 ` [dpdk-dev] [PATCH v2 1/6] ethdev: added switch_domain and representor port flag Ferruh Yigit
2017-12-08 15:33 ` Mohammad Abdul Awal
2017-12-11 13:45 ` Alex Rosenbaum
2017-12-11 18:00 ` Mohammad Abdul Awal
2017-12-22 14:41 ` [dpdk-dev] [DPDK 0/5] lib: add Port Representors Remy Horton
2017-12-22 14:41 ` [dpdk-dev] [DPDK 1/5] lib: add Port Representor library Remy Horton
2017-12-22 14:41 ` [dpdk-dev] [DPDK 2/5] eal: add Port Representor command-line option Remy Horton
2017-12-22 14:41 ` [dpdk-dev] [DPDK 3/5] drivers/net/i40e: add Port Representor functionality Remy Horton
2017-12-22 14:41 ` [dpdk-dev] [DPDK 4/5] drivers/net/ixgbe: " Remy Horton
2017-12-22 14:41 ` [dpdk-dev] [DPDK 5/5] app/test-pmd: add Port Representor commands Remy Horton
2017-12-22 15:09 ` [dpdk-dev] [DPDK 0/5] lib: add Port Representors Remy Horton
2017-12-22 16:37 ` Neil Horman
2018-01-03 12:14 ` Mohammad Abdul Awal
2017-12-22 14:52 ` [dpdk-dev] [PATCH v3 " Remy Horton
2017-12-22 14:52 ` [dpdk-dev] [PATCH v3 1/5] lib: add Port Representor library Remy Horton
2017-12-22 14:52 ` [dpdk-dev] [PATCH v3 2/5] eal: add Port Representor command-line option Remy Horton
2017-12-22 14:52 ` [dpdk-dev] [PATCH v3 3/5] drivers/net/i40e: add Port Representor functionality Remy Horton
2017-12-22 14:52 ` [dpdk-dev] [PATCH v3 4/5] drivers/net/ixgbe: " Remy Horton
2017-12-22 14:52 ` [dpdk-dev] [PATCH v3 5/5] app/test-pmd: add Port Representor commands Remy Horton
2017-12-26 13:54 ` [dpdk-dev] [PATCH v3 0/5] lib: add Port Representors Neil Horman
2018-01-03 12:12 ` Mohammad Abdul Awal
2018-01-08 14:37 ` [dpdk-dev] [PATCH v4 " Remy Horton
2018-01-08 14:37 ` [dpdk-dev] [PATCH v4 1/5] lib: add Port Representor library Remy Horton
2018-01-09 22:06 ` Ferruh Yigit [this message]
2018-01-10 11:40 ` Remy Horton
2018-01-08 14:37 ` [dpdk-dev] [PATCH v4 2/5] eal: add Port Representor command-line option Remy Horton
2018-01-09 22:07 ` Ferruh Yigit
2018-01-08 14:37 ` [dpdk-dev] [PATCH v4 3/5] drivers/net/i40e: add Port Representor functionality Remy Horton
2018-01-09 22:09 ` Ferruh Yigit
2018-01-10 7:56 ` Xing, Beilei
2018-01-10 10:11 ` Mohammad Abdul Awal
2018-01-10 12:37 ` Xing, Beilei
2018-01-10 14:04 ` Mohammad Abdul Awal
2018-01-08 14:37 ` [dpdk-dev] [PATCH v4 4/5] drivers/net/ixgbe: " Remy Horton
2018-01-08 14:37 ` [dpdk-dev] [PATCH v4 5/5] app/test-pmd: add Port Representor commands Remy Horton
2018-01-09 22:10 ` Ferruh Yigit
2018-01-10 7:22 ` Remy Horton
2018-01-09 22:01 ` [dpdk-dev] [PATCH v4 0/5] lib: add Port Representors Ferruh Yigit
2018-01-10 14:17 ` Mohammad Abdul Awal
2018-01-09 23:22 ` Thomas Monjalon
2018-01-10 13:46 ` Doherty, Declan
2018-01-10 19:26 ` Thomas Monjalon
2018-01-15 12:12 ` Doherty, Declan
2018-01-15 16:09 ` Thomas Monjalon
2018-01-10 15:54 ` [dpdk-dev] [PATCH v5 " Remy Horton
2018-01-10 15:54 ` [dpdk-dev] [PATCH v5 1/5] lib: add Port Representor library Remy Horton
2018-01-30 9:23 ` Andrew Rybchenko
2018-01-10 15:54 ` [dpdk-dev] [PATCH v5 2/5] eal: add Port Representor command-line option Remy Horton
2018-01-10 15:54 ` [dpdk-dev] [PATCH v5 3/5] drivers/net/i40e: add Port Representor functionality Remy Horton
2018-01-10 15:54 ` [dpdk-dev] [PATCH v5 4/5] drivers/net/ixgbe: " Remy Horton
2018-01-10 15:54 ` [dpdk-dev] [PATCH v5 5/5] app/test-pmd: add Port Representor commands Remy Horton
2018-01-10 17:49 ` [dpdk-dev] [PATCH v5 0/5] lib: add Port Representors Remy Horton
2018-01-10 17:49 ` [dpdk-dev] [PATCH v5 1/5] lib: add Port Representor library Remy Horton
2018-01-10 17:49 ` [dpdk-dev] [PATCH v5 2/5] eal: add Port Representor command-line option Remy Horton
2018-01-10 17:49 ` [dpdk-dev] [PATCH v5 3/5] drivers/net/i40e: add Port Representor functionality Remy Horton
2018-01-10 17:49 ` [dpdk-dev] [PATCH v5 4/5] drivers/net/ixgbe: " Remy Horton
2018-01-10 17:49 ` [dpdk-dev] [PATCH v5 5/5] app/test-pmd: add Port Representor commands Remy Horton
2018-01-11 3:18 ` [dpdk-dev] [PATCH v4 0/5] lib: add Port Representors Yuanhan Liu
2018-01-11 8:37 ` Alex Rosenbaum
2018-01-11 9:38 ` Remy Horton
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f83d9741-0924-609b-77d1-ff9a80a43eac@intel.com \
--to=ferruh.yigit@intel.com \
--cc=bluca@debian.org \
--cc=declan.doherty@intel.com \
--cc=dev@dpdk.org \
--cc=jingjing.wu@intel.com \
--cc=john.mcnamara@intel.com \
--cc=mohammad.abdul.awal@intel.com \
--cc=remy.horton@intel.com \
--cc=wenzhuo.lu@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).