Re: [dpdk-dev] [PATCH] kni: add support for core_id param in single threaded mode

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

On 9/6/2016 12:25 PM, Vladyslav Buslov wrote:
> Allow binding KNI thread to specific core in single threaded mode
> by setting core_id and force_bind config parameters.

Thanks, patch does exactly what we talked, and as I tested it works fine.

1) There are a few comments, can you please find them inline.

2) Would you mind adding some document related this new feature?
Document path is: doc/guides/prog_guide/kernel_nic_interface.rst

> 
> Signed-off-by: Vladyslav Buslov <vladyslav.buslov@harmonicinc.com>
> ---
>  lib/librte_eal/linuxapp/kni/kni_misc.c | 48 ++++++++++++++++++++++------------
>  1 file changed, 32 insertions(+), 16 deletions(-)
> 
> diff --git a/lib/librte_eal/linuxapp/kni/kni_misc.c b/lib/librte_eal/linuxapp/kni/kni_misc.c
> index 59d15ca..5e7cf21 100644
> --- a/lib/librte_eal/linuxapp/kni/kni_misc.c
> +++ b/lib/librte_eal/linuxapp/kni/kni_misc.c
> @@ -30,6 +30,7 @@
>  #include <linux/pci.h>
>  #include <linux/kthread.h>
>  #include <linux/rwsem.h>
> +#include <linux/mutex.h>
>  #include <linux/nsproxy.h>
>  #include <net/net_namespace.h>
>  #include <net/netns/generic.h>
> @@ -100,6 +101,7 @@ static int kni_net_id;
>  
>  struct kni_net {
>  	unsigned long device_in_use; /* device in use flag */
> +	struct mutex kni_kthread_lock;
>  	struct task_struct *kni_kthread;
>  	struct rw_semaphore kni_list_lock;
>  	struct list_head kni_list_head;
> @@ -123,6 +125,9 @@ static int __net_init kni_init_net(struct net *net)
>  	/* Clear the bit of device in use */
>  	clear_bit(KNI_DEV_IN_USE_BIT_NUM, &knet->device_in_use);
>  
> +	mutex_init(&knet->kni_kthread_lock);
> +	knet->kni_kthread = NULL;
> +
>  	init_rwsem(&knet->kni_list_lock);
>  	INIT_LIST_HEAD(&knet->kni_list_head);
>  
> @@ -139,9 +144,9 @@ static int __net_init kni_init_net(struct net *net)
>  
>  static void __net_exit kni_exit_net(struct net *net)
>  {
> -#ifndef HAVE_SIMPLIFIED_PERNET_OPERATIONS
>  	struct kni_net *knet = net_generic(net, kni_net_id);
> -
> +	mutex_destroy(&knet->kni_kthread_lock);
> +#ifndef HAVE_SIMPLIFIED_PERNET_OPERATIONS
>  	kfree(knet);
>  #endif
>  }
> @@ -236,16 +241,9 @@ kni_open(struct inode *inode, struct file *file)
>  		return -EBUSY;
>  
>  	/* Create kernel thread for single mode */
> -	if (multiple_kthread_on == 0) {
> +	if (multiple_kthread_on == 0)
>  		KNI_PRINT("Single kernel thread for all KNI devices\n");
> -		/* Create kernel thread for RX */
> -		knet->kni_kthread = kthread_run(kni_thread_single, (void *)knet,
> -						"kni_single");
> -		if (IS_ERR(knet->kni_kthread)) {
> -			KNI_ERR("Unable to create kernel threaed\n");
> -			return PTR_ERR(knet->kni_kthread);
> -		}
> -	} else
> +	else
>  		KNI_PRINT("Multiple kernel thread mode enabled\n");

Can move logs to where threads actually created (kni_ioctl_create)

>  
>  	file->private_data = get_net(net);
> @@ -263,9 +261,13 @@ kni_release(struct inode *inode, struct file *file)
>  
>  	/* Stop kernel thread for single mode */
>  	if (multiple_kthread_on == 0) {
> +		mutex_lock(&knet->kni_kthread_lock);
>  		/* Stop kernel thread */
> -		kthread_stop(knet->kni_kthread);
> -		knet->kni_kthread = NULL;
> +		if (knet->kni_kthread != NULL) {
> +			kthread_stop(knet->kni_kthread);
> +			knet->kni_kthread = NULL;
> +		}
> +		mutex_unlock(&knet->kni_kthread_lock);
>  	}
>  
>  	down_write(&knet->kni_list_lock);
> @@ -415,10 +417,9 @@ kni_ioctl_create(struct net *net,
>  	}
>  
>  	/**
> -	 * Check if the cpu core id is valid for binding,
> -	 * for multiple kernel thread mode.
> +	 * Check if the cpu core id is valid for binding.
>  	 */
> -	if (multiple_kthread_on && dev_info.force_bind &&
> +	if (dev_info.force_bind &&
>  				!cpu_online(dev_info.core_id)) {
>  		KNI_ERR("cpu %u is not online\n", dev_info.core_id);
>  		return -EINVAL;
> @@ -581,6 +582,21 @@ kni_ioctl_create(struct net *net,
>  		if (dev_info.force_bind)
>  			kthread_bind(kni->pthread, kni->core_id);
>  		wake_up_process(kni->pthread);
> +	} else {
> +		mutex_lock(&knet->kni_kthread_lock);
> +		if (knet->kni_kthread == NULL) {

----->
> +			knet->kni_kthread = kthread_create(kni_thread_single,
> +									(void *)knet,
> +									"kni_single");

Syntax of this line is not proper, and I am aware above same call has
this syntax J But let's fix here..

> +			if (IS_ERR(knet->kni_kthread)) {
> +				kni_dev_remove(kni);
> +				return -ECANCELED;
> +			}
> +			if (dev_info.force_bind)
> +				kthread_bind(knet->kni_kthread, kni->core_id);
> +			wake_up_process(knet->kni_kthread);
<----- This block is very common for multi and single process, what do
you think extracting this piece as a function, kni_ioctl_create already
longer than it should be.

> +		}
> +		mutex_unlock(&knet->kni_kthread_lock);
>  	}
>  
>  	down_write(&knet->kni_list_lock);
>