[dpdk-dev] [PATCH] kni: add module parameter 'bind_to_core'

[dpdk-dev] [PATCH] kni: add module parameter 'bind_to_core'

[Vladyslav Buslov]

Allow binding KNI thread to specific core in single threaded mode.

Signed-off-by: Vladyslav Buslov <vladyslav.buslov@harmonicinc.com>
---
 lib/librte_eal/linuxapp/kni/kni_misc.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/linuxapp/kni/kni_misc.c b/lib/librte_eal/linuxapp/kni/kni_misc.c
index 59d15ca..e98f4a9 100644
--- a/lib/librte_eal/linuxapp/kni/kni_misc.c
+++ b/lib/librte_eal/linuxapp/kni/kni_misc.c
@@ -93,6 +93,7 @@ static char *lo_mode = NULL;
 /* Kernel thread mode */
 static char *kthread_mode = NULL;
 static unsigned multiple_kthread_on = 0;
+static int bind_to_core = -1;
 
 #define KNI_DEV_IN_USE_BIT_NUM 0 /* Bit number for device in use */
 
@@ -239,12 +240,17 @@ kni_open(struct inode *inode, struct file *file)
 	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,
+		knet->kni_kthread = kthread_create(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);
 		}
+		if (bind_to_core >= 0) {
+			KNI_PRINT("Bind main thread to core %d\n", bind_to_core);
+			kthread_bind(knet->kni_kthread, bind_to_core);
+		}
+		wake_up_process(knet->kni_kthread);
 	} else
 		KNI_PRINT("Multiple kernel thread mode enabled\n");
 
@@ -698,3 +704,8 @@ MODULE_PARM_DESC(kthread_mode,
 "    multiple  Multiple kernel thread mode enabled.\n"
 "\n"
 );
+
+module_param(bind_to_core, int, S_IRUGO);
+MODULE_PARM_DESC(bind_to_core,
+"Bind KNI main kernel thread to specific core (default=-1(disabled)):\n"
+);
\ No newline at end of file
-- 
2.8.3

Re: [dpdk-dev] [PATCH] kni: add module parameter 'bind_to_core'

[Ferruh Yigit]

Hi Vladyslav,

On 8/16/2016 7:24 PM, Vladyslav Buslov wrote:
> Allow binding KNI thread to specific core in single threaded mode.

I think this is good idea.
But I am not sure about making this a module parameter, setting core can
be more dynamic.

There is already a kni->core_id field, which is only used for
multiple_kthread mode.
What do you think moving single thread creation into kni_ioctl_create()
and use first kni->core_id to bind the thread? If there is no
kni->core_id set, it will behave as it is now.

> 
> Signed-off-by: Vladyslav Buslov <vladyslav.buslov@harmonicinc.com>
> ---
<...>

Re: [dpdk-dev] [PATCH] kni: add module parameter 'bind_to_core'

[Vladyslav Buslov]

Hi Ferruh,

I agree that your solution is more flexible.
I'll work on moving single thread creation to kni_ioctl_create next week and get back to you with results.

Regards,
Vladyslav

-----Original Message-----
From: Ferruh Yigit [mailto:ferruh.yigit@intel.com] 
Sent: Thursday, August 25, 2016 5:47 PM
To: Vladyslav Buslov
Cc: dev@dpdk.org
Subject: Re: [PATCH] kni: add module parameter 'bind_to_core'

Hi Vladyslav,

On 8/16/2016 7:24 PM, Vladyslav Buslov wrote:
> Allow binding KNI thread to specific core in single threaded mode.

I think this is good idea.
But I am not sure about making this a module parameter, setting core can be more dynamic.

There is already a kni->core_id field, which is only used for multiple_kthread mode.
What do you think moving single thread creation into kni_ioctl_create() and use first kni->core_id to bind the thread? If there is no
kni->core_id set, it will behave as it is now.

> 
> Signed-off-by: Vladyslav Buslov <vladyslav.buslov@harmonicinc.com>
> ---
<...>

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

