From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by dpdk.space (Postfix) with ESMTP id 9431AA0096 for ; Wed, 5 Jun 2019 17:17:20 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 512631BB74; Wed, 5 Jun 2019 17:17:20 +0200 (CEST) Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id 0A8AF1BB35 for ; Wed, 5 Jun 2019 17:17:18 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 05 Jun 2019 08:17:17 -0700 X-ExtLoop1: 1 Received: from fyigit-mobl.ger.corp.intel.com (HELO [10.237.221.26]) ([10.237.221.26]) by fmsmga006.fm.intel.com with ESMTP; 05 Jun 2019 08:17:16 -0700 To: Allain Legacy , thomas@monjalon.net Cc: dev@dpdk.org, Matt Peters References: <20190527170255.30853-1-allain.legacy@windriver.com> From: Ferruh Yigit Openpgp: preference=signencrypt Autocrypt: addr=ferruh.yigit@intel.com; prefer-encrypt=mutual; keydata= mQINBFXZCFABEADCujshBOAaqPZpwShdkzkyGpJ15lmxiSr3jVMqOtQS/sB3FYLT0/d3+bvy qbL9YnlbPyRvZfnP3pXiKwkRoR1RJwEo2BOf6hxdzTmLRtGtwWzI9MwrUPj6n/ldiD58VAGQ +iR1I/z9UBUN/ZMksElA2D7Jgg7vZ78iKwNnd+vLBD6I61kVrZ45Vjo3r+pPOByUBXOUlxp9 GWEKKIrJ4eogqkVNSixN16VYK7xR+5OUkBYUO+sE6etSxCr7BahMPKxH+XPlZZjKrxciaWQb +dElz3Ab4Opl+ZT/bK2huX+W+NJBEBVzjTkhjSTjcyRdxvS1gwWRuXqAml/sh+KQjPV1PPHF YK5LcqLkle+OKTCa82OvUb7cr+ALxATIZXQkgmn+zFT8UzSS3aiBBohg3BtbTIWy51jNlYdy ezUZ4UxKSsFuUTPt+JjHQBvF7WKbmNGS3fCid5Iag4tWOfZoqiCNzxApkVugltxoc6rG2TyX CmI2rP0mQ0GOsGXA3+3c1MCdQFzdIn/5tLBZyKy4F54UFo35eOX8/g7OaE+xrgY/4bZjpxC1 1pd66AAtKb3aNXpHvIfkVV6NYloo52H+FUE5ZDPNCGD0/btFGPWmWRmkPybzColTy7fmPaGz cBcEEqHK4T0aY4UJmE7Ylvg255Kz7s6wGZe6IR3N0cKNv++O7QARAQABtCVGZXJydWggWWln aXQgPGZlcnJ1aC55aWdpdEBpbnRlbC5jb20+iQJUBBMBCgA+AhsDAh4BAheABQkI71rKFiEE 0jZTh0IuwoTjmYHH+TPrQ98TYR8FAlznMMQFCwkIBwMFFQoJCAsFFgIDAQAACgkQ+TPrQ98T YR/B9Q//a57esjq996nfZVm7AsUl7zbvhN+Ojity25ib2gcSVVsAN2j6lcQS4hf6/OVvRj3q CgebJ4o2gXR6X12UzWBJL7NE8Xpc70MvUIe0r11ykurQ9n9jUaWMjxdSqBPF93hU+Z/MZe5M 1rW5O2VJLuTJzkDw3EYUCbHOwPjeaS8Qqj3RI0LYbGthbHBIp9CsjkgsJSjTT5GQ8AQWkE7I z+hvPx6f1rllfjxFyi4DI3jLhAI+j1Nm+l+ESyoX59HrLTHAvq4RPkLpTnGBj9gOnJ+5sVEr GE0fcffsNcuMSkpqSEoJCPAHmChoLgezskhhsy0BiU3xlSIj1Dx2XMDerUXFOK3ftlbYNRte HQy4EKubfZRB8H5Rvcpksom3fRBDcJT8zw+PTH14htRApU9f8I/RamQ7Ujks7KuaB7JX5QaG gMjfPzHGYX9PfF6KIchaFmAWLytIP1t0ht8LpJkjtvUCSQZ2VxpCXwKyUzPDIF3co3tp90o7 X07uiC5ymX0K0+Owqs6zeslLY6DMxNdt8ye+h1TVkSZ5g4dCs4C/aiEF230+luL1CnejOv/K /s1iSbXQzJNM7be3FlRUz4FdwsfKiJJF7xYALSBnSvEB04R7I2P2V9Zpudkq6DRT6HZjBeJ1 pBF2J655cdoenPBIeimjnnh4K7YZBzwOLJf2c6u76fe5Ag0EV9ZMvgEQAKc0Db17xNqtSwEv mfp4tkddwW9XA0tWWKtY4KUdd/jijYqc3fDD54ESYpV8QWj0xK4YM0dLxnDU2IYxjEshSB1T qAatVWz9WtBYvzalsyTqMKP3w34FciuL7orXP4AibPtrHuIXWQOBECcVZTTOdZYGAzaYzxiA ONzF9eTiwIqe9/oaOjTwTLnOarHt16QApTYQSnxDUQljeNvKYt1lZE/gAUUxNLWsYyTT+22/ vU0GDUahsJxs1+f1yEr+OGrFiEAmqrzpF0lCS3f/3HVTU6rS9cK3glVUeaTF4+1SK5ZNO35p iVQCwphmxa+dwTG/DvvHYCtgOZorTJ+OHfvCnSVjsM4kcXGjJPy3JZmUtyL9UxEbYlrffGPQ I3gLXIGD5AN5XdAXFCjjaID/KR1c9RHd7Oaw0Pdcq9UtMLgM1vdX8RlDuMGPrj5sQrRVbgYH fVU/TQCk1C9KhzOwg4Ap2T3tE1umY/DqrXQgsgH71PXFucVjOyHMYXXugLT8YQ0gcBPHy9mZ qw5mgOI5lCl6d4uCcUT0l/OEtPG/rA1lxz8ctdFBVOQOxCvwRG2QCgcJ/UTn5vlivul+cThi 6ERPvjqjblLncQtRg8izj2qgmwQkvfj+h7Ex88bI8iWtu5+I3K3LmNz/UxHBSWEmUnkg4fJl Rr7oItHsZ0ia6wWQ8lQnABEBAAGJAjwEGAEKACYCGwwWIQTSNlOHQi7ChOOZgcf5M+tD3xNh HwUCXOcvZgUJBvIWKAAKCRD5M+tD3xNhHxhBD/9toXMIaPIVFd9w1nKsRDM1GE6gZe4jie8q MJpeHB9O+936fSXA0W2X0het60wJQQ45O8TpTcxpc9nGzcE4MTaLAI3E8TjIXAO0cPqUNLyp g0DXezmTw5BU+SKZ51+jSKOtFmzJCHOJZQaMeCHD+G3CrdUHQVQBb5AeuH3KFv9ltgDcWsc8 YO70o3+tGHwcEnyXLdrI0q05wV7ncnLdkgVo+VUN4092bNMPwYly1TZWcU3Jw5gczOUEfTY7 sgo6E/sGX3B+FzgIs5t4yi1XOweCAQ/mPnb6uFeNENEFyGKyMG1HtjwBqnftbiFO3qitEIUY xWGQH23oKscv7i9lT0gg2D+ktzZhVWwHJVY/2vWSB9aCSWChcH2BT+lWrkwSpoPhy+almM84 Qz2wF72/d4ce4L27pSrS+vOXtXHLGOOGcAn8yr9TV0kM4aR+NbGBRXGKhG6w4lY54uNd9IBa ARIPUhij5JSygxZCBaJKo+X64AHGkk5bXq+f0anwAMNuJXbYC/lz4DEdKmPgQGShOWNs1Y1a N3cI87Hun/RBVwQ0a3Tr1g6OWJ6xK8cYbMcoR8NZ7L9ALMeJeuUDQR39+fEeHg/6sQN0P0mv 0sL+//BAJphCzDk8ztbrFw+JaPtgzZpRSM6JhxnY+YMAsatJRXA0WSpYP5zzl7yu/GZJIgsv VQ== Message-ID: <2938bfb6-7215-4098-74b4-d11fcc12dff4@intel.com> Date: Wed, 5 Jun 2019 16:17:16 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.7.0 MIME-Version: 1.0 In-Reply-To: <20190527170255.30853-1-allain.legacy@windriver.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v2] net/avp: remove resources when port is closed 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 5/27/2019 6:02 PM, Allain Legacy wrote: > The rte_eth_dev_close() function now handles freeing resources for > devices (e.g., mac_addrs). To conform with the new close() behaviour we > are asserting the RTE_ETH_DEV_CLOSE_REMOVE flag so that > rte_eth_dev_close() releases all device level dynamic memory. > > Second level memory allocated to each individual rx/tx queue is now > freed as part of the close() operation therefore making it safe for the > rte_eth_dev_close() function to free the device private data without > orphaning the rx/tx queue pointers. > > Cc: Matt Peters > Signed-off-by: Allain Legacy > --- > drivers/net/avp/avp_ethdev.c | 44 ++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 42 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/avp/avp_ethdev.c b/drivers/net/avp/avp_ethdev.c > index 09388d05f..0f7481c86 100644 > --- a/drivers/net/avp/avp_ethdev.c > +++ b/drivers/net/avp/avp_ethdev.c > @@ -959,6 +959,8 @@ eth_avp_dev_init(struct rte_eth_dev *eth_dev) > eth_dev->dev_ops = &avp_eth_dev_ops; > eth_dev->rx_pkt_burst = &avp_recv_pkts; > eth_dev->tx_pkt_burst = &avp_xmit_pkts; > + /* Let rte_eth_dev_close() release the port resources */ > + eth_dev->data->dev_flags |= RTE_ETH_DEV_CLOSE_REMOVE; > > if (rte_eal_process_type() != RTE_PROC_PRIMARY) { > /* > @@ -1940,8 +1942,25 @@ avp_dev_rx_queue_release(void *rx_queue) > unsigned int i; > > for (i = 0; i < avp->num_rx_queues; i++) { > - if (data->rx_queues[i] == rxq) > + if (data->rx_queues[i] == rxq) { > + rte_free(data->rx_queues[i]); > data->rx_queues[i] = NULL; > + } > + } > +} > + > +static void > +avp_dev_rx_queue_release_all(struct rte_eth_dev *eth_dev) > +{ > + struct avp_dev *avp = AVP_DEV_PRIVATE_TO_HW(eth_dev->data->dev_private); > + struct rte_eth_dev_data *data = avp->dev_data; > + unsigned int i; > + > + for (i = 0; i < avp->num_rx_queues; i++) { > + if (data->rx_queues[i]) { > + rte_free(data->rx_queues[i]); > + data->rx_queues[i] = NULL; > + } > } > } > > @@ -1954,8 +1973,25 @@ avp_dev_tx_queue_release(void *tx_queue) > unsigned int i; > > for (i = 0; i < avp->num_tx_queues; i++) { > - if (data->tx_queues[i] == txq) > + if (data->tx_queues[i] == txq) { > + rte_free(data->tx_queues[i]); > data->tx_queues[i] = NULL; > + } > + } > +} > + > +static void > +avp_dev_tx_queue_release_all(struct rte_eth_dev *eth_dev) > +{ > + struct avp_dev *avp = AVP_DEV_PRIVATE_TO_HW(eth_dev->data->dev_private); > + struct rte_eth_dev_data *data = avp->dev_data; > + unsigned int i; > + > + for (i = 0; i < avp->num_tx_queues; i++) { > + if (data->tx_queues[i]) { > + rte_free(data->tx_queues[i]); > + data->tx_queues[i] = NULL; > + } > } > } > > @@ -2104,6 +2140,10 @@ avp_dev_close(struct rte_eth_dev *eth_dev) > /* continue */ > } > > + /* release dynamic storage for rx/tx queues */ > + avp_dev_rx_queue_release_all(eth_dev); > + avp_dev_tx_queue_release_all(eth_dev); > + > unlock: > rte_spinlock_unlock(&avp->lock); > } > Patch looks correct as it is and cover the resource freeing on 'rte_eth_dev_close()' path, but not complete in remove path. The remove path stack trace is like following: rte_avp_pmd->.remove [eth_avp_pci_remove] rte_eth_dev_pci_generic_remove() eth_avp_dev_uninit() rte_eth_dev_pci_release() rte_eth_dev_release_port() rte_eth_dev_release_port() will free the ethdev allocated resources but not PMD private ones (like the queues freed above), it looks like just adding a 'avp_dev_close()' call into the 'eth_avp_dev_uninit()' can solve this, can you please check this option? And if it make sense, can you please send a new version?