From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 30A8E68DD for ; Tue, 6 Sep 2016 16:15:06 +0200 (CEST) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga101.jf.intel.com with ESMTP; 06 Sep 2016 07:15:01 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.30,291,1470726000"; d="scan'208";a="1046306832" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.237.220.81]) ([10.237.220.81]) by orsmga002.jf.intel.com with ESMTP; 06 Sep 2016 07:15:00 -0700 To: Vladyslav Buslov References: <20160906112513.26090-1-vladyslav.buslov@harmonicinc.com> <20160906112513.26090-2-vladyslav.buslov@harmonicinc.com> Cc: dev@dpdk.org From: Ferruh Yigit Message-ID: <072eecca-8108-a45b-91b7-7245dfbe760b@intel.com> Date: Tue, 6 Sep 2016 15:14:59 +0100 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: <20160906112513.26090-2-vladyslav.buslov@harmonicinc.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH] kni: add support for core_id param in single threaded mode X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 06 Sep 2016 14:15:06 -0000 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 > --- > 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 > #include > #include > +#include > #include > #include > #include > @@ -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); >