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

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 11/10/2016 11:25 PM, jt at labs.hpe.com (Jean Tourrilhes) wrote:
> 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 <jt at labs.hpe.com>

Hi Jean,

There is no comment on the patch for a long time, more than two years, updating
patch status as rejected, if it is still relevant please let us know.

Sorry for any inconvenience caused.

For record, patch: https://patches.dpdk.org/patch/17000/

> ---
>  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;
>  }
>  
>