[Vladyslav Buslov]

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

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");
 
 	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");
+			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);
+		}
+		mutex_unlock(&knet->kni_kthread_lock);
 	}
 
 	down_write(&knet->kni_list_lock);
-- 
2.8.3

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

[Vladyslav Buslov]

Hi Ferruh,

According to your suggestions I implemented dynamic KNI thread affinity
patch in single threaded mode.

It reuses core_id and force_bind config parameters from multi threaded
mode.

Regards,
Vladyslav

Vladyslav Buslov (1):
  kni: add support for core_id param in single threaded mode

 lib/librte_eal/linuxapp/kni/kni_misc.c | 48 ++++++++++++++++++++++------------
 1 file changed, 32 insertions(+), 16 deletions(-)

-- 
2.8.3

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

[Vladyslav Buslov]

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

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");
 
 	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");
+			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);
+		}
+		mutex_unlock(&knet->kni_kthread_lock);
 	}
 
 	down_write(&knet->kni_list_lock);
-- 
2.8.3

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

[Ferruh Yigit]

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

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

[Ferruh Yigit]

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 ..

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

[Ferruh Yigit]

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...

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

[Vladyslav Buslov]

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...

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

[Vladyslav Buslov]

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

v2:
* Fixed formatting.
* Refactored kthread create/bind functionality into separate function.
* Moved thread mode print into kni_init.
* Added short description to KNI Programmer's Gude doc.
* Fixed outdated mbuf processing description in KNI Programmer's Gude doc.

 doc/guides/prog_guide/kernel_nic_interface.rst |  5 +-
 lib/librte_eal/linuxapp/kni/kni_misc.c         | 72 +++++++++++++++++---------
 2 files changed, 51 insertions(+), 26 deletions(-)

diff --git a/doc/guides/prog_guide/kernel_nic_interface.rst b/doc/guides/prog_guide/kernel_nic_interface.rst
index fac1960..0fdc307 100644
--- a/doc/guides/prog_guide/kernel_nic_interface.rst
+++ b/doc/guides/prog_guide/kernel_nic_interface.rst
@@ -102,6 +102,9 @@ Refer to rte_kni_common.h in the DPDK source code for more details.
 
 The physical addresses will be re-mapped into the kernel address space and stored in separate KNI contexts.
 
+The affinity of kernel RX thread (both single and multi-threaded modes) is controlled by force_bind and
+core_id config parameters.
+
 The KNI interfaces can be deleted by a DPDK application dynamically after being created.
 Furthermore, all those KNI interfaces not deleted will be deleted on the release operation
 of the miscellaneous device (when the DPDK application is closed).
@@ -128,7 +131,7 @@ Use Case: Ingress
 On the DPDK RX side, the mbuf is allocated by the PMD in the RX thread context.
 This thread will enqueue the mbuf in the rx_q FIFO.
 The KNI thread will poll all KNI active devices for the rx_q.
-If an mbuf is dequeued, it will be converted to a sk_buff and sent to the net stack via netif_rx().
+If an mbuf is dequeued, it will be converted to a sk_buff and sent to the net stack via netif_rx_ni().
 The dequeued mbuf must be freed, so the same pointer is sent back in the free_q FIFO.
 
 The RX thread, in the same main loop, polls this FIFO and frees the mbuf after dequeuing it.
diff --git a/lib/librte_eal/linuxapp/kni/kni_misc.c b/lib/librte_eal/linuxapp/kni/kni_misc.c
index 5e7cf21..c79f5a8 100644
--- a/lib/librte_eal/linuxapp/kni/kni_misc.c
+++ b/lib/librte_eal/linuxapp/kni/kni_misc.c
@@ -172,6 +172,11 @@ kni_init(void)
 		return -EINVAL;
 	}
 
