From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f66.google.com (mail-wm0-f66.google.com [74.125.82.66]) by dpdk.org (Postfix) with ESMTP id 516455587 for ; Thu, 17 Nov 2016 13:53:24 +0100 (CET) Received: by mail-wm0-f66.google.com with SMTP id m203so20951626wma.3 for ; Thu, 17 Nov 2016 04:53:24 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=w08lg1cfx5SoubYMhf5ppkEHVUGVQomvawItlhlhdnc=; b=Ie9ip0UaR7+F52M0kKgjhBxfl1nDzwPj2UDw/mjwY3cWsC7WiSB4j0sbtXxoXs/5Tm JWO+aNgrzf0rK8CvwD9mdqawI6f2ODApzo0ixPbAAtei5oLOlEegHRZoiAqLcXU9RbVi /2xfXpmhDIdM/tSseVQDulVgJw4ziCrIQ4Km/ohiZRyyqVmK5eQQkYTpfeofUliBqYUc fQzmeRBBhyNZNmarIjGDImUT+CpT9STwvGR8pt6xJEH+6N4Aa92q2hJeoYtfwSOTnQ9a NbQBWUYdLmIZxYObUmG05xoBLoh0FNulwut/YUOS08juACl19ug9xm525DmP+erhAigx FWxQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc; bh=w08lg1cfx5SoubYMhf5ppkEHVUGVQomvawItlhlhdnc=; b=cT6t+MFHKrPyEDvPF1DjDVb/bBLumBIgYx4YuqgONbETmFgsWz8ZrlEYs7XWLHFvA6 a4zkG5JtP5KcgoEpjyjRLp1kr+aRJO2j9sfIrJlMxQPNW5iD9lgMhKJUV1vcROlbn8qz BYR1Oa7yY62Xuz7uf0TZspfd+/KN9jf1P6TPJQgSWhM2lfV6ceEBvTWM5Fa2mHbUC/f4 5E4oJ+iaSKFgnruOyQRqm4qMRBv/2XZCEbU08PeLB63CTfDvZchq180VQds+Mully9qn d018knBEEuf4Sj9thnTFumLyvfK/JSS2spew73FLl78nd0wSifyy+PAdkaClImh862Uj YZDw== X-Gm-Message-State: AKaTC00ABKhlYZT4d1kYbnr8Gb7IxO17D4t4KqlTNUWUdTIjtxHuRuHuZ2jwVzfWcCIvjDaOU1OYXK7PCZoiTg== X-Received: by 10.194.95.194 with SMTP id dm2mr1910418wjb.207.1479387203709; Thu, 17 Nov 2016 04:53:23 -0800 (PST) MIME-Version: 1.0 Sender: jblunck@gmail.com Received: by 10.28.191.8 with HTTP; Thu, 17 Nov 2016 04:53:23 -0800 (PST) In-Reply-To: <1479360605-20558-7-git-send-email-shreyansh.jain@nxp.com> References: <1479360605-20558-1-git-send-email-shreyansh.jain@nxp.com> <1479360605-20558-7-git-send-email-shreyansh.jain@nxp.com> From: Jan Blunck Date: Thu, 17 Nov 2016 13:53:23 +0100 X-Google-Sender-Auth: USXqAgnpvONlQn1wQNzyZ-KsQiI Message-ID: To: Shreyansh Jain Cc: David Marchand , dev@dpdk.org Content-Type: text/plain; charset=UTF-8 Subject: Re: [dpdk-dev] [RFC PATCH 6/6] eal: removing eth_driver X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 17 Nov 2016 12:53:24 -0000 On Thu, Nov 17, 2016 at 6:30 AM, Shreyansh Jain wrote: > This patch demonstrates how eth_driver can be replaced with appropriate > changes for rte_xxx_driver from the PMD itself. It uses ixgbe_ethernet as > an example. > > A large set of changes exists in the rte_ethdev.c - primarily because too > much PCI centric code (names, assumption of rte_pci_device) still exists > in it. Most, except symbol naming, has been changed in this patch. > > This proposes that: > - PMD would declare the rte_xxx_driver. In case of ixgbe, it would be > rte_pci_driver. > - Probe and remove continue to exists in rte_pci_driver. But, the > rte_driver has new hooks for init and uninit. The rationale is that > once a ethernet or cryto device is created, the rte_driver->init would > be responsible for initializing the device. > -- Eth_dev -> rte_driver -> rte_pci_driver > | `-> probe/remove > `--> init/uninit Hmm, from my perspective this moves struct rte_driver a step closer to struct rte_eth_dev instead of decoupling them. It is up to the rte_driver->probe if it wants to allocate a struct rte_eth_dev, rte_crypto_dev or the famous rte_foo_dev. Instead of explicitly modelling rte_eth_dev specifics like init, unit or dev_private_size I think we should delegate this to the rte_driver->probe instead. Most of what is in rte_eth_dev_pci_probe() today is anyway a rte_eth_dev_allocate_priv() anyway. I already have some patches in this area in my patch stack. > - necessary changes in the rte_eth_dev have also been done so that it > refers to the rte_device and rte_driver rather than rte_xxx_*. This > would imply, ethernet device is 'linked' to a rte_device/rte_driver > which in turn is a rte_xxx_device/rte_xxx_driver type. > - for all operations related to extraction relvant xxx type, > container_of would have to be used. > > Signed-off-by: Shreyansh Jain > --- > drivers/net/ixgbe/ixgbe_ethdev.c | 49 +++++++++++++++++++++------------------- > lib/librte_ether/rte_ethdev.c | 36 +++++++++++++++++------------ > lib/librte_ether/rte_ethdev.h | 6 ++--- > 3 files changed, 51 insertions(+), 40 deletions(-) > > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c > index edc9b22..acead31 100644 > --- a/drivers/net/ixgbe/ixgbe_ethdev.c > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c > @@ -1419,7 +1419,7 @@ eth_ixgbevf_dev_init(struct rte_eth_dev *eth_dev) > return 0; > } > > - pci_dev = eth_dev->pci_dev; > + pci_dev = container_of(eth_dev->device, struct rte_pci_device, device); > > rte_eth_copy_pci_info(eth_dev, pci_dev); > > @@ -1532,7 +1532,9 @@ static int > eth_ixgbevf_dev_uninit(struct rte_eth_dev *eth_dev) > { > struct ixgbe_hw *hw; > - struct rte_pci_device *pci_dev = eth_dev->pci_dev; > + struct rte_pci_device *pci_dev; > + > + pci_dev = container_of(eth_dev->device, struct rte_pci_device, device); > > PMD_INIT_FUNC_TRACE(); > > @@ -1562,32 +1564,33 @@ eth_ixgbevf_dev_uninit(struct rte_eth_dev *eth_dev) > return 0; > } > > -static struct eth_driver rte_ixgbe_pmd = { > - .pci_drv = { > - .id_table = pci_id_ixgbe_map, > - .drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC | > - RTE_PCI_DRV_DETACHABLE, > - .probe = rte_eth_dev_pci_probe, > - .remove = rte_eth_dev_pci_remove, > +static struct rte_pci_driver rte_ixgbe_pci_driver = { > + .id_table = pci_id_ixgbe_map, > + .drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC | > + RTE_PCI_DRV_DETACHABLE, > + .probe = rte_eth_dev_pci_probe, > + .remove = rte_eth_dev_pci_remove, > + .driver = { > + .driver_init_t= eth_ixgbe_dev_init, > + .driver_uninit_t= eth_ixgbe_dev_uninit, > + .dev_private_size = sizeof(struct ixgbe_adapter), > }, > - .eth_dev_init = eth_ixgbe_dev_init, > - .eth_dev_uninit = eth_ixgbe_dev_uninit, > - .dev_private_size = sizeof(struct ixgbe_adapter), > }; > > /* > * virtual function driver struct > */ > -static struct eth_driver rte_ixgbevf_pmd = { > - .pci_drv = { > - .id_table = pci_id_ixgbevf_map, > - .drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_DETACHABLE, > - .probe = rte_eth_dev_pci_probe, > - .remove = rte_eth_dev_pci_remove, > +static struct rte_pci_driver rte_ixgbevf_pci_driver = { > + .id_table = pci_id_ixgbevf_map, > + .drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_DETACHABLE, > + .probe = rte_eth_dev_pci_probe, > + .remove = rte_eth_dev_pci_remove, > + .driver = { > + /* rte_driver hooks */ > + .init = eth_ixgbevf_dev_init, > + .uninit = eth_ixgbevf_dev_uninit, > + .dev_private_size = sizeof(struct ixgbe_adapter), > }, > - .eth_dev_init = eth_ixgbevf_dev_init, > - .eth_dev_uninit = eth_ixgbevf_dev_uninit, > - .dev_private_size = sizeof(struct ixgbe_adapter), > }; > > static int > @@ -7592,7 +7595,7 @@ ixgbevf_dev_interrupt_handler(__rte_unused struct rte_intr_handle *handle, > ixgbevf_dev_interrupt_action(dev); > } > > -RTE_PMD_REGISTER_PCI(net_ixgbe, rte_ixgbe_pmd.pci_drv); > +RTE_PMD_REGISTER_PCI(net_ixgbe, rte_ixgbe_pci_driver); > RTE_PMD_REGISTER_PCI_TABLE(net_ixgbe, pci_id_ixgbe_map); > -RTE_PMD_REGISTER_PCI(net_ixgbe_vf, rte_ixgbevf_pmd.pci_drv); > +RTE_PMD_REGISTER_PCI(net_ixgbe_vf, rte_ixgbevf_pci_driver); > RTE_PMD_REGISTER_PCI_TABLE(net_ixgbe_vf, pci_id_ixgbevf_map); > diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c > index fde8112..3535ff4 100644 > --- a/lib/librte_ether/rte_ethdev.c > +++ b/lib/librte_ether/rte_ethdev.c > @@ -235,13 +235,13 @@ int > rte_eth_dev_pci_probe(struct rte_pci_driver *pci_drv, > struct rte_pci_device *pci_dev) > { > - struct eth_driver *eth_drv; > + struct rte_driver *drv; > struct rte_eth_dev *eth_dev; > char ethdev_name[RTE_ETH_NAME_MAX_LEN]; > > int diag; > > - eth_drv = (struct eth_driver *)pci_drv; > + drv = pci_drv->driver; > > rte_eal_pci_device_name(&pci_dev->addr, ethdev_name, > sizeof(ethdev_name)); > @@ -252,13 +252,13 @@ rte_eth_dev_pci_probe(struct rte_pci_driver *pci_drv, > > if (rte_eal_process_type() == RTE_PROC_PRIMARY) { > eth_dev->data->dev_private = rte_zmalloc("ethdev private structure", > - eth_drv->dev_private_size, > + drv->dev_private_size, > RTE_CACHE_LINE_SIZE); > if (eth_dev->data->dev_private == NULL) > rte_panic("Cannot allocate memzone for private port data\n"); > } > - eth_dev->pci_dev = pci_dev; > - eth_dev->driver = eth_drv; > + eth_dev->device = pci_dev->device; > + eth_dev->driver = drv; > eth_dev->data->rx_mbuf_alloc_failed = 0; > > /* init user callbacks */ > @@ -270,7 +270,7 @@ rte_eth_dev_pci_probe(struct rte_pci_driver *pci_drv, > eth_dev->data->mtu = ETHER_MTU; > > /* Invoke PMD device initialization function */ > - diag = (*eth_drv->eth_dev_init)(eth_dev); > + diag = (*drv->init)(eth_dev); > if (diag == 0) > return 0; > > @@ -287,7 +287,7 @@ rte_eth_dev_pci_probe(struct rte_pci_driver *pci_drv, > int > rte_eth_dev_pci_remove(struct rte_pci_device *pci_dev) > { > - const struct eth_driver *eth_drv; > + const struct rte_driver *drv; > struct rte_eth_dev *eth_dev; > char ethdev_name[RTE_ETH_NAME_MAX_LEN]; > int ret; > @@ -302,11 +302,11 @@ rte_eth_dev_pci_remove(struct rte_pci_device *pci_dev) > if (eth_dev == NULL) > return -ENODEV; > > - eth_drv = (const struct eth_driver *)pci_dev->driver; > + drv = pci_dev->driver; > > /* Invoke PMD device uninit function */ > - if (*eth_drv->eth_dev_uninit) { > - ret = (*eth_drv->eth_dev_uninit)(eth_dev); > + if (*drv->uninit) { > + ret = (*drv->uninit)(eth_dev); > if (ret) > return ret; > } > @@ -317,7 +317,7 @@ rte_eth_dev_pci_remove(struct rte_pci_device *pci_dev) > if (rte_eal_process_type() == RTE_PROC_PRIMARY) > rte_free(eth_dev->data->dev_private); > > - eth_dev->pci_dev = NULL; > + eth_dev->device = NULL; > eth_dev->driver = NULL; > eth_dev->data = NULL; > > @@ -1556,7 +1556,7 @@ rte_eth_dev_info_get(uint8_t port_id, struct rte_eth_dev_info *dev_info) > > RTE_FUNC_PTR_OR_RET(*dev->dev_ops->dev_infos_get); > (*dev->dev_ops->dev_infos_get)(dev, dev_info); > - dev_info->pci_dev = dev->pci_dev; > + dev_info->device = dev->device; > dev_info->driver_name = dev->data->drv_name; > dev_info->nb_rx_queues = dev->data->nb_rx_queues; > dev_info->nb_tx_queues = dev->data->nb_tx_queues; > @@ -2537,6 +2537,7 @@ rte_eth_dev_rx_intr_ctl(uint8_t port_id, int epfd, int op, void *data) > { > uint32_t vec; > struct rte_eth_dev *dev; > + struct rte_pci_device *pci_dev; > struct rte_intr_handle *intr_handle; > uint16_t qid; > int rc; > @@ -2544,6 +2545,10 @@ rte_eth_dev_rx_intr_ctl(uint8_t port_id, int epfd, int op, void *data) > RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); > > dev = &rte_eth_devices[port_id]; > + /* TODO intr_handle is currently in rte_pci_device; > + * Below is incorrect until that time > + */ > + pci_dev = container_of(dev->device, struct rte_pci_device, device); > intr_handle = &dev->pci_dev->intr_handle; > if (!intr_handle->intr_vec) { > RTE_PMD_DEBUG_TRACE("RX Intr vector unset\n"); > @@ -2572,7 +2577,7 @@ rte_eth_dma_zone_reserve(const struct rte_eth_dev *dev, const char *ring_name, > const struct rte_memzone *mz; > > snprintf(z_name, sizeof(z_name), "%s_%s_%d_%d", > - dev->driver->pci_drv.driver.name, ring_name, > + dev->driver->name, ring_name, > dev->data->port_id, queue_id); > > mz = rte_memzone_lookup(z_name); > @@ -2593,6 +2598,7 @@ rte_eth_dev_rx_intr_ctl_q(uint8_t port_id, uint16_t queue_id, > { > uint32_t vec; > struct rte_eth_dev *dev; > + struct rte_pci_device *pci_dev; > struct rte_intr_handle *intr_handle; > int rc; > > @@ -2604,7 +2610,9 @@ rte_eth_dev_rx_intr_ctl_q(uint8_t port_id, uint16_t queue_id, > return -EINVAL; > } > > - intr_handle = &dev->pci_dev->intr_handle; > + /* TODO; Until intr_handle is available in rte_device, below is incorrect */ > + pci_dev = container_of(dev->device, struct rte_pci_device, device); > + intr_handle = &pci_dev->intr_handle; > if (!intr_handle->intr_vec) { > RTE_PMD_DEBUG_TRACE("RX Intr vector unset\n"); > return -EPERM; > diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h > index 38641e8..2b1d826 100644 > --- a/lib/librte_ether/rte_ethdev.h > +++ b/lib/librte_ether/rte_ethdev.h > @@ -876,7 +876,7 @@ struct rte_eth_conf { > * Ethernet device information > */ > struct rte_eth_dev_info { > - struct rte_pci_device *pci_dev; /**< Device PCI information. */ > + struct rte_device *device; /**< Device PCI information. */ We already the situation that virtual devices don't set the pci_dev field. I wonder if it really makes sense to replace it with a struct rte_device because that is not adding a lot of value (only numa_node). If we break ABI we might want to add numa_node and dev_flags as suggested by Stephen Hemminger already. If we choose to not break ABI we can delegate the population of the pci_dev field to dev_infos_get. I already have that patch in my patch stack too. The problem with rte_eth_dev_info is that it doesn't support extensions. Maybe its time to add rte_eth_dev_info_ex() ... or rte_eth_dev_xinfo() if you don't like the IB verbs API. > const char *driver_name; /**< Device Driver name. */ > unsigned int if_index; /**< Index to bound host interface, or 0 if none. > Use if_indextoname() to translate into an interface name. */ > @@ -1623,9 +1623,9 @@ struct rte_eth_dev { > eth_rx_burst_t rx_pkt_burst; /**< Pointer to PMD receive function. */ > eth_tx_burst_t tx_pkt_burst; /**< Pointer to PMD transmit function. */ > struct rte_eth_dev_data *data; /**< Pointer to device data */ > - const struct eth_driver *driver;/**< Driver for this device */ > + const struct rte_driver *driver;/**< Driver for this device */ > const struct eth_dev_ops *dev_ops; /**< Functions exported by PMD */ > - struct rte_pci_device *pci_dev; /**< PCI info. supplied by probing */ > + struct rte_device *device; /**< Device instance */ > /** User application callbacks for NIC interrupts */ > struct rte_eth_dev_cb_list link_intr_cbs; > /** > -- > 2.7.4 >