From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by dpdk.org (Postfix) with ESMTP id E64931B1A6 for ; Tue, 9 Jan 2018 23:06:36 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 09 Jan 2018 14:06:34 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.46,337,1511856000"; d="scan'208";a="9470570" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.237.220.48]) ([10.237.220.48]) by orsmga008.jf.intel.com with ESMTP; 09 Jan 2018 14:06:32 -0800 To: Remy Horton , dev@dpdk.org Cc: John McNamara , Wenzhuo Lu , Jingjing Wu , Declan Doherty , Mohammad Abdul Awal , Luca Boccassi References: <20180108143720.7994-1-remy.horton@intel.com> <20180108143720.7994-2-remy.horton@intel.com> From: Ferruh Yigit Message-ID: Date: Tue, 9 Jan 2018 22:06:31 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: <20180108143720.7994-2-remy.horton@intel.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v4 1/5] lib: add Port Representor library X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 09 Jan 2018 22:06:37 -0000 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 > Signed-off-by: Mohammad Abdul Awal > Signed-off-by: Remy Horton <...> > @@ -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.