Patch Detail
get:
Show a patch.
patch:
Update a patch.
put:
Update a patch.
GET /api/patches/50518/?format=api
{ "id": 50518, "url": "http://patches.dpdk.org/api/patches/50518/?format=api", "web_url": "http://patches.dpdk.org/project/dpdk/patch/d43eba58247bbd4a5c3eb1eb36cd728338daf364.1551201154.git.anatoly.burakov@intel.com/", "project": { "id": 1, "url": "http://patches.dpdk.org/api/projects/1/?format=api", "name": "DPDK", "link_name": "dpdk", "list_id": "dev.dpdk.org", "list_email": "dev@dpdk.org", "web_url": "http://core.dpdk.org", "scm_url": "git://dpdk.org/dpdk", "webscm_url": "http://git.dpdk.org/dpdk", "list_archive_url": "https://inbox.dpdk.org/dev", "list_archive_url_format": "https://inbox.dpdk.org/dev/{}", "commit_url_format": "" }, "msgid": "<d43eba58247bbd4a5c3eb1eb36cd728338daf364.1551201154.git.anatoly.burakov@intel.com>", "list_archive_url": "https://inbox.dpdk.org/dev/d43eba58247bbd4a5c3eb1eb36cd728338daf364.1551201154.git.anatoly.burakov@intel.com", "date": "2019-02-26T17:13:11", "name": "fbarray: add internal tailq for mapped areas", "commit_ref": null, "pull_url": null, "state": "accepted", "archived": true, "hash": "41be06a58f3c5b31fc0db740c9e96761e404213c", "submitter": { "id": 4, "url": "http://patches.dpdk.org/api/people/4/?format=api", "name": "Anatoly Burakov", "email": "anatoly.burakov@intel.com" }, "delegate": { "id": 1, "url": "http://patches.dpdk.org/api/users/1/?format=api", "username": "tmonjalo", "first_name": "Thomas", "last_name": "Monjalon", "email": "thomas@monjalon.net" }, "mbox": "http://patches.dpdk.org/project/dpdk/patch/d43eba58247bbd4a5c3eb1eb36cd728338daf364.1551201154.git.anatoly.burakov@intel.com/mbox/", "series": [ { "id": 3551, "url": "http://patches.dpdk.org/api/series/3551/?format=api", "web_url": "http://patches.dpdk.org/project/dpdk/list/?series=3551", "date": "2019-02-26T17:13:11", "name": "fbarray: add internal tailq for mapped areas", "version": 1, "mbox": "http://patches.dpdk.org/series/3551/mbox/" } ], "comments": "http://patches.dpdk.org/api/patches/50518/comments/", "check": "success", "checks": "http://patches.dpdk.org/api/patches/50518/checks/", "tags": {}, "related": [], "headers": { "Return-Path": "<dev-bounces@dpdk.org>", "X-Original-To": "patchwork@dpdk.org", "Delivered-To": "patchwork@dpdk.org", "Received": [ "from [92.243.14.124] (localhost [127.0.0.1])\n\tby dpdk.org (Postfix) with ESMTP id 645FA2C39;\n\tTue, 26 Feb 2019 18:13:16 +0100 (CET)", "from mga07.intel.com (mga07.intel.com [134.134.136.100])\n\tby dpdk.org (Postfix) with ESMTP id 0E3592C30\n\tfor <dev@dpdk.org>; Tue, 26 Feb 2019 18:13:13 +0100 (CET)", "from fmsmga007.fm.intel.com ([10.253.24.52])\n\tby orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;\n\t26 Feb 2019 09:13:12 -0800", "from irvmail001.ir.intel.com ([163.33.26.43])\n\tby fmsmga007.fm.intel.com with ESMTP; 26 Feb 2019 09:13:12 -0800", "from sivswdev05.ir.intel.com (sivswdev05.ir.intel.com\n\t[10.243.17.64])\n\tby irvmail001.ir.intel.com (8.14.3/8.13.6/MailSET/Hub) with ESMTP id\n\tx1QHDB6j032008 for <dev@dpdk.org>; Tue, 26 Feb 2019 17:13:11 GMT", "from sivswdev05.ir.intel.com (localhost [127.0.0.1])\n\tby sivswdev05.ir.intel.com with ESMTP id x1QHDBQ8005720\n\tfor <dev@dpdk.org>; Tue, 26 Feb 2019 17:13:11 GMT", "(from aburakov@localhost)\n\tby sivswdev05.ir.intel.com with LOCAL id x1QHDBcX005522\n\tfor dev@dpdk.org; Tue, 26 Feb 2019 17:13:11 GMT" ], "X-Amp-Result": "SKIPPED(no attachment in message)", "X-Amp-File-Uploaded": "False", "X-ExtLoop1": "1", "X-IronPort-AV": "E=Sophos;i=\"5.58,416,1544515200\"; d=\"scan'208\";a=\"125379795\"", "From": "Anatoly Burakov <anatoly.burakov@intel.com>", "To": "dev@dpdk.org", "Date": "Tue, 26 Feb 2019 17:13:11 +0000", "Message-Id": "<d43eba58247bbd4a5c3eb1eb36cd728338daf364.1551201154.git.anatoly.burakov@intel.com>", "X-Mailer": "git-send-email 1.7.0.7", "Subject": "[dpdk-dev] [PATCH] fbarray: add internal tailq for mapped areas", "X-BeenThere": "dev@dpdk.org", "X-Mailman-Version": "2.1.15", "Precedence": "list", "List-Id": "DPDK patches and discussions <dev.dpdk.org>", "List-Unsubscribe": "<https://mails.dpdk.org/options/dev>,\n\t<mailto:dev-request@dpdk.org?subject=unsubscribe>", "List-Archive": "<http://mails.dpdk.org/archives/dev/>", "List-Post": "<mailto:dev@dpdk.org>", "List-Help": "<mailto:dev-request@dpdk.org?subject=help>", "List-Subscribe": "<https://mails.dpdk.org/listinfo/dev>,\n\t<mailto:dev-request@dpdk.org?subject=subscribe>", "Errors-To": "dev-bounces@dpdk.org", "Sender": "\"dev\" <dev-bounces@dpdk.org>" }, "content": "Currently, there are numerous reliability issues with fbarray,\nsuch as:\n- There is no way to prevent attaching to overlapping memory\n areas\n- There is no way to prevent double-detach\n- Failed destroy leaves fbarray in an invalid state (fbarray\n itself is valid, but its backing memory area is already\n detached)\n\nIn addition, on FreeBSD, doing mmap() on a file descriptor\ndoes not keep the lock, so we also need to store the fd\nin order to keep the lock.\n\nThis patch improves upon fbarray to address both of these\nissues by adding an internal tailq to track allocated areas\nand their respective file descriptors.\n\nSigned-off-by: Anatoly Burakov <anatoly.burakov@intel.com>\n---\n lib/librte_eal/common/eal_common_fbarray.c | 212 +++++++++++++++++----\n 1 file changed, 179 insertions(+), 33 deletions(-)", "diff": "diff --git a/lib/librte_eal/common/eal_common_fbarray.c b/lib/librte_eal/common/eal_common_fbarray.c\nindex ea0735cb8..819d097bf 100644\n--- a/lib/librte_eal/common/eal_common_fbarray.c\n+++ b/lib/librte_eal/common/eal_common_fbarray.c\n@@ -28,6 +28,22 @@\n #define MASK_LEN_TO_MOD(x) ((x) - RTE_ALIGN_FLOOR(x, MASK_ALIGN))\n #define MASK_GET_IDX(idx, mod) ((idx << MASK_SHIFT) + mod)\n \n+/*\n+ * We use this to keep track of created/attached memory areas to prevent user\n+ * errors in API usage.\n+ */\n+struct mem_area {\n+\tTAILQ_ENTRY(mem_area) next;\n+\tvoid *addr;\n+\tsize_t len;\n+\tint fd;\n+};\n+TAILQ_HEAD(mem_area_head, mem_area);\n+/* local per-process tailq */\n+static struct mem_area_head mem_area_tailq =\n+\tTAILQ_HEAD_INITIALIZER(mem_area_tailq);\n+static rte_spinlock_t mem_area_lock = RTE_SPINLOCK_INITIALIZER;\n+\n /*\n * This is a mask that is always stored at the end of array, to provide fast\n * way of finding free/used spots without looping through each element.\n@@ -87,6 +103,22 @@ resize_and_map(int fd, void *addr, size_t len)\n \treturn 0;\n }\n \n+static int\n+overlap(const struct mem_area *ma, const void *start, size_t len)\n+{\n+\tconst void *end = RTE_PTR_ADD(start, len);\n+\tconst void *ma_start = ma->addr;\n+\tconst void *ma_end = RTE_PTR_ADD(ma->addr, ma->len);\n+\n+\t/* start overlap? */\n+\tif (start >= ma_start && start < ma_end)\n+\t\treturn 1;\n+\t/* end overlap? */\n+\tif (end >= ma_start && end < ma_end)\n+\t\treturn 1;\n+\treturn 0;\n+}\n+\n static int\n find_next_n(const struct rte_fbarray *arr, unsigned int start, unsigned int n,\n \t bool used)\n@@ -684,6 +716,7 @@ rte_fbarray_init(struct rte_fbarray *arr, const char *name, unsigned int len,\n \tsize_t page_sz, mmap_len;\n \tchar path[PATH_MAX];\n \tstruct used_mask *msk;\n+\tstruct mem_area *ma = NULL;\n \tvoid *data = NULL;\n \tint fd = -1;\n \n@@ -695,6 +728,13 @@ rte_fbarray_init(struct rte_fbarray *arr, const char *name, unsigned int len,\n \tif (fully_validate(name, elt_sz, len))\n \t\treturn -1;\n \n+\t/* allocate mem area before doing anything */\n+\tma = malloc(sizeof(*ma));\n+\tif (ma == NULL) {\n+\t\trte_errno = ENOMEM;\n+\t\treturn -1;\n+\t}\n+\n \tpage_sz = sysconf(_SC_PAGESIZE);\n \tif (page_sz == (size_t)-1)\n \t\tgoto fail;\n@@ -706,10 +746,14 @@ rte_fbarray_init(struct rte_fbarray *arr, const char *name, unsigned int len,\n \tif (data == NULL)\n \t\tgoto fail;\n \n+\trte_spinlock_lock(&mem_area_lock);\n+\n+\tfd = -1;\n+\n \tif (internal_config.no_shconf) {\n \t\t/* remap virtual area as writable */\n \t\tvoid *new_data = mmap(data, mmap_len, PROT_READ | PROT_WRITE,\n-\t\t\t\tMAP_FIXED | MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);\n+\t\t\t\tMAP_FIXED | MAP_PRIVATE | MAP_ANONYMOUS, fd, 0);\n \t\tif (new_data == MAP_FAILED) {\n \t\t\tRTE_LOG(DEBUG, EAL, \"%s(): couldn't remap anonymous memory: %s\\n\",\n \t\t\t\t\t__func__, strerror(errno));\n@@ -748,10 +792,13 @@ rte_fbarray_init(struct rte_fbarray *arr, const char *name, unsigned int len,\n \n \t\tif (resize_and_map(fd, data, mmap_len))\n \t\t\tgoto fail;\n-\n-\t\t/* we've mmap'ed the file, we can now close the fd */\n-\t\tclose(fd);\n \t}\n+\tma->addr = data;\n+\tma->len = mmap_len;\n+\tma->fd = fd;\n+\n+\t/* do not close fd - keep it until detach/destroy */\n+\tTAILQ_INSERT_TAIL(&mem_area_tailq, ma, next);\n \n \t/* initialize the data */\n \tmemset(data, 0, mmap_len);\n@@ -768,18 +815,24 @@ rte_fbarray_init(struct rte_fbarray *arr, const char *name, unsigned int len,\n \n \trte_rwlock_init(&arr->rwlock);\n \n+\trte_spinlock_unlock(&mem_area_lock);\n+\n \treturn 0;\n fail:\n \tif (data)\n \t\tmunmap(data, mmap_len);\n \tif (fd >= 0)\n \t\tclose(fd);\n+\tfree(ma);\n+\n+\trte_spinlock_unlock(&mem_area_lock);\n \treturn -1;\n }\n \n int __rte_experimental\n rte_fbarray_attach(struct rte_fbarray *arr)\n {\n+\tstruct mem_area *ma = NULL, *tmp = NULL;\n \tsize_t page_sz, mmap_len;\n \tchar path[PATH_MAX];\n \tvoid *data = NULL;\n@@ -799,12 +852,30 @@ rte_fbarray_attach(struct rte_fbarray *arr)\n \tif (fully_validate(arr->name, arr->elt_sz, arr->len))\n \t\treturn -1;\n \n+\tma = malloc(sizeof(*ma));\n+\tif (ma == NULL) {\n+\t\trte_errno = ENOMEM;\n+\t\treturn -1;\n+\t}\n+\n \tpage_sz = sysconf(_SC_PAGESIZE);\n \tif (page_sz == (size_t)-1)\n \t\tgoto fail;\n \n \tmmap_len = calc_data_size(page_sz, arr->elt_sz, arr->len);\n \n+\t/* check the tailq - maybe user has already mapped this address space */\n+\trte_spinlock_lock(&mem_area_lock);\n+\n+\tTAILQ_FOREACH(tmp, &mem_area_tailq, next) {\n+\t\tif (overlap(tmp, arr->data, mmap_len)) {\n+\t\t\trte_errno = EEXIST;\n+\t\t\tgoto fail;\n+\t\t}\n+\t}\n+\n+\t/* we know this memory area is unique, so proceed */\n+\n \tdata = eal_get_virtual_area(arr->data, &mmap_len, page_sz, 0, 0);\n \tif (data == NULL)\n \t\tgoto fail;\n@@ -826,7 +897,12 @@ rte_fbarray_attach(struct rte_fbarray *arr)\n \tif (resize_and_map(fd, data, mmap_len))\n \t\tgoto fail;\n \n-\tclose(fd);\n+\t/* store our new memory area */\n+\tma->addr = data;\n+\tma->fd = fd; /* keep fd until detach/destroy */\n+\tma->len = mmap_len;\n+\n+\tTAILQ_INSERT_TAIL(&mem_area_tailq, ma, next);\n \n \t/* we're done */\n \n@@ -836,12 +912,17 @@ rte_fbarray_attach(struct rte_fbarray *arr)\n \t\tmunmap(data, mmap_len);\n \tif (fd >= 0)\n \t\tclose(fd);\n+\tfree(ma);\n \treturn -1;\n }\n \n int __rte_experimental\n rte_fbarray_detach(struct rte_fbarray *arr)\n {\n+\tstruct mem_area *tmp = NULL;\n+\tsize_t mmap_len;\n+\tint ret = -1;\n+\n \tif (arr == NULL) {\n \t\trte_errno = EINVAL;\n \t\treturn -1;\n@@ -860,49 +941,114 @@ rte_fbarray_detach(struct rte_fbarray *arr)\n \tif (page_sz == (size_t)-1)\n \t\treturn -1;\n \n-\t/* this may already be unmapped (e.g. repeated call from previously\n-\t * failed destroy(), but this is on user, we can't (easily) know if this\n-\t * is still mapped.\n-\t */\n-\tmunmap(arr->data, calc_data_size(page_sz, arr->elt_sz, arr->len));\n+\tmmap_len = calc_data_size(page_sz, arr->elt_sz, arr->len);\n \n-\treturn 0;\n+\t/* does this area exist? */\n+\trte_spinlock_lock(&mem_area_lock);\n+\n+\tTAILQ_FOREACH(tmp, &mem_area_tailq, next) {\n+\t\tif (tmp->addr == arr->data && tmp->len == mmap_len)\n+\t\t\tbreak;\n+\t}\n+\tif (tmp == NULL) {\n+\t\trte_errno = ENOENT;\n+\t\tret = -1;\n+\t\tgoto out;\n+\t}\n+\n+\tmunmap(arr->data, mmap_len);\n+\n+\t/* area is unmapped, close fd and remove the tailq entry */\n+\tif (tmp->fd >= 0)\n+\t\tclose(tmp->fd);\n+\tTAILQ_REMOVE(&mem_area_tailq, tmp, next);\n+\tfree(tmp);\n+\n+\tret = 0;\n+out:\n+\trte_spinlock_unlock(&mem_area_lock);\n+\treturn ret;\n }\n \n int __rte_experimental\n rte_fbarray_destroy(struct rte_fbarray *arr)\n {\n+\tstruct mem_area *tmp = NULL;\n+\tsize_t mmap_len;\n \tint fd, ret;\n \tchar path[PATH_MAX];\n \n-\tret = rte_fbarray_detach(arr);\n-\tif (ret)\n-\t\treturn ret;\n+\tif (arr == NULL) {\n+\t\trte_errno = EINVAL;\n+\t\treturn -1;\n+\t}\n \n+\t/*\n+\t * we don't need to synchronize detach as two values we need (element\n+\t * size and total capacity) are constant for the duration of life of\n+\t * the array, so the parts we care about will not race. if the user is\n+\t * detaching while doing something else in the same process, we can't\n+\t * really do anything about it, things will blow up either way.\n+\t */\n+\n+\tsize_t page_sz = sysconf(_SC_PAGESIZE);\n+\n+\tif (page_sz == (size_t)-1)\n+\t\treturn -1;\n+\n+\tmmap_len = calc_data_size(page_sz, arr->elt_sz, arr->len);\n+\n+\t/* does this area exist? */\n+\trte_spinlock_lock(&mem_area_lock);\n+\n+\tTAILQ_FOREACH(tmp, &mem_area_tailq, next) {\n+\t\tif (tmp->addr == arr->data && tmp->len == mmap_len)\n+\t\t\tbreak;\n+\t}\n+\tif (tmp == NULL) {\n+\t\trte_errno = ENOENT;\n+\t\tret = -1;\n+\t\tgoto out;\n+\t}\n \t/* with no shconf, there were never any files to begin with */\n-\tif (internal_config.no_shconf)\n-\t\treturn 0;\n+\tif (!internal_config.no_shconf) {\n+\t\t/*\n+\t\t * attempt to get an exclusive lock on the file, to ensure it\n+\t\t * has been detached by all other processes\n+\t\t */\n+\t\tfd = tmp->fd;\n+\t\tif (flock(fd, LOCK_EX | LOCK_NB)) {\n+\t\t\tRTE_LOG(DEBUG, EAL, \"Cannot destroy fbarray - another process is using it\\n\");\n+\t\t\trte_errno = EBUSY;\n+\t\t\tret = -1;\n+\t\t\tgoto out;\n+\t\t}\n \n-\t/* try deleting the file */\n-\teal_get_fbarray_path(path, sizeof(path), arr->name);\n+\t\t/* we're OK to destroy the file */\n+\t\teal_get_fbarray_path(path, sizeof(path), arr->name);\n+\t\tif (unlink(path)) {\n+\t\t\tRTE_LOG(DEBUG, EAL, \"Cannot unlink fbarray: %s\\n\",\n+\t\t\t\tstrerror(errno));\n+\t\t\trte_errno = errno;\n+\t\t\t/*\n+\t\t\t * we're still holding an exclusive lock, so drop it to\n+\t\t\t * shared.\n+\t\t\t */\n+\t\t\tflock(fd, LOCK_SH | LOCK_NB);\n \n-\tfd = open(path, O_RDONLY);\n-\tif (fd < 0) {\n-\t\tRTE_LOG(ERR, EAL, \"Could not open fbarray file: %s\\n\",\n-\t\t\tstrerror(errno));\n-\t\treturn -1;\n+\t\t\tret = -1;\n+\t\t\tgoto out;\n+\t\t}\n+\t\tclose(fd);\n \t}\n-\tif (flock(fd, LOCK_EX | LOCK_NB)) {\n-\t\tRTE_LOG(DEBUG, EAL, \"Cannot destroy fbarray - another process is using it\\n\");\n-\t\trte_errno = EBUSY;\n-\t\tret = -1;\n-\t} else {\n-\t\tret = 0;\n-\t\tunlink(path);\n-\t\tmemset(arr, 0, sizeof(*arr));\n-\t}\n-\tclose(fd);\n+\tmunmap(arr->data, mmap_len);\n \n+\t/* area is unmapped, remove the tailq entry */\n+\tTAILQ_REMOVE(&mem_area_tailq, tmp, next);\n+\tfree(tmp);\n+\tret = 0;\n+out:\n+\trte_spinlock_unlock(&mem_area_lock);\n \treturn ret;\n }\n \n", "prefixes": [] }