[dpdk-dev] [PATCH] mempool: Add sanity check when secondary link in less mempools than primary

[dpdk-dev] [PATCH] mempool: Add sanity check when secondary link in less mempools than primary

[Jean Tourrilhes]

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 <jt@labs.hpe.com>
---
 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;
 }
 

Re: [dpdk-dev] [PATCH] mempool: Add sanity check when secondary link in less mempools than primary

[Olivier Matz]

Hi Jean,

On 10/12/2016 10:04 PM, Jean Tourrilhes wrote:
> 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 <jt@labs.hpe.com>
> ---
>  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;
>  }
>  
> 

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. Some other issues may happen if the configuration is different,
for instance the size of structures may be different.

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.

Regards,
Olivier

Re: [dpdk-dev] [PATCH] mempool: Add sanity check when secondary link in less mempools than primary

[Jean Tourrilhes]

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