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

  Allow binding KNI thread to specific core in single threaded mode
by setting core_id and force_bind config parameters.

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(-)
  

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

On 9/6/2016 3:14 PM, Ferruh Yigit wrote:
> 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>
>> ---

...
 	 */
>> -	if (multiple_kthread_on && dev_info.force_bind &&
>> +	if (dev_info.force_bind &&
>>  				!cpu_online(dev_info.core_id)) {

minor thing, these two lines can join into single line ..
  

On 9/6/2016 3:14 PM, Ferruh Yigit wrote:
> 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)

Thinking twice, moving logs to kni_ioctl_create() can be too verbose
since it has been called multiple times, but we need this log only once.

Keepin in open() cb isn't good option, what do you think moving into
kni_init(), after kni_parse_ktread_mode? I think fits here better since
this is a module param...
  

Ferruh,

Thanks for suggestions.
I'll try to provide new patch this week.

Regards,
Vladyslav


-----Original Message-----
From: Ferruh Yigit [mailto:ferruh.yigit@intel.com] 
Sent: Tuesday, September 06, 2016 5:31 PM
To: Vladyslav Buslov
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] kni: add support for core_id param in single threaded mode

On 9/6/2016 3:14 PM, Ferruh Yigit wrote:
> 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)

Thinking twice, moving logs to kni_ioctl_create() can be too verbose since it has been called multiple times, but we need this log only once.

Keepin in open() cb isn't good option, what do you think moving into kni_init(), after kni_parse_ktread_mode? I think fits here better since this is a module param...