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 4448FA04B5; Wed, 30 Sep 2020 14:17:34 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 1DA4A1D659; Wed, 30 Sep 2020 14:17:33 +0200 (CEST) Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id 3267D1D537 for ; Wed, 30 Sep 2020 14:17:31 +0200 (CEST) IronPort-SDR: 1l9H5QVIi/4ZybMmycpIToqu3t2V9xnKxXEv8MGjPmXEvHM6XMFk2sznhdu2wSIEMRB7p7E8Eo 7wideDyFrV2A== X-IronPort-AV: E=McAfee;i="6000,8403,9759"; a="162489860" X-IronPort-AV: E=Sophos;i="5.77,322,1596524400"; d="scan'208";a="162489860" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 30 Sep 2020 05:17:17 -0700 IronPort-SDR: bfOxdVYRekJM0+rCuCY0RiDVv3PLT/opE6Qp2rA5TW6olYEh1jUWIxim1hJeZ1x9fQHAhoSofV hp412a6PJ+7A== X-IronPort-AV: E=Sophos;i="5.77,322,1596524400"; d="scan'208";a="497283353" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.213.251.186]) ([10.213.251.186]) by orsmga005-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 30 Sep 2020 05:17:08 -0700 From: Ferruh Yigit 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> <2016f2e6-6ce5-3a7b-47b9-c3b8bb4be8be@intel.com> Message-ID: <05c84510-a145-8e47-93a3-9b4ae212bc22@intel.com> Date: Wed, 30 Sep 2020 13:17:07 +0100 MIME-Version: 1.0 In-Reply-To: <2016f2e6-6ce5-3a7b-47b9-c3b8bb4be8be@intel.com> 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:35 PM, Ferruh Yigit wrote: > 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. > I didn't get any error with 1/29, will proceed with the patchset. >>> Instead of a big memset, I am for selectively set pointers to NULL. >> >> I will prepare the patch with selective resets. >> >> >