+	if (multiple_kthread_on == 0)
+		KNI_PRINT("Single kernel thread for all KNI devices\n");
+	else
+		KNI_PRINT("Multiple kernel thread mode enabled\n");
+
 #ifdef HAVE_SIMPLIFIED_PERNET_OPERATIONS
 	rc = register_pernet_subsys(&kni_net_ops);
 #else
@@ -240,12 +245,6 @@ kni_open(struct inode *inode, struct file *file)
 	if (test_and_set_bit(KNI_DEV_IN_USE_BIT_NUM, &knet->device_in_use))
 		return -EBUSY;
 
-	/* Create kernel thread for single mode */
-	if (multiple_kthread_on == 0)
-		KNI_PRINT("Single kernel thread for all KNI devices\n");
-	else
-		KNI_PRINT("Multiple kernel thread mode enabled\n");
-
 	file->private_data = get_net(net);
 	KNI_PRINT("/dev/kni opened\n");
 
@@ -391,6 +390,32 @@ kni_check_param(struct kni_dev *kni, struct rte_kni_device_info *dev)
 	return 0;
 }
 
+__printf(5, 6) static struct task_struct *
+kni_run_thread(int (*threadfn)(void *data),
+	void *data,
+	uint8_t force_bind,
+	unsigned core_id,
+	const char namefmt[], ...)
+{
+	struct task_struct *kni_thread = NULL;
+	char task_comm[TASK_COMM_LEN];
+	va_list args;
+
+	va_start(args, namefmt);
+	vsnprintf(task_comm, sizeof(task_comm), namefmt, args);
+	va_end(args);
+
+	kni_thread = kthread_create(threadfn, data, task_comm);
+	if (IS_ERR(kni_thread))
+		return NULL;
+
+	if (force_bind)
+		kthread_bind(kni_thread, core_id);
+	wake_up_process(kni_thread);
+
+	return kni_thread;
+}
+
 static int
 kni_ioctl_create(struct net *net,
 		unsigned int ioctl_num, unsigned long ioctl_param)
@@ -419,8 +444,7 @@ kni_ioctl_create(struct net *net,
 	/**
 	 * Check if the cpu core id is valid for binding.
 	 */
-	if (dev_info.force_bind &&
-				!cpu_online(dev_info.core_id)) {
+	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;
 	}
@@ -572,31 +596,29 @@ kni_ioctl_create(struct net *net,
 	 * and finally wake it up.
 	 */
 	if (multiple_kthread_on) {
-		kni->pthread = kthread_create(kni_thread_multiple,
-					      (void *)kni,
-					      "kni_%s", kni->name);
-		if (IS_ERR(kni->pthread)) {
+		kni->pthread = kni_run_thread(kni_thread_multiple,
+			(void *)kni,
+			dev_info.force_bind,
+			kni->core_id,
+			"kni_%s", kni->name);
+		if (kni->pthread == NULL) {
 			kni_dev_remove(kni);
 			return -ECANCELED;
 		}
-		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");
-			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);
+			knet->kni_kthread = kni_run_thread(kni_thread_single,
+				(void *)knet,
+				dev_info.force_bind,
+				kni->core_id,
+				"kni_single");
 		}
 		mutex_unlock(&knet->kni_kthread_lock);
+		if (knet->kni_kthread == NULL) {
+			kni_dev_remove(kni);
+			return -ECANCELED;
+		}
 	}
 
 	down_write(&knet->kni_list_lock);
-- 
2.8.3

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

[Vladyslav Buslov]

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>
---
 doc/guides/prog_guide/kernel_nic_interface.rst |   3 +
 lib/librte_eal/linuxapp/kni/kni_misc.c         | 101 ++++++++++++++++---------
 2 files changed, 67 insertions(+), 37 deletions(-)

diff --git a/doc/guides/prog_guide/kernel_nic_interface.rst b/doc/guides/prog_guide/kernel_nic_interface.rst
index fac1960..eb16e2e 100644
--- a/doc/guides/prog_guide/kernel_nic_interface.rst
+++ b/doc/guides/prog_guide/kernel_nic_interface.rst
@@ -102,6 +102,9 @@ Refer to rte_kni_common.h in the DPDK source code for more details.
 
 The physical addresses will be re-mapped into the kernel address space and stored in separate KNI contexts.
 
+The affinity of kernel RX thread (both single and multi-threaded modes) is controlled by force_bind and
+core_id config parameters.
+
 The KNI interfaces can be deleted by a DPDK application dynamically after being created.
 Furthermore, all those KNI interfaces not deleted will be deleted on the release operation
 of the miscellaneous device (when the DPDK application is closed).
diff --git a/lib/librte_eal/linuxapp/kni/kni_misc.c b/lib/librte_eal/linuxapp/kni/kni_misc.c
index 59d15ca..056e13b 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
 }
