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