[dpdk-dev] [PATCH v2 1/1] mempool: Add sanity check when secondary link in less mempools than primary
If the mempool ops the caller wants to use is not registered, the
library will segfault in an obscure way when trying to use that
mempool. It's better to catch it early and warn the user.
If the primary and secondary process were build using different build
systems, the list of constructors included by the linker in each
binary might be different. Mempools are registered via constructors, so
the linker magic will directly impact which tailqs are registered with
the primary and the secondary.
DPDK currently assumes that the secondary has a superset of the
mempools registered at the primary, and they are in the same order
(same index in primary and secondary). In some build scenario, the
secondary might not initialise any mempools at all.
This would also catch cases where there is a bug in the mempool
registration, or some memory corruptions, but this has not been
observed.
Signed-off-by: Jean Tourrilhes
---
lib/librte_mempool/rte_mempool.c | 19 +++
1 file changed, 19 insertions(+)
diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
index 2e28e2e..82260cc 100644
--- a/lib/librte_mempool/rte_mempool.c
+++ b/lib/librte_mempool/rte_mempool.c
@@ -1275,6 +1275,25 @@ rte_mempool_lookup(const char *name)
return NULL;
}
+ /* Sanity check : secondary may have initialised less mempools
+* than primary due to linker and constructor magic. Or maybe
+* there is a mempool corruption or bug. In any case, we can't
+* go on, we will segfault in an obscure way.
+* This does not detect the case where the constructor order
+* is different between primary and secondary and where the
+* index points to the wrong ops. This would require more
+* extensive changes, and is much less likely.
+* Jean II */
+ if(mp->ops_index >= (int32_t) rte_mempool_ops_table.num_ops) {
+ unsigned i;
+ /* Dump list of mempool ops for further investigation. */
+ for (i = 0; i < rte_mempool_ops_table.num_ops; i++) {
+ RTE_LOG(ERR, EAL, "Registered mempool[%d] is %s\n", i,
rte_mempool_ops_table.ops[i].name);
+ }
+ /* Do not dump mempool list itself, it will segfault. */
+ rte_panic("Cannot find ops for mempool, ops_index %d, num_ops
%d - maybe due to build process or linker configuration\n", mp->ops_index,
rte_mempool_ops_table.num_ops);
+ }
+
return mp;
}
[dpdk-dev] [PATCH 1/1] eal: Fix misleading error messages, errno can't be trusted.
lib/librte_eal/linuxapp/eal/eal.c| 14 +++---
lib/librte_eal/linuxapp/eal/eal_memory.c | 16
2 files changed, 23 insertions(+), 7 deletions(-)
diff --git a/lib/librte_eal/linuxapp/eal/eal.c
b/lib/librte_eal/linuxapp/eal/eal.c
index 3fb2188..f6907f5 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -238,7 +238,8 @@ rte_eal_config_attach(void)
mem_config = (struct rte_mem_config *) mmap(NULL, sizeof(*mem_config),
PROT_READ, MAP_SHARED, mem_cfg_fd, 0);
if (mem_config == MAP_FAILED)
- rte_panic("Cannot mmap memory for rte_config\n");
+ rte_panic("Cannot mmap memory for rte_config! error %i (%s)\n",
+ errno, strerror(errno));
rte_config.mem_config = mem_config;
}
@@ -263,9 +264,16 @@ rte_eal_config_reattach(void)
mem_config = (struct rte_mem_config *) mmap(rte_mem_cfg_addr,
sizeof(*mem_config), PROT_READ | PROT_WRITE, MAP_SHARED,
mem_cfg_fd, 0);
+ if (mem_config == MAP_FAILED || mem_config != rte_mem_cfg_addr) {
+ if (mem_config != rte_mem_cfg_addr)
+ /* errno is stale, don't use */
+ rte_panic("Cannot mmap memory for rte_config at [%p],
got [%p] - please use '--base-virtaddr' option\n",
+ rte_mem_cfg_addr, mem_config);
+ else
+ rte_panic("Cannot mmap memory for rte_config! error %i
(%s)\n",
+ errno, strerror(errno));
+ }
close(mem_cfg_fd);
- if (mem_config == MAP_FAILED || mem_config != rte_mem_cfg_addr)
- rte_panic("Cannot mmap memory for rte_config\n");
rte_config.mem_config = mem_config;
}
diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c
b/lib/librte_eal/linuxapp/eal/eal_memory.c
index 41e0a92..f2549cc 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memory.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
@@ -1615,10 +1615,18 @@ rte_eal_hugepage_attach(void)
PROT_READ, MAP_PRIVATE, fd_zero, 0);
if (base_addr == MAP_FAILED ||
base_addr != mcfg->memseg[s].addr) {
- RTE_LOG(ERR, EAL, "Could not mmap %llu bytes "
- "in /dev/zero to requested address [%p]:
'%s'\n",
- (unsigned long long)mcfg->memseg[s].len,
- mcfg->memseg[s].addr, strerror(errno));
+ if (base_addr != mcfg->memseg[s].addr)
+ /* errno is stale, don't use */
+ RTE_LOG(ERR, EAL, "Could not mmap %llu bytes "
+ "in /dev/zero at [%p], got [%p] - "
+ "please use '--base-virtaddr' option\n",
+ (unsigned long long)mcfg->memseg[s].len,
+ mcfg->memseg[s].addr, base_addr);
+ else
+ RTE_LOG(ERR, EAL, "Could not mmap %llu bytes "
+ "in /dev/zero at [%p]: '%s'\n",
+ (unsigned long long)mcfg->memseg[s].len,
+ mcfg->memseg[s].addr, strerror(errno));
if (aslr_enabled() > 0) {
RTE_LOG(ERR, EAL, "It is recommended to "
"disable ASLR in the kernel "
[dpdk-dev] [PATCH 1/1 v2] eal: Fix misleading error messages, errno can't be trusted.
lib/librte_eal/linuxapp/eal/eal.c| 14 +++---
lib/librte_eal/linuxapp/eal/eal_memory.c | 16
2 files changed, 23 insertions(+), 7 deletions(-)
diff --git a/lib/librte_eal/linuxapp/eal/eal.c
b/lib/librte_eal/linuxapp/eal/eal.c
index 3fb2188..5df9f6a 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -238,7 +238,8 @@ rte_eal_config_attach(void)
mem_config = (struct rte_mem_config *) mmap(NULL, sizeof(*mem_config),
PROT_READ, MAP_SHARED, mem_cfg_fd, 0);
if (mem_config == MAP_FAILED)
- rte_panic("Cannot mmap memory for rte_config\n");
+ rte_panic("Cannot mmap memory for rte_config! error %i (%s)\n",
+ errno, strerror(errno));
rte_config.mem_config = mem_config;
}
@@ -263,9 +264,16 @@ rte_eal_config_reattach(void)
mem_config = (struct rte_mem_config *) mmap(rte_mem_cfg_addr,
sizeof(*mem_config), PROT_READ | PROT_WRITE, MAP_SHARED,
mem_cfg_fd, 0);
+ if (mem_config == MAP_FAILED || mem_config != rte_mem_cfg_addr) {
+ if (mem_config != MAP_FAILED)
+ /* errno is stale, don't use */
+ rte_panic("Cannot mmap memory for rte_config at [%p],
got [%p] - please use '--base-virtaddr' option\n",
+ rte_mem_cfg_addr, mem_config);
+ else
+ rte_panic("Cannot mmap memory for rte_config! error %i
(%s)\n",
+ errno, strerror(errno));
+ }
close(mem_cfg_fd);
- if (mem_config == MAP_FAILED || mem_config != rte_mem_cfg_addr)
- rte_panic("Cannot mmap memory for rte_config\n");
rte_config.mem_config = mem_config;
}
diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c
b/lib/librte_eal/linuxapp/eal/eal_memory.c
index 41e0a92..b036ffc 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memory.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
@@ -1615,10 +1615,18 @@ rte_eal_hugepage_attach(void)
PROT_READ, MAP_PRIVATE, fd_zero, 0);
if (base_addr == MAP_FAILED ||
base_addr != mcfg->memseg[s].addr) {
- RTE_LOG(ERR, EAL, "Could not mmap %llu bytes "
- "in /dev/zero to requested address [%p]:
'%s'\n",
- (unsigned long long)mcfg->memseg[s].len,
- mcfg->memseg[s].addr, strerror(errno));
+ if (base_addr != MAP_FAILED)
+ /* errno is stale, don't use */
+ RTE_LOG(ERR, EAL, "Could not mmap %llu bytes "
+ "in /dev/zero at [%p], got [%p] - "
+ "please use '--base-virtaddr' option\n",
+ (unsigned long long)mcfg->memseg[s].len,
+ mcfg->memseg[s].addr, base_addr);
+ else
+ RTE_LOG(ERR, EAL, "Could not mmap %llu bytes "
+ "in /dev/zero at [%p]: '%s'\n",
+ (unsigned long long)mcfg->memseg[s].len,
+ mcfg->memseg[s].addr, strerror(errno));
if (aslr_enabled() > 0) {
RTE_LOG(ERR, EAL, "It is recommended to "
"disable ASLR in the kernel "
[dpdk-dev] [Bug] Static constructors considered evil
Hi, Expecting static constructors to match between the primary and the secondary is ill advised and putting yourself at the mercy of the linker magic. In this case, both primary and secondary were compiled using the same DPDK directory and exact same libdpdk.a. Config : -- CONFIG_RTE_LIBRTE_HASH=y CONFIG_RTE_LIBRTE_LPM=y CONFIG_RTE_LIBRTE_ACL=y CONFIG_RTE_LIBRTE_DISTRIBUTOR=y CONFIG_RTE_LIBRTE_REORDER=y Primary : --- EAL: Registered tailq: RTE_ACL EAL: Registered tailq: RTE_HASH EAL: Registered tailq: RTE_FBK_HASH EAL: Registered tailq: RTE_MEMPOOL EAL: Registered tailq: RTE_RING EAL: Registered tailq: VFIO_RESOURCE_LIST EAL: Registered tailq: UIO_RESOURCE_LIST Tailq 0: qname:, tqh_first:(nil), tqh_last:0x7feff41c Tailq 1: qname:, tqh_first:(nil), tqh_last:0x7feff44c Tailq 2: qname:, tqh_first:(nil), tqh_last:0x7feff47c Tailq 3: qname:, tqh_first:(nil), tqh_last:0x7feff4ac Tailq 4: qname:, tqh_first:(nil), tqh_last:0x7feff4dc Tailq 5: qname:, tqh_first:(nil), tqh_last:0x7feff50c Tailq 6: qname:, tqh_first:(nil), tqh_last:0x7feff53c Tailq 7: qname:<>, tqh_first:(nil), tqh_last:(nil) Tailq 8: qname:<>, tqh_first:(nil), tqh_last:(nil) Tailq 9: qname:<>, tqh_first:(nil), tqh_last:(nil) Secondary : - EAL: Registered tailq: RTE_LPM EAL: Registered tailq: RTE_LPM6 EAL: Registered tailq: RTE_HASH EAL: Registered tailq: RTE_FBK_HASH EAL: Registered tailq: RTE_ACL EAL: Registered tailq: RTE_REORDER EAL: Registered tailq: VFIO_RESOURCE_LIST EAL: Registered tailq: UIO_RESOURCE_LIST EAL: Registered tailq: RTE_DISTRIBUTOR EAL: Registered tailq: RTE_MEMPOOL EAL: Registered tailq: RTE_RING EAL: Cannot initialize tailq: RTE_LPM Tailq 0: qname:, tqh_first:(nil), tqh_last:0x7feff41c Tailq 1: qname:, tqh_first:0x7ff0004c62c0, tqh_last:0x7ff0004c62c0 Tailq 2: qname:, tqh_first:(nil), tqh_last:0x7feff47c Tailq 3: qname:, tqh_first:0x7ff0004a81c0, tqh_last:0x7ff0004a8040 Tailq 4: qname:, tqh_first:0x7ff0004a8140, tqh_last:0x7ff0004c6340 Tailq 5: qname:, tqh_first:0x7ff0005fee80, tqh_last:0x7ff00052be80 Tailq 6: qname:, tqh_first:(nil), tqh_last:0x7feff53c Tailq 7: qname:<>, tqh_first:(nil), tqh_last:(nil) Tailq 8: qname:<>, tqh_first:(nil), tqh_last:(nil) Tailq 9: qname:<>, tqh_first:(nil), tqh_last:(nil) [...] PANIC in rte_eal_init(): Cannot init tail queues for objects Obviously not happy... Any advices ? Jean
[dpdk-dev] [PATCH 1/1] eal: Don't fail secondary if primary is missing tailqs
lib/librte_eal/common/eal_common_tailqs.c | 15 ---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/lib/librte_eal/common/eal_common_tailqs.c
b/lib/librte_eal/common/eal_common_tailqs.c
index bb08ec8..6960d06 100644
--- a/lib/librte_eal/common/eal_common_tailqs.c
+++ b/lib/librte_eal/common/eal_common_tailqs.c
@@ -143,6 +143,8 @@ rte_eal_tailq_update(struct rte_tailq_elem *t)
t->head = rte_eal_tailq_create(t->name);
} else {
t->head = rte_eal_tailq_lookup(t->name);
+ if (t->head != NULL)
+ rte_tailqs_count++;
}
}
@@ -188,9 +190,16 @@ rte_eal_tailqs_init(void)
if (t->head == NULL) {
RTE_LOG(ERR, EAL,
"Cannot initialize tailq: %s\n", t->name);
- /* no need to TAILQ_REMOVE, we are going to panic in
-* rte_eal_init() */
- goto fail;
+ if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+ /* no need to TAILQ_REMOVE, we are going
+* to panic in rte_eal_init() */
+ goto fail;
+ } else {
+ /* This means our list of constructor is
+* no the same as primary. Just remove
+* that missing tailq and continue */
+ TAILQ_REMOVE(&rte_tailq_elem_head, t, next);
+ }
}
}
[dpdk-dev] [PATCH v3] mempool: Add sanity check when secondary link-in less mempools than primary
If the mempool ops the caller wants to use is not registered, the
library will segfault in an obscure way when trying to use that
mempool. It's better to catch it early and warn the user.
If the primary and secondary process were build using different build
systems, the list of constructors included by the linker in each
binary might be different. Mempools are registered via constructors, so
the linker magic will directly impact which mempools are registered with
the primary and the secondary.
DPDK currently assumes that the secondary has a superset of the
mempools registered at the primary, and they are in the same order
(same index in primary and secondary). In some build scenario, the
secondary might not initialise any mempools at all.
This would also catch cases where there is a bug in the mempool
registration, or some memory corruptions, but this has not been
observed.
Signed-off-by: Jean Tourrilhes
---
lib/librte_mempool/rte_mempool.c | 23 +++
1 file changed, 23 insertions(+)
diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
index e94e56f..bbb6723 100644
--- a/lib/librte_mempool/rte_mempool.c
+++ b/lib/librte_mempool/rte_mempool.c
@@ -1273,6 +1273,29 @@ rte_mempool_lookup(const char *name)
return NULL;
}
+ /* Sanity check : secondary may have initialised less mempools
+* than primary due to linker and constructor magic. Or maybe
+* there is a mempool corruption or bug. In any case, we can't
+* go on, we will segfault in an obscure way.
+* This does not detect the cases where the constructor order
+* is different between primary and secondary or where the
+* index points to the wrong ops. This would require more
+* extensive changes, and is much less likely. Jean II */
+ if (mp->ops_index >= (int32_t) rte_mempool_ops_table.num_ops) {
+ unsigned i;
+ /* Dump list of mempool ops for further investigation.
+* In most cases, list is empty... */
+ for (i = 0; i < rte_mempool_ops_table.num_ops; i++) {
+ RTE_LOG(ERR, EAL, "Registered mempool[%d] is %s\n",
+ i, rte_mempool_ops_table.ops[i].name);
+ }
+ /* Do not dump mempool list itself, it will segfault. */
+ rte_panic("Cannot find ops for mempool, ops_index %d, "
+ "num_ops %d - maybe due to build process or "
+ "linker configuration\n",
+ mp->ops_index, rte_mempool_ops_table.num_ops);
+ }
+
return mp;
}
[dpdk-dev] [PATCH v2 1/1] mempool: Add sanity check when secondary link in less mempools than primary
On Fri, Nov 18, 2016 at 04:11:12PM +0100, Olivier Matz wrote: > Hi Jean, > > > Do you mind if we put back this conversation on the ML? Oh, I forgot to do it ? I intended to. Bummer. Please do so. > I think your example shows that there is no linker magic: you just > need the same linker flags for dpdk libraries than in the dpdk > framework. I suppose we need something in the build framework > to provide these flags externally, Good luck integrating that in all foreign build system (I'm looking at you, Snort). > but I don't think we need to patch mempool for that. This sanity check is just that, a sanity check. I don't understand what's bad about a sanity check, it does not change functionality, it does not fix anything and just warn users about those issues. Please look at the patch itself at face value. > Regards, > Olivier Regards, Jean
[dpdk-dev] [PATCH 1/1 v2] eal: Fix misleading error messages, errno can't be trusted.
On Mon, Oct 03, 2016 at 02:25:40PM +0100, Sergio Gonzalez Monroy wrote:
> Hi Jean,
>
> There are some format issues with the patch:
>
> You can run scripts/check-git-log.sh to check them:
> Wrong headline format:
> eal: Fix misleading error messages, errno can't be trusted.
> Wrong headline uppercase:
> eal: Fix misleading error messages, errno can't be trusted.
> Missing 'Fixes' tag:
> eal: Fix misleading error messages, errno can't be trusted.
>
> The script's output highlights the different issues.
SOrry about that, I casually read the page on
http://dpdk.org/dev, but obviously I need to look at it again.
> On 21/09/2016 22:10, Jean Tourrilhes wrote:
> >@@ -263,9 +264,16 @@ rte_eal_config_reattach(void)
> > mem_config = (struct rte_mem_config *) mmap(rte_mem_cfg_addr,
> > sizeof(*mem_config), PROT_READ | PROT_WRITE, MAP_SHARED,
> > mem_cfg_fd, 0);
> >+if (mem_config == MAP_FAILED || mem_config != rte_mem_cfg_addr) {
> >+if (mem_config != MAP_FAILED)
> >+/* errno is stale, don't use */
> >+rte_panic("Cannot mmap memory for rte_config at [%p],
> >got [%p] - please use '--base-virtaddr' option\n",
> >+ rte_mem_cfg_addr, mem_config);
> >+else
> >+rte_panic("Cannot mmap memory for rte_config! error %i
> >(%s)\n",
> >+ errno, strerror(errno));
> >+}
> > close(mem_cfg_fd);
> >-if (mem_config == MAP_FAILED || mem_config != rte_mem_cfg_addr)
> >-rte_panic("Cannot mmap memory for rte_config\n");
>
> NIT but any reason you moved the check before closing the file descriptor?
> (not that it matters with current code as we panic anyway)
"close()" may change "errno" according to its man page.
> Thanks,
> Sergio
Thanks for the review !
Jean
[dpdk-dev] [PATCH 1/1 v2] eal: Fix misleading error messages, errno can't be trusted.
On Mon, Oct 03, 2016 at 02:25:40PM +0100, Sergio Gonzalez Monroy wrote: > Hi Jean, > > NIT but any reason you moved the check before closing the file descriptor? > (not that it matters with current code as we panic anyway) > > Thanks, > Sergio More details, as I admit I was terse Running secondary is tricky due to the need to map the memory region at the right place, which is whatever primary has chosen. If the base address for primary happens to by already mapped in the secondary, we will hit precisely this error message (well, in a few case we might hit the other one). This is why there is already a comment about ASLR. A colleague of mine hit that message and was misled by errno claiming "permission denied", which sent him down the wrong track. It's such a common error for secondary that I feel this error message should be unambiguous and helpful. Regards, Jean
[dpdk-dev] [PATCH 1/1 v2] eal: Fix misleading error messages, errno can't be trusted.
On Mon, Oct 03, 2016 at 04:15:11PM +, Mcnamara, John wrote: > > The longer more detailed version is here: "Contributing Code to DPDK": > > http://dpdk.org/doc/guides/contributing/patches.html > > John Thanks a lot. I'll try to find time to look at it. Jean
[dpdk-dev] [PATCH 1/1] eal: Don't fail secondary if primary is missing tailqs
On Tue, Oct 04, 2016 at 02:11:39PM +0100, Sergio Gonzalez Monroy wrote: > Hi Jean, > > As with your other patch, commit title needs fixing and also missing > Signed-off-by line. I'll do that, no worries... > I might be missing something here so bear with me. Yes, I know I was terse. Sorry. > The case you are trying to fix is, as an example, when your secondary app is > using LPM but your primary is not. > So basically with this patch, you are removing the tailq for LPM on > secondary and continuing as normal, is that the case? The secondary can't use tailq types that the primary does not have, because they are shared across the shared memory. What happens is that the primary and secondary did not compile in the same list of tailq. See previous e-mail : http://dpdk.org/ml/archives/dev/2016-September/047329.html The reason it's happening is that the secondary was not compiled with the DPDK build system, but with the build system of the application (in this case, Snort). Oubviously, porting the application to the DPDK build system is not practical, so we need to live with this case. The build system of the application does not have all the subtelties of the DPDK build system, and ends up including *all* the constructors, wether they are used or not in the code. Moreover, they are included in a different order. Actually, by default the builds include no constructors at all (which is a big fail), so the library needs to be included with --whole-archive (see Snort DPDK instructions). > I am not convinced about this approach. I agree that the whole constructor approach is flaky and my patch is only a band aid. Constructors should be entirely removed IMHO, and a more deterministic init method should be used instead of depending on linker magic. Note that the other constructors happen to work right in my case, but that's probably pure luck. The list of mempool constructors happen to be the same and in the same order (order matters for mempool constructors). The app is not using spinlock, hash, crc and acl, so I did not look if the lists did match. > I guess the assumption here is that all the TAILQ infrastructure works even > when the tailq list itself is NULL? If tailq are not used in the primary, I assume it would work. > I say assumption because I don't have an easy way to trigger this use case > with default DPDK sample/test apps. I know. I'll see what I can do to release the code. > What about letting the secondary process create a tailq if it doesn't > exists? Primary owns the shared memory, and I don't see how primary could handle an unknown tailq. Secondary can't do much without primary. So, I don't see how adding the missing tailqs helps. > We would need to lock protect at least the register function to avoid race > conditions when having multiple secondary processes. > > David, what do you think? > > Sergio Regards, Jean
[dpdk-dev] [PATCH v2 1/3] mem: fix hugepage mapping error messages
Running secondary is tricky due to the need to map the memory region
at the right place in VM, which is whatever primary has chosen. If the
base address for primary happens to by already mapped in the
secondary, we will hit precisely these error messages (depending if we
fail on the config region or the hugepages). This is why there is
already a comment about ASLR.
The issue is that in most cases, remapping does not happen and "errno"
is not changed and therefore stale. In our case, we got a "permission
denied", which sent us down the wrong track. It's such a common error
for secondary that I feel this error message should be unambiguous and
helpful.
The call to close was also moved because close() may override errno.
Signed-off-by: Jean Tourrilhes
---
lib/librte_eal/linuxapp/eal/eal.c| 14 +++---
lib/librte_eal/linuxapp/eal/eal_memory.c | 16
2 files changed, 23 insertions(+), 7 deletions(-)
diff --git a/lib/librte_eal/linuxapp/eal/eal.c
b/lib/librte_eal/linuxapp/eal/eal.c
index 3fb2188..5df9f6a 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -238,7 +238,8 @@ rte_eal_config_attach(void)
mem_config = (struct rte_mem_config *) mmap(NULL, sizeof(*mem_config),
PROT_READ, MAP_SHARED, mem_cfg_fd, 0);
if (mem_config == MAP_FAILED)
- rte_panic("Cannot mmap memory for rte_config\n");
+ rte_panic("Cannot mmap memory for rte_config! error %i (%s)\n",
+ errno, strerror(errno));
rte_config.mem_config = mem_config;
}
@@ -263,9 +264,16 @@ rte_eal_config_reattach(void)
mem_config = (struct rte_mem_config *) mmap(rte_mem_cfg_addr,
sizeof(*mem_config), PROT_READ | PROT_WRITE, MAP_SHARED,
mem_cfg_fd, 0);
+ if (mem_config == MAP_FAILED || mem_config != rte_mem_cfg_addr) {
+ if (mem_config != MAP_FAILED)
+ /* errno is stale, don't use */
+ rte_panic("Cannot mmap memory for rte_config at [%p],
got [%p] - please use '--base-virtaddr' option\n",
+ rte_mem_cfg_addr, mem_config);
+ else
+ rte_panic("Cannot mmap memory for rte_config! error %i
(%s)\n",
+ errno, strerror(errno));
+ }
close(mem_cfg_fd);
- if (mem_config == MAP_FAILED || mem_config != rte_mem_cfg_addr)
- rte_panic("Cannot mmap memory for rte_config\n");
rte_config.mem_config = mem_config;
}
diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c
b/lib/librte_eal/linuxapp/eal/eal_memory.c
index 41e0a92..b036ffc 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memory.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
@@ -1615,10 +1615,18 @@ rte_eal_hugepage_attach(void)
PROT_READ, MAP_PRIVATE, fd_zero, 0);
if (base_addr == MAP_FAILED ||
base_addr != mcfg->memseg[s].addr) {
- RTE_LOG(ERR, EAL, "Could not mmap %llu bytes "
- "in /dev/zero to requested address [%p]:
'%s'\n",
- (unsigned long long)mcfg->memseg[s].len,
- mcfg->memseg[s].addr, strerror(errno));
+ if (base_addr != MAP_FAILED)
+ /* errno is stale, don't use */
+ RTE_LOG(ERR, EAL, "Could not mmap %llu bytes "
+ "in /dev/zero at [%p], got [%p] - "
+ "please use '--base-virtaddr' option\n",
+ (unsigned long long)mcfg->memseg[s].len,
+ mcfg->memseg[s].addr, base_addr);
+ else
+ RTE_LOG(ERR, EAL, "Could not mmap %llu bytes "
+ "in /dev/zero at [%p]: '%s'\n",
+ (unsigned long long)mcfg->memseg[s].len,
+ mcfg->memseg[s].addr, strerror(errno));
if (aslr_enabled() > 0) {
RTE_LOG(ERR, EAL, "It is recommended to "
"disable ASLR in the kernel "
[dpdk-dev] [PATCH v2 1/3] mem: fix hugepage mapping error messages
On Wed, Oct 05, 2016 at 11:51:48AM +0200, Thomas Monjalon wrote: > > Applied, thanks > A rebase was necessary because of this patch: http://dpdk.org/commit/c00ae961 > Please check everything is OK. Tested today's master. Working as expected. Thanks ! Jean
[dpdk-dev] [PATCH 1/1] eal: Don't fail secondary if primary is missing tailqs
On Wed, Oct 05, 2016 at 09:58:01AM +0200, David Marchand wrote: > Hello, Hi there, > I thought you had unaligned binaries. > You are compiling only one binary ? Primary is compiled using the DPDK build process. Secondary is build using the Snort build process. Both are pointing to the exact same libdpdk.a. > I am not sure Sergio is talking about the constructor approach. But, this is exactly the cause of the problem. > Anyway, the constructors invocation order should not matter. For tailq, I agree. For mempool constructors, order do matter. > Primary and secondary processes build their local tailq entries list > in constructors (so far, I can't see how this is wrong). > "Later", each process updates this list with the actual pointer to the > lists by looking at the shared memory in rte_eal_init (calling > rte_eal_tailqs_init). > > What matters is that secondary tailqs are a subset of the primary tailqs. Which is not the case for me, I have secondary including all tailqs, and primary only having a subset. Check here : http://dpdk.org/ml/archives/dev/2016-September/047329.html > I still have some trouble understanding what you are trying to do. Having things work ;-) > As Sergio asked, can you come up with a simplified example/use case ? Not trivial. I'll see what I can do. > Thanks. > > > -- > David Marchand Regards, Jean
[dpdk-dev] [PATCH 1/1] eal: Don't fail secondary if primary is missing tailqs
On Wed, Oct 05, 2016 at 07:09:14PM +0200, Thomas Monjalon wrote: > > Probably that you would have some aligned builds if Snort was using > a pkg-config approach to link DPDK. I seriously doubt it, but maybe there is some deep linker magic that would pick the appropriate set of constructor. > > For tailq, I agree. For mempool constructors, order do matter. > > I don't know why such a complex function (rte_mempool_register_ops) is > called inside a constructor. Maybe that's the main problem. No. The problem is that the list of constructors linked by the linker in each binary is different, whereas DPDK expect them to be the same. Regards, Jean
[dpdk-dev] [PATCH v2] eal: don't fail secondary if primary is missing tailqs
If the primary and secondary process were build using different build
systems, the list of constructors included by the linker in each
binary might be different. Tailqs are registered via constructors, so
the linker magic will directly impact which tailqs are registered with
the primary and the secondary.
DPDK currently assumes that the secondary has a subset of the tailqs
registered at the primary. In some build scenario, the secondary might
register a tailq that the primary did not register. In this case,
instead of exiting with a panic, just unregister the offending tailq
and allow the secondary to run.
Signed-off-by: Jean Tourrilhes
---
lib/librte_eal/common/eal_common_tailqs.c | 18 ++
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/lib/librte_eal/common/eal_common_tailqs.c
b/lib/librte_eal/common/eal_common_tailqs.c
index bb08ec8..cf5a771 100644
--- a/lib/librte_eal/common/eal_common_tailqs.c
+++ b/lib/librte_eal/common/eal_common_tailqs.c
@@ -143,6 +143,8 @@ rte_eal_tailq_update(struct rte_tailq_elem *t)
t->head = rte_eal_tailq_create(t->name);
} else {
t->head = rte_eal_tailq_lookup(t->name);
+ if (t->head != NULL)
+ rte_tailqs_count++;
}
}
@@ -178,19 +180,27 @@ int
rte_eal_tailqs_init(void)
{
struct rte_tailq_elem *t;
+ void *tmp_te;
rte_tailqs_count = 0;
- TAILQ_FOREACH(t, &rte_tailq_elem_head, next) {
+ TAILQ_FOREACH_SAFE(t, &rte_tailq_elem_head, next, tmp_te) {
/* second part of register job for "early" tailqs, see
* rte_eal_tailq_register and EAL_REGISTER_TAILQ */
rte_eal_tailq_update(t);
if (t->head == NULL) {
RTE_LOG(ERR, EAL,
"Cannot initialize tailq: %s\n", t->name);
- /* no need to TAILQ_REMOVE, we are going to panic in
-* rte_eal_init() */
- goto fail;
+ if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+ /* no need to TAILQ_REMOVE, we are going
+* to panic in rte_eal_init() */
+ goto fail;
+ } else {
+ /* This means our list of constructor is
+* no the same as primary. Just remove
+* that missing tailq and continue */
+ TAILQ_REMOVE(&rte_tailq_elem_head, t, next);
+ }
}
}
[dpdk-dev] [PATCH] mempool: Add sanity check when secondary link in less mempools than primary
mempool: Add sanity check when secondary link in less mempools than primary
If the primary and secondary process were build using different build
systems, the list of constructors included by the linker in each
binary might be different. Mempools are registered via constructors, so
the linker magic will directly impact which tailqs are registered with
the primary and the secondary.
DPDK currently assumes that the secondary has a superset of the
mempools registered at the primary, and they are in the same order
(same index in primary and secondary). In some build scenario, the
secondary might not initialise any mempools at all. This would result
in an obscure segfault when trying to use the mempool. Instead, fail
early with a more explicit error message.
Signed-off-by: Jean Tourrilhes
---
lib/librte_mempool/rte_mempool.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
index 2e28e2e..4fe9158 100644
--- a/lib/librte_mempool/rte_mempool.c
+++ b/lib/librte_mempool/rte_mempool.c
@@ -1275,6 +1275,16 @@ rte_mempool_lookup(const char *name)
return NULL;
}
+ /* Sanity check : secondary may have initialised less mempools
+* than primary due to linker and constructor magic. Note that
+* this does not address the case where the constructor order
+* is different between primary and secondary and where the index
+* points to the wrong ops. Jean II */
+ if(mp->ops_index >= (int32_t) rte_mempool_ops_table.num_ops) {
+ /* Do not dump mempool list, it will segfault. */
+ rte_panic("Cannot find ops for mempool, ops_index %d, num_ops
%d - maybe due to build process or linker configuration\n", mp->ops_index,
rte_mempool_ops_table.num_ops);
+ }
+
return mp;
}
[dpdk-dev] [PATCH] mempool: Add sanity check when secondary link in less mempools than primary
On Fri, Oct 14, 2016 at 10:23:31AM +0200, Olivier Matz wrote: > Hi Jean, > > I'm not really fan of this. I think the configuration and build system > of primary and secondaries should be the same to avoid this kind of > issues. You are not going to convert all existing applications to the DPDK build system. I believe that restricting the build system is irrealistic, it would restrict DPDK secondary only to toy examples. Note that libdpdk.a is tricky to use outside the DPDK build system and require some quirks even for primary applications (see Snort DPDK patches). I would say that DPDK is not very friendly to foreign applications and their build system in general. > Some other issues may happen if the configuration is different, > for instance the size of structures may be different. Impossible, because then libdpdk.a would not work. Remember we are talking of using the exact same libdpdk.a in primary and secondary, and therefore any structure used in libdpdk.a has to match. And the structures used in the app has to match libdpdk.a as well. > There is already a lot of mess due to primary/secondary at many places > in the code, I'm not sure adding more is really desirable. Yes, one solution is obviously to get rid of secondary entirely. Personally, I believe it's pretty close to working, the number of issues I found is manageable. I have a complex application (Snort) working that way without any issues. If DPDK wants to support secondary, you might as well make it work for everybody. We could discuss better solutions to those issues. For example, the tailq subsystem has a better solution. But, I'm not going to waste time if secondary is deprecated. > Regards, > Olivier Regards, Jean