@@ -167,6 +172,11 @@ kni_init(void)
 		return -EINVAL;
 	}
 
+	if (multiple_kthread_on == 0)
+		KNI_PRINT("Single kernel thread for all KNI devices\n");
+	else
+		KNI_PRINT("Multiple kernel thread mode enabled\n");
+
 #ifdef HAVE_SIMPLIFIED_PERNET_OPERATIONS
 	rc = register_pernet_subsys(&kni_net_ops);
 #else
@@ -235,19 +245,6 @@ kni_open(struct inode *inode, struct file *file)
 	if (test_and_set_bit(KNI_DEV_IN_USE_BIT_NUM, &knet->device_in_use))
 		return -EBUSY;
 
-	/* Create kernel thread for single mode */
-	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
-		KNI_PRINT("Multiple kernel thread mode enabled\n");
-
 	file->private_data = get_net(net);
 	KNI_PRINT("/dev/kni opened\n");
 
@@ -263,9 +260,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);
@@ -390,6 +391,47 @@ kni_check_param(struct kni_dev *kni, struct rte_kni_device_info *dev)
 }
 
 static int
+kni_run_thread(struct kni_net *knet, struct kni_dev *kni, uint8_t force_bind)
+{
+	/**
+	 * Create a new kernel thread for multiple mode, set its core affinity,
+	 * and finally wake it up.
+	 */
+	if (multiple_kthread_on) {
+		kni->pthread = kthread_create(kni_thread_multiple,
+			(void *)kni, "kni_%s", kni->name);
+		if (IS_ERR(kni->pthread)) {
+			kni_dev_remove(kni);
+			return -ECANCELED;
+		}
+
+		if (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");
+			if (IS_ERR(knet->kni_kthread)) {
+				mutex_unlock(&knet->kni_kthread_lock);
+				kni_dev_remove(kni);
+				return -ECANCELED;
+			}
+
+			if (force_bind)
+				kthread_bind(knet->kni_kthread, kni->core_id);
+			wake_up_process(knet->kni_kthread);
+		}
+
+		mutex_unlock(&knet->kni_kthread_lock);
+	}
+
+	return 0;
+}
+
+static int
 kni_ioctl_create(struct net *net,
 		unsigned int ioctl_num, unsigned long ioctl_param)
 {
@@ -415,11 +457,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 &&
-				!cpu_online(dev_info.core_id)) {
+	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;
 	}
