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 ED663A0564; Sat, 7 Mar 2020 13:54:38 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id CE9681BFBA; Sat, 7 Mar 2020 13:54:37 +0100 (CET) Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [67.231.154.164]) by dpdk.org (Postfix) with ESMTP id A1AFD23D for ; Sat, 7 Mar 2020 13:54:36 +0100 (CET) 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-us4.ppe-hosted.com (PPE Hosted ESMTP Server) with ESMTPS id 8E2BA280057; Sat, 7 Mar 2020 12:54:35 +0000 (UTC) Received: from [127.0.0.27] (10.17.10.39) by ukex01.SolarFlarecom.com (10.17.10.4) with Microsoft SMTP Server (TLS) id 15.0.1395.4; Sat, 7 Mar 2020 12:54:26 +0000 From: Andrew Rybchenko To: Jerin Jacob , Tonghao Zhang CC: dpdk-dev , Olivier Matz , Gage Eads , "Artem V. Andreev" , "Jerin Jacob" , Nithin Dabilpuram , Vamsi Attunuru , Hemant Agrawal References: <1583114253-15345-1-git-send-email-xiangxia.m.yue@gmail.com> <1583501776-9958-1-git-send-email-xiangxia.m.yue@gmail.com> <7420c590-4906-34e2-b0b8-d412df9005c8@solarflare.com> Message-ID: Date: Sat, 7 Mar 2020 15:54:18 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0 MIME-Version: 1.0 In-Reply-To: <7420c590-4906-34e2-b0b8-d412df9005c8@solarflare.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Originating-IP: [10.17.10.39] 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.1020-25274.003 X-TM-AS-Result: No-14.758800-8.000000-10 X-TMASE-MatchedRID: byfwvk+IcRnmLzc6AOD8DfHkpkyUphL9q/CNWo5Wx+O2ZO4LLn5VSuFo /YmgpQJJLKxcrLNnnsbsmswpTDj0yh1YpEPWJiyz2oIkXC6Ol0Bj8vb8BkkBZMcySOqW2ey321p BzAE1M7ry2UZqCS3UtWhKmI6nO4J3O5cso4YfEOxH+PTjR9EWkm5BhzAcWjgGy5JfHvVu9IuMbv oO9mofAassgi/t6bTQqUFzXY3N6oG6687/uEqqCVkxnoxnQfVSOHhqIXe4IzYK1SsgKmaayYgnL dHU7oiOrsRbop3C67AO6bgzXko5LODocHyqS9HwE367MfaNw8PM8zLNncnslQdC9P1Le8dQtApi 5PiRGDpaFd4aGs5cxMUoojnT14mJG4m8MaUGgEbD0ZWEZr/ntn0tCKdnhB581B0Hk1Q1KyLUZxE AlFPo8/cUt5lc1lLgOMB0shqXhHo4U3N/cHJRwf2sx6SJMUPy0uN4NE3vAW7P866RTkN3Zhj8Vo Z2JdDh X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-TMASE-Result: 10--14.758800-8.000000 X-TMASE-Version: SMEX-12.5.0.1300-8.5.1020-25274.003 X-MDID: 1583585676-FQK6Yy_pnhbz Subject: Re: [dpdk-dev] [PATCH dpdk-dev v3] mempool: sort the rte_mempool_ops by name 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 3/7/20 3:51 PM, Andrew Rybchenko wrote: > On 3/6/20 4:37 PM, Jerin Jacob wrote: >> On Fri, Mar 6, 2020 at 7:06 PM wrote: >>> From: Tonghao Zhang >>> >>> The order of mempool initiation affects mempool index in the >>> rte_mempool_ops_table. For example, when building APPs with: >>> >>> $ gcc -lrte_mempool_bucket -lrte_mempool_ring ... >>> >>> The "bucket" mempool will be registered firstly, and its index >>> in table is 0 while the index of "ring" mempool is 1. DPDK >>> uses the mk/rte.app.mk to build APPs, and others, for example, >>> Open vSwitch, use the libdpdk.a or libdpdk.so to build it. >>> The mempool lib linked in dpdk and Open vSwitch is different. >>> >>> The mempool can be used between primary and secondary process, >>> such as dpdk-pdump and pdump-pmd/Open vSwitch(pdump enabled). >>> There will be a crash because dpdk-pdump creates the "ring_mp_mc" >>> ring which index in table is 0, but the index of "bucket" ring >>> is 0 in Open vSwitch. If Open vSwitch use the index 0 to get >>> mempool ops and malloc memory from mempool. The crash will occur: >>> >>>      bucket_dequeue (access null and crash) >>>      rte_mempool_get_ops (should get "ring_mp_mc", >>>                           but get "bucket" mempool) >>>      rte_mempool_ops_dequeue_bulk >>>      ... >>>      rte_pktmbuf_alloc >>>      rte_pktmbuf_copy >>>      pdump_copy >>>      pdump_rx >>>      rte_eth_rx_burst >>> >>> To avoid the crash, there are some solution: >>> * constructor priority: Different mempool uses different >>>    priority in RTE_INIT, but it's not easy to maintain. >>> >>> * change mk/rte.app.mk: Change the order in mk/rte.app.mk to >>>    be same as libdpdk.a/libdpdk.so, but when adding a new mempool >>>    driver in future, we must make sure the order. >>> >>> * register mempool orderly: Sort the mempool when registering, >>>    so the lib linked will not affect the index in mempool table. >>> >>> Signed-off-by: Tonghao Zhang >>> Acked-by: Olivier Matz >> Acked-by: Jerin Jacob > > The patch is OK, but the fact that ops index changes during > mempool driver lifetime is frightening. In fact it breaks > rte_mempool_register_ops() return value semantics (read > as API break). The return value is not used in DPDK, but it > is a public function. If I'm not mistaken it should be taken > into account. > > Also I remember patches which warn about above behaviour > in documentation. If behaviour changes, corresponding > documentation must be updated. One more point. If the patch is finally accepted it definitely deserves few lines in release notes.