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 A48CBA052E; Mon, 9 Mar 2020 09:27:10 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 10C8D1BFFC; Mon, 9 Mar 2020 09:27:10 +0100 (CET) Received: from mail-wm1-f67.google.com (mail-wm1-f67.google.com [209.85.128.67]) by dpdk.org (Postfix) with ESMTP id D1A5E1BFFB for ; Mon, 9 Mar 2020 09:27:07 +0100 (CET) Received: by mail-wm1-f67.google.com with SMTP id a141so8583593wme.2 for ; Mon, 09 Mar 2020 01:27:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=gmBG2FMmZ5VAfFK3pPNjOIPi9yhO4GvGqAsdPYKp2R8=; b=HJpYboeCapYr32P+d2fizMGOnNbYRnhMf5uH3NJk2IENBmYRZ0Q7A6bJFbZnaqgeuj SZrkyN4nQkCGseJkMrUNhRUPQ5A8jQTtdIrCfbFOVlYtCGvBvLowLKObGrf6hctLnId3 gGMrSudui60WZcLVjP9tVWmEcXrL70G8YG21AdJNKJCfdVzrmgF05XEJRJD+Fe/QRJbc QqeY+6wCvCZ5dAIMBRLCccXQ6YkqENWM67Dvq2iGY5FN/RC7sn1ZsnptM3ODku25pMqK TFm6cCF1ilfgeeHAM+dT7oNmslv8rLYHWUePvG1CWgfCB0cGNt5MGxUfZEzZL05LdAhE 7+bw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=gmBG2FMmZ5VAfFK3pPNjOIPi9yhO4GvGqAsdPYKp2R8=; b=fhzaNcu1BO1NpMw7TBQ+Aznun/Q/8bGnvyj3Gl84XSxaqgFslOqIK04qzAFDtu/ckl cWdEhdftle+6UIRStYZOpVYd6c7BM2dhOK5tOxC8Nwmyi0ErVzPKv7Tb74lVDvv+ibg2 oQ6pUTA8JJGVVpCUEVOmZLvkxdOuhlvopU3tuI2ExDtgvvUvIdGjhLZaXusYnASWGmCZ 7gagYE47YPYZJaTx1QPkpjZGd2ZDY2t3E0iErPPsYH2RzQWOFMvHIdOO5RdapRnoEEah 4uPvUKcuEa49zK7H+pvTWksxDcQeofZlfM979oNiCeAJgqXw1ph//pOxJ88qdZfc5Mmo tC6w== X-Gm-Message-State: ANhLgQ3EsHCBzAP9RR5M5ridR5Ui5r/b8MOyJ7eKSAo+966WvKNb3ui7 ziO8ubL2EGzlBCmmzweGzvpQXw== X-Google-Smtp-Source: ADFU+vtMzyW2nAWXPyYaUsH3UifpDMRkIXlR6haf69o0gYL8puNijv9w8sA9Chc81ArEOrLfOSz6Vw== X-Received: by 2002:a1c:dc55:: with SMTP id t82mr7975033wmg.6.1583742427153; Mon, 09 Mar 2020 01:27:07 -0700 (PDT) Received: from 6wind.com (2a01cb0c0005a600345636f7e65ed1a0.ipv6.abo.wanadoo.fr. [2a01:cb0c:5:a600:3456:36f7:e65e:d1a0]) by smtp.gmail.com with ESMTPSA id r19sm23776076wmh.26.2020.03.09.01.27.06 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 09 Mar 2020 01:27:06 -0700 (PDT) Date: Mon, 9 Mar 2020 09:27:05 +0100 From: Olivier Matz To: Tonghao Zhang Cc: Andrew Rybchenko , Jerin Jacob , dpdk-dev , Gage Eads , "Artem V. Andreev" , Jerin Jacob , Nithin Dabilpuram , Vamsi Attunuru , Hemant Agrawal Message-ID: <20200309082705.GM13822@platinum> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) 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" Hi, On Mon, Mar 09, 2020 at 11:01:25AM +0800, Tonghao Zhang wrote: > On Sat, Mar 7, 2020 at 8:54 PM Andrew Rybchenko > wrote: > > > > 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. Good points. The fact that the ops index changes during mempool driver lifetime is indeed frightening, especially knowning that this is a dynamic registration that could happen at any moment in the life of the application. Also, breaking the ABI is not desirable. Let me try to propose something else to solve your issue: 1/ At init, the primary process allocates a struct in shared memory (named memzone): struct rte_mempool_shared_ops { size_t num_mempool_ops; struct { char name[RTE_MEMPOOL_OPS_NAMESIZE]; } mempool_ops[RTE_MEMPOOL_MAX_OPS_IDX]; char *mempool_ops_name[RTE_MEMPOOL_MAX_OPS_IDX]; rte_spinlock_t mempool; } 2/ When we register a mempool ops, we first get a name and id from the shared struct: with the lock held, lookup for the registered name and return its index, else get the last id and copy the name in the struct. 3/ Then do as before (in the per-process global table), except that we reuse the registered id. We can remove the num_ops field from rte_mempool_ops_table. Thoughts? > Yes, should update the doc: how about this: > > diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h > index c90cf31..5a9c8a7 100644 > --- a/lib/librte_mempool/rte_mempool.h > +++ b/lib/librte_mempool/rte_mempool.h > @@ -904,7 +904,9 @@ int rte_mempool_ops_get_info(const struct rte_mempool *mp, > * @param ops > * Pointer to an ops structure to register. > * @return > - * - >=0: Success; return the index of the ops struct in the table. > + * - >=0: Success; return the index of the last ops struct in the table. > + * The number of the ops struct registered is equal to index > + * returned + 1. > * - -EINVAL - some missing callbacks while registering ops struct. > * - -ENOSPC - the maximum number of ops structs has been reached. > */ > diff --git a/lib/librte_mempool/rte_mempool_ops.c > b/lib/librte_mempool/rte_mempool_ops.c > index b0da096..053f340 100644 > --- a/lib/librte_mempool/rte_mempool_ops.c > +++ b/lib/librte_mempool/rte_mempool_ops.c > @@ -26,7 +26,11 @@ struct rte_mempool_ops_table rte_mempool_ops_table = { > return strcmp(m_a->name, m_b->name); > } > > -/* add a new ops struct in rte_mempool_ops_table, return its index. */ > +/* > + * add a new ops struct in rte_mempool_ops_table. > + * on success, return the index of the last ops > + * struct in the table. > + */ > int > rte_mempool_register_ops(const struct rte_mempool_ops *h) > { > > > 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. > OK, a separate patch should be sent before DPDK 20.05 release ? > > > > > -- > Thanks, > Tonghao