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 F0490A0577; Mon, 13 Apr 2020 17:20:16 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 5C08E1C037; Mon, 13 Apr 2020 17:20:13 +0200 (CEST) Received: from mail-pj1-f66.google.com (mail-pj1-f66.google.com [209.85.216.66]) by dpdk.org (Postfix) with ESMTP id 4F9F61BE9D for ; Mon, 13 Apr 2020 17:20:12 +0200 (CEST) Received: by mail-pj1-f66.google.com with SMTP id mn19so3915898pjb.0 for ; Mon, 13 Apr 2020 08:20:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=f2gnX54kZLzFZHdQw+9h+BM1SFfWAdNrK5iMXJKifBU=; b=fn3jsA6TS/uX8xiWflvfsxE59lwkbztbH+5qwllg15wE7CQ5SX6dDSYI4hTYpCzkT4 2PBGQ9Ey+SY18m4QLRU1xiDLMiOqM2tOlflXKEchiwrGUtmKDgaaSStOUo7cVnPJFsKE B95K6L9tMPzXJo2wJ1fdNVuVTuFnd7ozEELGsMI4FjgyQk19g5poLVXqfxm21liWAPE4 x1uD2pm1G8kUVHZllOCe5hjrbMEA9SbettYFE+gUap0C1emjW5M4Lga2VjgNiA2gnQha OF/mTzWqM3aN/QT8XPuLqv0ZSmAQh83hHpe4S8u8GffOFDTGhyP/6/HQ9Ia9Rht1sTN0 sMBQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=f2gnX54kZLzFZHdQw+9h+BM1SFfWAdNrK5iMXJKifBU=; b=RiZfIRZbBHswpKKGxYXH/xkDYrkYPe5ESf9msHxjtOdNgVByOr0UPp/UmSQ2h23dcI bScvKAQarPGHEaUT0Y5nTulSPGinxUvjZ9etgrQ3goRF53+rDAb0sB7z6eslcrO6ZhZ/ 8opPBKbgU3PO7L0mua0mnMKUEjJCmDbpnhpa9INaEg+0ANYliUmKUXIHTiibPHdiCwdt KJKjoHk3SfwHfxBWiB/SvoOd7/7VYheoYeoWiUVpxqCzOVAnNOc5Hixwyq+j90R+t5KY DuwyyGLmtxy9mLEKPs1BopwFON9vKFxQqtsO8SLwf+0dpUrbb9buotRwd8vsYh2Y6JjN 3vWQ== X-Gm-Message-State: AGi0PuZM/jRm0nGXQWki8YQTdLteGn52Tt5T9PhtI83ZeIuG0av/Tuc0 F5eISzYWvTQDAStFgJimz1w= X-Google-Smtp-Source: APiQypIcubnlczZiqM72mbJtnqSuw5SAtrUeuxzL3bu6OodDSH9yDbGZ1YFHAI6VCzsmvwpAQw3Eaw== X-Received: by 2002:a17:902:aa97:: with SMTP id d23mr17673875plr.244.1586791211511; Mon, 13 Apr 2020 08:20:11 -0700 (PDT) Received: from local.opencloud.tech.localdomain ([115.171.63.184]) by smtp.gmail.com with ESMTPSA id d23sm8887600pfq.210.2020.04.13.08.20.06 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Mon, 13 Apr 2020 08:20:10 -0700 (PDT) From: xiangxia.m.yue@gmail.com To: olivier.matz@6wind.com, arybchenko@solarflare.com, gage.eads@intel.com, artem.andreev@oktetlabs.ru, jerinj@marvell.com, ndabilpuram@marvell.com, vattunuru@marvell.com, hemant.agrawal@nxp.com, david.marchand@redhat.com, anatoly.burakov@intel.com, bruce.richardson@intel.com Cc: dev@dpdk.org, Tonghao Zhang Date: Mon, 13 Apr 2020 22:21:54 +0800 Message-Id: <1586787714-13890-2-git-send-email-xiangxia.m.yue@gmail.com> X-Mailer: git-send-email 1.8.3.1 In-Reply-To: <1586787714-13890-1-git-send-email-xiangxia.m.yue@gmail.com> References: <1583114253-15345-1-git-send-email-xiangxia.m.yue@gmail.com> <1586787714-13890-1-git-send-email-xiangxia.m.yue@gmail.com> Subject: [dpdk-dev] [PATCH dpdk-dev v3 2/2] mempool: use shared memzone for rte_mempool_ops 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" 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. but the number of mempool libraries may be different. * shared memzone: The primary process allocates a struct in shared memory named memzone, 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. Previous discussion: https://mails.dpdk.org/archives/dev/2020-March/159354.html Suggested-by: Olivier Matz Suggested-by: Jerin Jacob Signed-off-by: Tonghao Zhang --- v2: * fix checkpatch warning --- lib/librte_mempool/rte_mempool.h | 28 +++++++++++- lib/librte_mempool/rte_mempool_ops.c | 89 ++++++++++++++++++++++++++++-------- 2 files changed, 96 insertions(+), 21 deletions(-) diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h index c90cf31467b2..2709b9e1d51b 100644 --- a/lib/librte_mempool/rte_mempool.h +++ b/lib/librte_mempool/rte_mempool.h @@ -50,6 +50,7 @@ #include #include #include +#include #ifdef __cplusplus extern "C" { @@ -678,7 +679,6 @@ struct rte_mempool_ops { */ struct rte_mempool_ops_table { rte_spinlock_t sl; /**< Spinlock for add/delete. */ - uint32_t num_ops; /**< Number of used ops structs in the table. */ /** * Storage for all possible ops structs. */ @@ -910,6 +910,30 @@ int rte_mempool_ops_get_info(const struct rte_mempool *mp, */ int rte_mempool_register_ops(const struct rte_mempool_ops *ops); +struct rte_mempool_shared_ops { + size_t num_mempool_ops; + struct { + char name[RTE_MEMPOOL_OPS_NAMESIZE]; + } mempool_ops[RTE_MEMPOOL_MAX_OPS_IDX]; + + rte_spinlock_t mempool; +}; + +static inline int +mempool_ops_register_cb(const void *arg) +{ + const struct rte_mempool_ops *h = (const struct rte_mempool_ops *)arg; + + return rte_mempool_register_ops(h); +} + +static inline void +mempool_ops_register(const struct rte_mempool_ops *ops) +{ + rte_init_register(mempool_ops_register_cb, (const void *)ops, + RTE_INIT_PRE); +} + /** * Macro to statically register the ops of a mempool handler. * Note that the rte_mempool_register_ops fails silently here when @@ -918,7 +942,7 @@ int rte_mempool_ops_get_info(const struct rte_mempool *mp, #define MEMPOOL_REGISTER_OPS(ops) \ RTE_INIT(mp_hdlr_init_##ops) \ { \ - rte_mempool_register_ops(&ops); \ + mempool_ops_register(&ops); \ } /** diff --git a/lib/librte_mempool/rte_mempool_ops.c b/lib/librte_mempool/rte_mempool_ops.c index 22c5251eb068..b10fda662db6 100644 --- a/lib/librte_mempool/rte_mempool_ops.c +++ b/lib/librte_mempool/rte_mempool_ops.c @@ -14,43 +14,95 @@ /* indirect jump table to support external memory pools. */ struct rte_mempool_ops_table rte_mempool_ops_table = { .sl = RTE_SPINLOCK_INITIALIZER, - .num_ops = 0 }; -/* add a new ops struct in rte_mempool_ops_table, return its index. */ -int -rte_mempool_register_ops(const struct rte_mempool_ops *h) +static int +rte_mempool_register_shared_ops(const char *name) { - struct rte_mempool_ops *ops; - int16_t ops_index; + static bool mempool_shared_ops_inited; + struct rte_mempool_shared_ops *shared_ops; + const struct rte_memzone *mz; + uint32_t ops_index = 0; + + if (rte_eal_process_type() == RTE_PROC_PRIMARY && + !mempool_shared_ops_inited) { + + mz = rte_memzone_reserve("mempool_ops_shared", + sizeof(*shared_ops), 0, 0); + if (mz == NULL) + return -ENOMEM; + + shared_ops = mz->addr; + shared_ops->num_mempool_ops = 0; + rte_spinlock_init(&shared_ops->mempool); + + mempool_shared_ops_inited = true; + } else { + mz = rte_memzone_lookup("mempool_ops_shared"); + if (mz == NULL) + return -ENOMEM; + + shared_ops = mz->addr; + } - rte_spinlock_lock(&rte_mempool_ops_table.sl); + rte_spinlock_lock(&shared_ops->mempool); - if (rte_mempool_ops_table.num_ops >= - RTE_MEMPOOL_MAX_OPS_IDX) { - rte_spinlock_unlock(&rte_mempool_ops_table.sl); + if (shared_ops->num_mempool_ops >= RTE_MEMPOOL_MAX_OPS_IDX) { + rte_spinlock_unlock(&shared_ops->mempool); RTE_LOG(ERR, MEMPOOL, "Maximum number of mempool ops structs exceeded\n"); return -ENOSPC; } + while (shared_ops->mempool_ops[ops_index].name[0]) { + if (!strcmp(name, shared_ops->mempool_ops[ops_index].name)) { + rte_spinlock_unlock(&shared_ops->mempool); + return ops_index; + } + + ops_index++; + } + + strlcpy(shared_ops->mempool_ops[ops_index].name, name, + sizeof(shared_ops->mempool_ops[0].name)); + + shared_ops->num_mempool_ops++; + + rte_spinlock_unlock(&shared_ops->mempool); + return ops_index; +} + +/* add a new ops struct in rte_mempool_ops_table, return its index. */ +int +rte_mempool_register_ops(const struct rte_mempool_ops *h) +{ + struct rte_mempool_ops *ops; + int16_t ops_index; + if (h->alloc == NULL || h->enqueue == NULL || - h->dequeue == NULL || h->get_count == NULL) { - rte_spinlock_unlock(&rte_mempool_ops_table.sl); + h->dequeue == NULL || h->get_count == NULL) { RTE_LOG(ERR, MEMPOOL, "Missing callback while registering mempool ops\n"); + rte_errno = EINVAL; return -EINVAL; } if (strlen(h->name) >= sizeof(ops->name) - 1) { - rte_spinlock_unlock(&rte_mempool_ops_table.sl); - RTE_LOG(DEBUG, EAL, "%s(): mempool_ops <%s>: name too long\n", - __func__, h->name); + RTE_LOG(ERR, MEMPOOL, + "The registering mempool_ops <%s>: name too long\n", + h->name); rte_errno = EEXIST; return -EEXIST; } - ops_index = rte_mempool_ops_table.num_ops++; + ops_index = rte_mempool_register_shared_ops(h->name); + if (ops_index < 0) { + rte_errno = -ops_index; + return ops_index; + } + + rte_spinlock_lock(&rte_mempool_ops_table.sl); + ops = &rte_mempool_ops_table.ops[ops_index]; strlcpy(ops->name, h->name, sizeof(ops->name)); ops->alloc = h->alloc; @@ -165,9 +217,8 @@ struct rte_mempool_ops_table rte_mempool_ops_table = { if (mp->flags & MEMPOOL_F_POOL_CREATED) return -EEXIST; - for (i = 0; i < rte_mempool_ops_table.num_ops; i++) { - if (!strcmp(name, - rte_mempool_ops_table.ops[i].name)) { + for (i = 0; i < RTE_MEMPOOL_MAX_OPS_IDX; i++) { + if (!strcmp(name, rte_mempool_ops_table.ops[i].name)) { ops = &rte_mempool_ops_table.ops[i]; break; } -- 1.8.3.1