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

[Vladyslav Buslov <Vladyslav.Buslov@harmonicinc.com>]

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