Re: [dpdk-dev] [PATCH v2] kni: dynamically allocate memory for each KNI

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

On 9/23/2018 8:12 PM, Igor Ryzhov wrote:
> Long time ago preallocation of memory for KNI was introduced in commit
> 0c6bc8e. It was done because of lack of ability to free previously
> allocated memzones, which led to memzone exhaustion. Currently memzones
> can be freed and this patch uses this ability for dynamic KNI memory
> allocation. 

Hi Igor,

Good cleanup, thanks.
+1 to eal_tailq for ctx

A few minor comments below, but they are not significant enough to block the
patch, please let us know if you don't have bandwidth for a new version.

> Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
<...>

> @@ -294,41 +180,52 @@ rte_kni_alloc(struct rte_mempool *pktmbuf_pool,
>  {
>  	int ret;
>  	struct rte_kni_device_info dev_info;
> -	struct rte_kni *ctx;
> -	char intf_name[RTE_KNI_NAMESIZE];
> -	const struct rte_memzone *mz;
> -	struct rte_kni_memzone_slot *slot = NULL;
> +	struct rte_kni *kni;
> +	struct rte_tailq_entry *te = NULL;
> +	struct rte_kni_list *kni_list;
> +
> +	kni_list = RTE_TAILQ_CAST(rte_kni_tailq.head, rte_kni_list);

Can you move this below input validation, no need this assignment if API will
fail because of wrong input.

>  	if (!pktmbuf_pool || !conf || !conf->name[0])
>  		return NULL;
>  
>  	/* Check if KNI subsystem has been initialized */
> -	if (kni_memzone_pool.initialized != 1) {
> +	if (kni_fd < 0) {
>  		RTE_LOG(ERR, KNI, "KNI subsystem has not been initialized. Invoke rte_kni_init() first\n");
>  		return NULL;
>  	}
>  
> -	/* Get an available slot from the pool */
> -	slot = kni_memzone_pool_alloc();
> -	if (!slot) {
> -		RTE_LOG(ERR, KNI, "Cannot allocate more KNI interfaces; increase the number of max_kni_ifaces(current %d) or release unused ones.\n",
> -			kni_memzone_pool.max_ifaces);
> -		return NULL;
> +	rte_rwlock_write_lock(RTE_EAL_TAILQ_RWLOCK);
> +
> +	TAILQ_FOREACH(te, kni_list, next) {
> +		kni = (struct rte_kni *) te->data;
> +		if (strncmp(conf->name, kni->name, RTE_KNI_NAMESIZE) == 0)
> +			break;
>  	}

This is rte_kni_get(), why not reuse it. You can create an version of it without
lock.

like _rte_kni_get() which you can call here.

And
rte_kni_get()
	rte_rwlock_read_lock(RTE_EAL_TAILQ_RWLOCK);
	_rte_kni_get()
	rte_rwlock_read_unlock(RTE_EAL_TAILQ_RWLOCK);

<...>

> +
> +	if (te == NULL) {
> +		goto unlock;
> +	}

No need {} for single line.
One more below.