@@ -566,22 +606,9 @@ kni_ioctl_create(struct net *net,
 	kni_vhost_init(kni);
 #endif
 
-	/**
-	 * Create a new kernel thread for multiple mode, set its core affinity,
-	 * and finally wake it up.
-	 */
-	if (multiple_kthread_on) {
-		kni->pthread = kthread_create(kni_thread_multiple,
-					      (void *)kni,
-					      "kni_%s", kni->name);
-		if (IS_ERR(kni->pthread)) {
-			kni_dev_remove(kni);
-			return -ECANCELED;
-		}
-		if (dev_info.force_bind)
-			kthread_bind(kni->pthread, kni->core_id);
-		wake_up_process(kni->pthread);
-	}
+	ret = kni_run_thread(knet, kni, dev_info.force_bind);
+	if (ret != 0)
+		return ret;
 
 	down_write(&knet->kni_list_lock);
 	list_add(&kni->list, &knet->kni_list_head);
-- 
2.8.3

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

[Stephen Hemminger]

On Tue, 20 Sep 2016 21:16:37 +0300
Vladyslav Buslov <vladyslav.buslov@harmonicinc.com> wrote:

> @@ -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;
> +

Why not just use kzalloc() here? You would still need to init the mutex
etc, but it would be safer.

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

[Ferruh Yigit]

On 9/20/2016 7:36 PM, Stephen Hemminger wrote:
> On Tue, 20 Sep 2016 21:16:37 +0300
> Vladyslav Buslov <vladyslav.buslov@harmonicinc.com> wrote:
> 
>> @@ -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;
>> +
> 
> Why not just use kzalloc() here? You would still need to init the mutex
> etc, but it would be safer.
> 

Hi Vladyslav,

This is good suggestion, if you send a new version for this update,
please keep my Ack.

Thanks,
ferruh

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

[Vladyslav Buslov]

> On 9/20/2016 7:36 PM, Stephen Hemminger wrote:
> > On Tue, 20 Sep 2016 21:16:37 +0300
> > Vladyslav Buslov <vladyslav.buslov@harmonicinc.com> wrote:
> >
> >> @@ -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;
> >> +
> >
> > Why not just use kzalloc() here? You would still need to init the
> > mutex etc, but it would be safer.
> >
> 
> Hi Vladyslav,
> 
> This is good suggestion, if you send a new version for this update, please
> keep my Ack.
> 
> Thanks,
> ferruh

Hi Ferruh, Stephen,

Could you please elaborate on using kzalloc for this code.
Currently kni_thread_lock is value member of kni_net structure and never explicitly allocated or deallocated.
Kni_kthread is pointer member of kni_net and is implicitly created and destroyed by kthread_run, kthread_stop functions.
Which one of those do you suggest to allocate with kzalloc() and how would it improve safety?

Sorry for not being able to follow your code review but my Kernel programming experience is somewhat limited.

Thanks,
Vladyslav

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

[Ferruh Yigit]

On 9/21/2016 6:15 PM, Vladyslav Buslov wrote:
>> On 9/20/2016 7:36 PM, Stephen Hemminger wrote:
>>> On Tue, 20 Sep 2016 21:16:37 +0300
>>> Vladyslav Buslov <vladyslav.buslov@harmonicinc.com> wrote:
>>>
>>>> @@ -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;
>>>> +
>>>
>>> Why not just use kzalloc() here? You would still need to init the
>>> mutex etc, but it would be safer.
>>>
>>
>> Hi Vladyslav,
>>
>> This is good suggestion, if you send a new version for this update, please
>> keep my Ack.
>>
>> Thanks,
>> ferruh
> 
> Hi Ferruh, Stephen,
> 
> Could you please elaborate on using kzalloc for this code.
> Currently kni_thread_lock is value member of kni_net structure and never explicitly allocated or deallocated.
> Kni_kthread is pointer member of kni_net and is implicitly created and destroyed by kthread_run, kthread_stop functions.
> Which one of those do you suggest to allocate with kzalloc() and how would it improve safety?
> 

Currently:

kni_init_net() {
    knet = kmalloc(..);
    ..
    mutex_init(..);
    knet->kni_thread = NULL;
}

If you allocate knet via kzalloc(), no need to assign NULL to
kni_thread. Also this is safer because any uninitialized knet field will
be zero instead of random value.

This is what I understood at least J

Thanks,
ferruh

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

[Stephen Hemminger]

On Wed, 21 Sep 2016 19:23:47 +0100
Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 9/21/2016 6:15 PM, Vladyslav Buslov wrote:
> >> On 9/20/2016 7:36 PM, Stephen Hemminger wrote:  
> >>> On Tue, 20 Sep 2016 21:16:37 +0300
> >>> Vladyslav Buslov <vladyslav.buslov@harmonicinc.com> wrote:
> >>>  
> >>>> @@ -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;
> >>>> +  
> >>>
> >>> Why not just use kzalloc() here? You would still need to init the
> >>> mutex etc, but it would be safer.
> >>>  
> >>
> >> Hi Vladyslav,
> >>
> >> This is good suggestion, if you send a new version for this update, please
> >> keep my Ack.
> >>
> >> Thanks,
> >> ferruh  
> > 
> > Hi Ferruh, Stephen,
> > 
> > Could you please elaborate on using kzalloc for this code.
> > Currently kni_thread_lock is value member of kni_net structure and never explicitly allocated or deallocated.
> > Kni_kthread is pointer member of kni_net and is implicitly created and destroyed by kthread_run, kthread_stop functions.
> > Which one of those do you suggest to allocate with kzalloc() and how would it improve safety?
> >   
> 
> Currently:
> 
> kni_init_net() {
>     knet = kmalloc(..);
>     ..
>     mutex_init(..);
>     knet->kni_thread = NULL;
> }
> 
> If you allocate knet via kzalloc(), no need to assign NULL to
> kni_thread. Also this is safer because any uninitialized knet field will
> be zero instead of random value.
> 
> This is what I understood at least J

Also any additional fields in knet will be set, avoiding any present
or future uninitialized memory bugs.

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

[Vladyslav Buslov]

> On Wed, 21 Sep 2016 19:23:47 +0100
> Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> 
> > On 9/21/2016 6:15 PM, Vladyslav Buslov wrote:
> > >> On 9/20/2016 7:36 PM, Stephen Hemminger wrote:
> > >>> On Tue, 20 Sep 2016 21:16:37 +0300 Vladyslav Buslov
> > >>> <vladyslav.buslov@harmonicinc.com> wrote:
> > >>>
> > >>>> @@ -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;
> > >>>> +
> > >>>
> > >>> Why not just use kzalloc() here? You would still need to init the
> > >>> mutex etc, but it would be safer.
> > >>>
> > >>
> > >> Hi Vladyslav,
> > >>
> > >> This is good suggestion, if you send a new version for this update,
> > >> please keep my Ack.
> > >>
> > >> Thanks,
> > >> ferruh
> > >
> > > Hi Ferruh, Stephen,
> > >
> > > Could you please elaborate on using kzalloc for this code.
> > > Currently kni_thread_lock is value member of kni_net structure and
> never explicitly allocated or deallocated.
> > > Kni_kthread is pointer member of kni_net and is implicitly created and
> destroyed by kthread_run, kthread_stop functions.
> > > Which one of those do you suggest to allocate with kzalloc() and how
> would it improve safety?
> > >
> >
> > Currently:
> >
> > kni_init_net() {
> >     knet = kmalloc(..);
> >     ..
> >     mutex_init(..);
> >     knet->kni_thread = NULL;
> > }
> >
> > If you allocate knet via kzalloc(), no need to assign NULL to
> > kni_thread. Also this is safer because any uninitialized knet field
> > will be zero instead of random value.
> >
> > This is what I understood at least J
> 
> Also any additional fields in knet will be set, avoiding any present or future
> uninitialized memory bugs.
> 

What about net_generic which is used instead of kmalloc in KNI code for newer kernels?
Quick skim through Linux code indicates that it doesn't zero out memory and people memset it manually.
Just add memset(0) in HAVE_SIMPLIFIED_PERNET_OPERATIONS code?

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

[Ferruh Yigit]

On 9/22/2016 10:29 AM, Vladyslav Buslov wrote:
>> On Wed, 21 Sep 2016 19:23:47 +0100
>> Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>>
>>> On 9/21/2016 6:15 PM, Vladyslav Buslov wrote:
>>>>> On 9/20/2016 7:36 PM, Stephen Hemminger wrote:
>>>>>> On Tue, 20 Sep 2016 21:16:37 +0300 Vladyslav Buslov
>>>>>> <vladyslav.buslov@harmonicinc.com> wrote:
>>>>>>
>>>>>>> @@ -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;
>>>>>>> +
>>>>>>
>>>>>> Why not just use kzalloc() here? You would still need to init the
>>>>>> mutex etc, but it would be safer.
>>>>>>
>>>>>
>>>>> Hi Vladyslav,
>>>>>
>>>>> This is good suggestion, if you send a new version for this update,
>>>>> please keep my Ack.
>>>>>
>>>>> Thanks,
>>>>> ferruh
>>>>
>>>> Hi Ferruh, Stephen,
>>>>
>>>> Could you please elaborate on using kzalloc for this code.
>>>> Currently kni_thread_lock is value member of kni_net structure and
>> never explicitly allocated or deallocated.
>>>> Kni_kthread is pointer member of kni_net and is implicitly created and
>> destroyed by kthread_run, kthread_stop functions.
>>>> Which one of those do you suggest to allocate with kzalloc() and how
>> would it improve safety?
>>>>
>>>
>>> Currently:
>>>
>>> kni_init_net() {
>>>     knet = kmalloc(..);
>>>     ..
>>>     mutex_init(..);
>>>     knet->kni_thread = NULL;
>>> }
>>>
>>> If you allocate knet via kzalloc(), no need to assign NULL to
>>> kni_thread. Also this is safer because any uninitialized knet field
>>> will be zero instead of random value.
>>>
>>> This is what I understood at least J
>>
>> Also any additional fields in knet will be set, avoiding any present or future
>> uninitialized memory bugs.
>>
> 
> What about net_generic which is used instead of kmalloc in KNI code for newer kernels?
> Quick skim through Linux code indicates that it doesn't zero out memory and people memset it manually.

You are right, for that path memset required.

> Just add memset(0) in HAVE_SIMPLIFIED_PERNET_OPERATIONS code?
> 

Yes, I think that is good.

Thanks,
ferruh

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

[Ferruh Yigit]

On 9/20/2016 7:16 PM, Vladyslav Buslov wrote:
> 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>

Thanks Vladyslav!

Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>

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

[Vladyslav Buslov]

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>
Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 doc/guides/prog_guide/kernel_nic_interface.rst |   3 +
 lib/librte_eal/linuxapp/kni/kni_misc.c         | 103 ++++++++++++++++---------
 2 files changed, 68 insertions(+), 38 deletions(-)

diff --git a/doc/guides/prog_guide/kernel_nic_interface.rst b/doc/guides/prog_guide/kernel_nic_interface.rst
index fac1960..eb16e2e 100644
--- a/doc/guides/prog_guide/kernel_nic_interface.rst
+++ b/doc/guides/prog_guide/kernel_nic_interface.rst
@@ -102,6 +102,9 @@ Refer to rte_kni_common.h in the DPDK source code for more details.
 
 The physical addresses will be re-mapped into the kernel address space and stored in separate KNI contexts.
 
+The affinity of kernel RX thread (both single and multi-threaded modes) is controlled by force_bind and
+core_id config parameters.
+
 The KNI interfaces can be deleted by a DPDK application dynamically after being created.
 Furthermore, all those KNI interfaces not deleted will be deleted on the release operation
 of the miscellaneous device (when the DPDK application is closed).
diff --git a/lib/librte_eal/linuxapp/kni/kni_misc.c b/lib/librte_eal/linuxapp/kni/kni_misc.c
index 59d15ca..7ef2d3e 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;
@@ -109,11 +111,12 @@ static int __net_init kni_init_net(struct net *net)
 {
 #ifdef HAVE_SIMPLIFIED_PERNET_OPERATIONS
 	struct kni_net *knet = net_generic(net, kni_net_id);
+	memset(knet, 0, sizeof(*knet));
 #else
 	struct kni_net *knet;
 	int ret;
 
-	knet = kmalloc(sizeof(struct kni_net), GFP_KERNEL);
+	knet = kzalloc(sizeof(struct kni_net), GFP_KERNEL);
 	if (!knet) {
 		ret = -ENOMEM;
 		return ret;
@@ -123,6 +126,8 @@ 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);
+
 	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
 }
@@ -167,6 +172,11 @@ kni_init(void)
 		return -EINVAL;
 	}
 
