From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id AC933A04BC; Tue, 29 Sep 2020 18:36:15 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 2CC2B1BC8B; Tue, 29 Sep 2020 18:36:13 +0200 (CEST) Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by dpdk.org (Postfix) with ESMTP id 23F321BC27 for ; Tue, 29 Sep 2020 18:36:09 +0200 (CEST) IronPort-SDR: StS0/KZS6BuGNYSIx0mIK6iX/JoLqtsU7I3dFaeQQfntZ50Qb8DcYlMXchMphA1QWZFA7yTzAR PhUmJtcbiI2A== X-IronPort-AV: E=McAfee;i="6000,8403,9759"; a="141633307" X-IronPort-AV: E=Sophos;i="5.77,319,1596524400"; d="scan'208";a="141633307" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 29 Sep 2020 09:36:07 -0700 IronPort-SDR: Pgp3Sw3CpVWVYwa2yAMJCDMQo69zujIu+T+YoZF4GqtdUZ5hWLM3qZXmd6zV8GRLmvih8WlWla 1b9+ZW/9NGxQ== X-IronPort-AV: E=Sophos;i="5.77,319,1596524400"; d="scan'208";a="492096923" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.213.220.198]) ([10.213.220.198]) by orsmga005-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 29 Sep 2020 09:35:58 -0700 To: Thomas Monjalon , "Wang, Haiyue" , Maxime Coquelin Cc: "dev@dpdk.org" , "arybchenko@solarflare.com" , Shepard Siegel , Ed Czeck , John Miller , Igor Russkikh , Pavel Belous , Somalapuram Amaranath , Ajit Khaparde , Somnath Kotur , Chas Williams , "Wei Hu (Xavier)" , Hemant Agrawal , Sachin Saxena , "Guo, Jia" , Marcin Wojtas , Michal Krawczyk , Guy Tzalik , Evgeny Schemeilin , Igor Chauskin , "Zhang, Qi Z" , "Wang, Xiao W" , Ziyang Xuan , Xiaoyun Wang , Guoyang Zhou , "Min Hu (Connor)" , Yisen Zhuang , "Xing, Beilei" , "Wu, Jingjing" , "Yang, Qiming" , Alfredo Cardigliano , Shijith Thotton , Srisivasubramanian Srinivasan , Stephen Hemminger , "K. Y. Srinivasan" , Haiyang Zhang , Long Li , Harman Kalra , Rasesh Mody , Shahed Shaikh , "Wiles, Keith" , "Xia, Chenbo" , "Wang, Zhihong" , Yong Wang , "matan@nvidia.com" References: <20200913220711.3768597-1-thomas@monjalon.net> <4c4ae719-f870-c219-babc-c34ff67b25c7@intel.com> <26589070.GSGI34Fg33@thomas> From: Ferruh Yigit Message-ID: <2016f2e6-6ce5-3a7b-47b9-c3b8bb4be8be@intel.com> Date: Tue, 29 Sep 2020 17:35:54 +0100 MIME-Version: 1.0 In-Reply-To: <26589070.GSGI34Fg33@thomas> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v3 28/29] ethdev: reset all when releasing a 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 9/29/2020 5:02 PM, Thomas Monjalon wrote: > 29/09/2020 17:50, Ferruh Yigit: >> On 9/29/2020 12:58 PM, Wang, Haiyue wrote: >>> From: Thomas Monjalon >>>> 29/09/2020 12:26, Maxime Coquelin: >>>>> On 9/29/20 1:14 AM, Thomas Monjalon wrote: >>>>>> The function rte_eth_dev_release_port() was resetting partially >>>>>> the struct rte_eth_dev. The drivers were completing it >>>>>> with more pointers set to NULL in the close or remove operations. >>>>>> >>>>>> A full memset is done so most of those assignments become useless. >>>> [...] >>>>> With this patch, I get following segfault at init time with Virtio PMD: >>>>> >>>>> Program received signal SIGSEGV, Segmentation fault. >>>>> 0x0000000000854c9b in rte_eth_dev_callback_register (port_id=32, >>>>> event=RTE_ETH_EVENT_UNKNOWN, cb_fn=0x4b24de >>>>> , >>>>> cb_arg=0x0) at ../lib/librte_ethdev/rte_ethdev.c:4042 >>>>> 4042 >>>>> TAILQ_INSERT_TAIL(&(dev->link_intr_cbs), >>>> >>>> Yes this is because after closing a port, everything is resetted, >>>> including .link_intr_cbs which is set only once in a constructor: >>>> http://git.dpdk.org/dpdk/commit/?id=9ec0b3869d8 >>>> >>>> I can change this patch to selectively set pointers to NULL. >>>> >>>> Or if we prefer a big memset 0, we need to rework how RTE_ETH_ALL >>>> is managed to register a callback for any event. >>>> Instead of setting the callback for all ports, we could have >>>> a special catch-call callback list which is called for all events. >>>> This way we could revert initializing .link_intr_cbs in eth_dev_get(). >>>> >>>> >>> >>> Move 'struct rte_eth_dev_cb_list link_intr_cbs' to the end of 'struct rte_eth_dev', >>> and starting from link_intr_cbs, these members will be kept after closed ? :-) >>> >>> memset(eth_dev, 0, offsetof(struct rte_eth_dev, link_intr_cbs)); >>> >> >> This is similar version of a previous patch: >> https://patches.dpdk.org/patch/65795/ > > I forgot this patch :) > >> That one is also waiting because I remember it was not safe for hotplug. > > I don't think there is a safety issue. > It makes harder to play with dangling pointers, which is good. > Note: the .device pointer was already resetted by > rte_eth_dev_pci_generic_remove(), and generalized in patch 1. > As far as I remember, resetting '.device' in the 'close()' was causing some issues on hot remove. Yes patch one already does it, I will test the hot remove after close with it. >> Instead of a big memset, I am for selectively set pointers to NULL. > > I will prepare the patch with selective resets. > >