Re: [dpdk-dev] [PATCH v5 4/4] test/hash: add tests for integrated RCU QSBR

[Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>]

<snip>

> >
> > Add functional and performance tests for the integrated RCU QSBR.
> >
> > Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > Signed-off-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > ---
> >  app/test/test_hash.c                   | 390 ++++++++++++++++++++++++-
> >  app/test/test_hash_readwrite_lf_perf.c | 170 ++++++++++-
> >  2 files changed, 556 insertions(+), 4 deletions(-)
> >
> > diff --git a/app/test/test_hash.c b/app/test/test_hash.c index
> > 990a1815f893..22b47b3e7728 100644
> > --- a/app/test/test_hash.c
> > +++ b/app/test/test_hash.c
> > @@ -52,7 +52,7 @@ static uint32_t hashtest_key_lens[] = {0, 2, 4, 5,
> > 6, 7, 8, 10, 11, 15, 16, 21,
> >  	}								\
> >  } while(0)
> >
> > -#define RETURN_IF_ERROR_FBK(cond, str, ...) do {
> > 	\
> > +#define RETURN_IF_ERROR_FBK(cond, str, ...) do {			\
> >  	if (cond) {							\
> >  		printf("ERROR line %d: " str "\n", __LINE__, ##__VA_ARGS__); \
> >  		if (handle) rte_fbk_hash_free(handle);			\
> > @@ -60,6 +60,20 @@ static uint32_t hashtest_key_lens[] = {0, 2, 4, 5,
> > 6, 7, 8, 10, 11, 15, 16, 21,
> >  	}								\
> >  } while(0)
> >
> > +#define RETURN_IF_ERROR_RCU_QSBR(cond, str, ...) do {
> > 	\
> > +	if (cond) {							\
> > +		printf("ERROR line %d: " str "\n", __LINE__, ##__VA_ARGS__);
> > \
> > +		if (rcu_cfg.mode == RTE_HASH_QSBR_MODE_SYNC) {
> > 	\
> > +			writer_done = 1;				\
> > +			/* Wait until reader exited. */			\
> > +			rte_eal_mp_wait_lcore();			\
> > +		}							\
> > +		if (g_handle) rte_hash_free(g_handle);			\
> > +		if (g_qsv) rte_free(g_qsv);				\
> > +		return -1;						\
> > +	}								\
> > +} while(0)
> > +
> >  /* 5-tuple key type */
> >  struct flow_key {
> >  	uint32_t ip_src;
> > @@ -1801,6 +1815,365 @@ test_hash_add_delete_jhash_3word(void)
> >  	return ret;
> >  }
> >
> > +static struct rte_hash *g_handle;
> > +static struct rte_rcu_qsbr *g_qsv;
> > +static volatile uint8_t writer_done;
> > +struct flow_key g_rand_keys[9];
> > +/*
> > + * rte_hash_rcu_qsbr_add positive and negative tests.
> > + *  - Add RCU QSBR variable to Hash
> > + *  - Add another RCU QSBR variable to Hash
> > + *  - Check returns
> > + */
> > +static int
> > +test_hash_rcu_qsbr_add(void)
> > +{
> > +	size_t sz;
> > +	struct rte_rcu_qsbr *qsv2 = NULL;
> > +	int32_t status;
> > +	struct rte_hash_rcu_config rcu_cfg = {0};
> > +
> > +	struct rte_hash_parameters params;
> > +
> > +	printf("\n# Running RCU QSBR add tests\n");
> > +	memcpy(&params, &ut_params, sizeof(params));
> > +	params.name = "test_hash_rcu_qsbr_add";
> > +	params.extra_flag = RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF
> > |
> > +
> > 	RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD;
> > +	g_handle = rte_hash_create(&params);
> > +	RETURN_IF_ERROR_RCU_QSBR(g_handle == NULL, "Hash creation
> > failed");
> > +
> > +	/* Create RCU QSBR variable */
> > +	sz = rte_rcu_qsbr_get_memsize(RTE_MAX_LCORE);
> > +	g_qsv = (struct rte_rcu_qsbr *)rte_zmalloc_socket(NULL, sz,
> > +					RTE_CACHE_LINE_SIZE,
> > SOCKET_ID_ANY);
> > +	RETURN_IF_ERROR_RCU_QSBR(g_qsv == NULL,
> > +				 "RCU QSBR variable creation failed");
> > +
> > +	status = rte_rcu_qsbr_init(g_qsv, RTE_MAX_LCORE);
> [Wang, Yipeng] It reminds me that could we hide this function in the
> rte_cuckoo_hash.c as well?
> I saw most of the rcu related functions are hidden in the hash implementation, it
> would be less confusing if we hide this one as well.
The functionality required for resource reclamation is split into various parts [1]. The functionality of creating the RCU variable, initializing it etc is left to the application. The application has better knowledge of the data plane threads, which ones are using the (hash) library etc. Hence it is better to keep this in the application.

[1] http://doc.dpdk.org/guides/prog_guide/rcu_lib.html#resource-reclamation-framework-for-dpdk

> 
> > +	RETURN_IF_ERROR_RCU_QSBR(status != 0,
> > +				 "RCU QSBR variable initialization failed");
> > +
> > +	rcu_cfg.v = g_qsv;
> > +	/* Invalid QSBR mode */
> > +	rcu_cfg.mode = 2;
> [Wang, Yipeng] Any other way rather than hardcode 2 here? Maybe just a large
> number like 0xff?
> 
> > +	status = rte_hash_rcu_qsbr_add(g_handle, &rcu_cfg);
> > +	RETURN_IF_ERROR_RCU_QSBR(status == 0, "Invalid QSBR mode test
> > +failed");
> > +
> > +	rcu_cfg.mode = RTE_HASH_QSBR_MODE_DQ;
> [Wang, Yipeng] This reminds me that if there is an explanation on the difference
> of the two modes for users to easy to choose?
> 
> > +	/* Attach RCU QSBR to hash table */
> > +	status = rte_hash_rcu_qsbr_add(g_handle, &rcu_cfg);
> > +	RETURN_IF_ERROR_RCU_QSBR(status != 0,
> > +				 "Attach RCU QSBR to hash table failed");
> > +
> > +	/* Create and attach another RCU QSBR to hash table */
> > +	qsv2 = (struct rte_rcu_qsbr *)rte_zmalloc_socket(NULL, sz,
> > +					RTE_CACHE_LINE_SIZE,
> > SOCKET_ID_ANY);
> > +	RETURN_IF_ERROR_RCU_QSBR(qsv2 == NULL,
> > +				 "RCU QSBR variable creation failed");
> > +
> > +	rcu_cfg.v = qsv2;
> > +	rcu_cfg.mode = RTE_HASH_QSBR_MODE_SYNC;
> > +	status = rte_hash_rcu_qsbr_add(g_handle, &rcu_cfg);
> > +	rte_free(qsv2);
> > +	RETURN_IF_ERROR_RCU_QSBR(status == 0,
> > +			"Attach RCU QSBR to hash table succeeded where
> > failure"
> > +			" is expected");
> > +
> > +	rte_hash_free(g_handle);
> > +	rte_free(g_qsv);
> > +
> > +	return 0;
> > +}
> <...>