From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [67.231.154.164]) by dpdk.org (Postfix) with ESMTP id 0CBCB5699 for ; Tue, 16 Oct 2018 13:16:51 +0200 (CEST) X-Virus-Scanned: Proofpoint Essentials engine Received: from webmail.solarflare.com (uk.solarflare.com [193.34.186.16]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by mx1-us1.ppe-hosted.com (Proofpoint Essentials ESMTP Server) with ESMTPS id C348278006E; Tue, 16 Oct 2018 11:16:49 +0000 (UTC) Received: from [192.168.38.17] (91.220.146.112) by ukex01.SolarFlarecom.com (10.17.10.4) with Microsoft SMTP Server (TLS) id 15.0.1395.4; Tue, 16 Oct 2018 12:16:44 +0100 To: Thomas Monjalon , CC: , , Rahul Lakkireddy References: <20180907233929.21950-1-thomas@monjalon.net> <20181014232020.12114-1-thomas@monjalon.net> <20181014232020.12114-2-thomas@monjalon.net> From: Andrew Rybchenko Message-ID: <95172f2a-41d4-aadf-65b0-0e4c57fca826@solarflare.com> Date: Tue, 16 Oct 2018 14:16:04 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.0 MIME-Version: 1.0 In-Reply-To: <20181014232020.12114-2-thomas@monjalon.net> Content-Language: en-GB X-Originating-IP: [91.220.146.112] X-ClientProxiedBy: ocex03.SolarFlarecom.com (10.20.40.36) To ukex01.SolarFlarecom.com (10.17.10.4) X-TM-AS-Product-Ver: SMEX-12.5.0.1300-8.5.1010-24158.003 X-TM-AS-Result: No-19.424800-8.000000-10 X-TMASE-MatchedRID: byfwvk+IcRkOwH4pD14DsPHkpkyUphL97yWPaQc4INT9nZJIPtHyFDuY BlClHjLc4MlnMHmZOwoYbwMqNgMVX294Ipa1otxopvwZ9GmdwDPEk+DMet9i+hxEHPT/dphZ2Zi Ob8LFs7umRwvkMsSvgh8oz5BPysqkOnfMHqHi4mkSEYfcJF0pRQrefVId6fzV80VZ5SgxdNBxEW 0uzmaRA19/NT1utweUM2ZXtJf8ligS3pAGY8tVrdkBRUb8BZm+CQ3xS+zL6e1BTdeoy0R9DwvvF 2DvnUj/pEYBIO560crrrT4Oq9+KU4IiDu0n/+6xNNHZMWDTEbeJ+A3Xg/7izcO/l0Ny5PZ5GoZz uF63mGbHllBxKcTyRR3DeMfpbWC96KhkLBjPN7AHGzB42DcRoYWQKSQHRQw2myiLZetSf8nJ4y0 wP1A6AKEwgORH8p/AIhDmZnlKoc8D/17GK7TEyMvtsQc5EjGJAuGALilEGQnNPE7EyPeZzfIF9Q 8QprY7 X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-TMASE-Result: 10--19.424800-8.000000 X-TMASE-Version: SMEX-12.5.0.1300-8.5.1010-24158.003 X-MDID: 1539688610-pXoQlw3E9kDj Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [PATCH v3 1/2] ethdev: free all common data when releasing port 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, 16 Oct 2018 11:16:51 -0000 On 10/15/18 2:20 AM, Thomas Monjalon wrote: > This is a clean-up of common ethdev data freeing. > All data freeing are moved to rte_eth_dev_release_port() > and done only in case of primary process. > > It is probably fixing some memory leaks for PMDs which were > not freeing all data. > > Signed-off-by: Thomas Monjalon <...> > diff --git a/drivers/net/cxgbe/cxgbe_main.c b/drivers/net/cxgbe/cxgbe_main.c > index a135df9c7..88dc851f8 100644 > --- a/drivers/net/cxgbe/cxgbe_main.c > +++ b/drivers/net/cxgbe/cxgbe_main.c > @@ -1710,12 +1710,7 @@ void cxgbe_close(struct adapter *adapter) > if (pi->viid != 0) > t4_free_vi(adapter, adapter->mbox, > adapter->pf, 0, pi->viid); > - rte_free(pi->eth_dev->data->mac_addrs); > - /* Skip first port since it'll be freed by DPDK stack */ > - if (i) { > - rte_free(pi->eth_dev->data->dev_private); > - rte_eth_dev_release_port(pi->eth_dev); > - } > + rte_eth_dev_release_port(pi->eth_dev); > } > adapter->flags &= ~FULL_INIT_DONE; > } > @@ -1918,14 +1913,7 @@ int cxgbe_probe(struct adapter *adapter) > if (pi->viid != 0) > t4_free_vi(adapter, adapter->mbox, adapter->pf, > 0, pi->viid); > - /* Skip first port since it'll be de-allocated by DPDK */ > - if (i == 0) > - continue; > - if (pi->eth_dev) { > - if (pi->eth_dev->data->dev_private) > - rte_free(pi->eth_dev->data->dev_private); > - rte_eth_dev_release_port(pi->eth_dev); > - } > + rte_eth_dev_release_port(pi->eth_dev); > } > > if (adapter->flags & FW_OK) > diff --git a/drivers/net/cxgbe/cxgbevf_main.c b/drivers/net/cxgbe/cxgbevf_main.c > index 4214d0312..6223e1250 100644 > --- a/drivers/net/cxgbe/cxgbevf_main.c > +++ b/drivers/net/cxgbe/cxgbevf_main.c > @@ -282,14 +282,7 @@ int cxgbevf_probe(struct adapter *adapter) > if (pi->viid != 0) > t4_free_vi(adapter, adapter->mbox, adapter->pf, > 0, pi->viid); > - /* Skip first port since it'll be de-allocated by DPDK */ > - if (i == 0) > - continue; > - if (pi->eth_dev) { > - if (pi->eth_dev->data->dev_private) > - rte_free(pi->eth_dev->data->dev_private); > - rte_eth_dev_release_port(pi->eth_dev); > - } > + rte_eth_dev_release_port(pi->eth_dev); > } > return -err; > } Above changes looks frightening. I've add cxgbe maintainer in CC. > diff --git a/drivers/net/softnic/rte_eth_softnic.c b/drivers/net/softnic/rte_eth_softnic.c > index 0fd264e25..f01ed36b6 100644 > --- a/drivers/net/softnic/rte_eth_softnic.c > +++ b/drivers/net/softnic/rte_eth_softnic.c > @@ -556,7 +556,6 @@ static int > pmd_remove(struct rte_vdev_device *vdev) > { > struct rte_eth_dev *dev = NULL; > - struct pmd_internals *p; > > if (!vdev) > return -EINVAL; > @@ -567,12 +566,11 @@ pmd_remove(struct rte_vdev_device *vdev) > dev = rte_eth_dev_allocated(rte_vdev_device_name(vdev)); > if (dev == NULL) > return -ENODEV; > - p = dev->data->dev_private; > > /* Free device data structures*/ > - rte_free(dev->data); > + pmd_free(dev->data->dev_private); > + dev->data->dev_private = NULL; > rte_eth_dev_release_port(dev); > - pmd_free(p); > > return 0; > } The above basically violates approach used everywhere else. It is OK to go, but should be reconsidered. > diff --git a/lib/librte_ethdev/rte_ethdev_pci.h b/lib/librte_ethdev/rte_ethdev_pci.h > index f652596f4..23257e986 100644 > --- a/lib/librte_ethdev/rte_ethdev_pci.h > +++ b/lib/librte_ethdev/rte_ethdev_pci.h > @@ -135,17 +135,6 @@ rte_eth_dev_pci_allocate(struct rte_pci_device *dev, size_t private_data_size) > static inline void > rte_eth_dev_pci_release(struct rte_eth_dev *eth_dev) > { > - if (rte_eal_process_type() == RTE_PROC_PRIMARY) > - rte_free(eth_dev->data->dev_private); > - > - eth_dev->data->dev_private = NULL; > - > - /* > - * Secondary process will check the name to attach. > - * Clear this field to avoid attaching a released ports. > - */ > - eth_dev->data->name[0] = '\0'; > - > eth_dev->device = NULL; > eth_dev->intr_handle = NULL; Not directly related to the patch, but I don't understand why does rte_eth_dev_pci_release () exist? It does nothing PCI specific. May be just for symmetry to rte_eth_dev_pci_allocate(). Why is intr_handle set to NULL above? Is it PCI specific? It does not look so, since failsafe uses it. In general it looks good, really big work, but ideally it should be acked by cxgbe maintainer as well. Acked-by: Andrew Rybchenko