+	if (multiple_kthread_on == 0)
+		KNI_PRINT("Single kernel thread for all KNI devices\n");
+	else
+		KNI_PRINT("Multiple kernel thread mode enabled\n");
+
 #ifdef HAVE_SIMPLIFIED_PERNET_OPERATIONS
 	rc = register_pernet_subsys(&kni_net_ops);
 #else
@@ -235,19 +245,6 @@ kni_open(struct inode *inode, struct file *file)
 	if (test_and_set_bit(KNI_DEV_IN_USE_BIT_NUM, &knet->device_in_use))
 		return -EBUSY;
 
-	/* Create kernel thread for single mode */
-	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
-		KNI_PRINT("Multiple kernel thread mode enabled\n");
-
 	file->private_data = get_net(net);
 	KNI_PRINT("/dev/kni opened\n");
 
@@ -263,9 +260,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);
@@ -390,6 +391,47 @@ kni_check_param(struct kni_dev *kni, struct rte_kni_device_info *dev)
 }
 
 static int
+kni_run_thread(struct kni_net *knet, struct kni_dev *kni, uint8_t force_bind)
+{
+	/**
+	 * Create a new kernel thread for multiple mode, set its core affinity,
+	 * and finally wake it up.
+	 */
+	if (multiple_kthread_on) {
+		kni->pthread = kthread_create(kni_thread_multiple,
+			(void *)kni, "kni_%s", kni->name);
+		if (IS_ERR(kni->pthread)) {
+			kni_dev_remove(kni);
+			return -ECANCELED;
+		}
+
+		if (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");
+			if (IS_ERR(knet->kni_kthread)) {
+				mutex_unlock(&knet->kni_kthread_lock);
+				kni_dev_remove(kni);
+				return -ECANCELED;
+			}
+
+			if (force_bind)
+				kthread_bind(knet->kni_kthread, kni->core_id);
+			wake_up_process(knet->kni_kthread);
+		}
+
+		mutex_unlock(&knet->kni_kthread_lock);
+	}
+
+	return 0;
+}
+
+static int
 kni_ioctl_create(struct net *net,
 		unsigned int ioctl_num, unsigned long ioctl_param)
 {
@@ -415,11 +457,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 &&
-				!cpu_online(dev_info.core_id)) {
+	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;
 	}
@@ -566,22 +606,9 @@ kni_ioctl_create(struct net *net,
 	kni_vhost_init(kni);
 #endif
 
-	/**
-	 * Create a new kernel thread for multiple mode, set its core affinity,
-	 * and finally wake it up.
-	 */
-	if (multiple_kthread_on) {
-		kni->pthread = kthread_create(kni_thread_multiple,
-					      (void *)kni,
-					      "kni_%s", kni->name);
-		if (IS_ERR(kni->pthread)) {
-			kni_dev_remove(kni);
-			return -ECANCELED;
-		}
-		if (dev_info.force_bind)
-			kthread_bind(kni->pthread, kni->core_id);
-		wake_up_process(kni->pthread);
-	}
+	ret = kni_run_thread(knet, kni, dev_info.force_bind);
+	if (ret != 0)
+		return ret;
 
 	down_write(&knet->kni_list_lock);
 	list_add(&kni->list, &knet->kni_list_head);
-- 
2.8.3

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

[Ferruh Yigit]

On 9/24/2016 2:13 PM, Vladyslav Buslov wrote:
> 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>
> Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>

Tested-by: Ferruh Yigit <ferruh.yigit@intel.com>

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

[Thomas Monjalon]

> > 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>
> > Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 
> Tested-by: Ferruh Yigit <ferruh.yigit@intel.com>

Applied, thanks