From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id CECA45597 for ; Mon, 12 Sep 2016 19:08:22 +0200 (CEST) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga103.jf.intel.com with ESMTP; 12 Sep 2016 10:08:21 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.30,323,1470726000"; d="scan'208";a="1028837454" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.237.220.189]) ([10.237.220.189]) by orsmga001.jf.intel.com with ESMTP; 12 Sep 2016 10:08:12 -0700 To: Vladyslav Buslov , dev@dpdk.org References: <20160906112513.26090-2-vladyslav.buslov@harmonicinc.com> <20160910135016.6468-1-vladyslav.buslov@harmonicinc.com> <20160910135016.6468-2-vladyslav.buslov@harmonicinc.com> From: Ferruh Yigit Message-ID: <60f8b06a-4122-d0ef-e66c-267ee75e6c98@intel.com> Date: Mon, 12 Sep 2016 18:08:05 +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: <20160910135016.6468-2-vladyslav.buslov@harmonicinc.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v2 2/2] 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: Mon, 12 Sep 2016 17:08:23 -0000 On 9/10/2016 2:50 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 > --- > > 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(). Although this is small and correct modification, unrelated to this patch. It is good to remove from this patch, and feel free to create a separate patch if you want. > 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"); > + Instead of fixing these in a second patch, why not do the correct one in first patch? Or I think it is better to have a single patch instead of two. What about squashing second patch into first? > #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[], ...) Syntax should be updated. > +{ > + 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); What about just using a "char *" and make things simpler, instead of variable length parameters. Name can be kni_%s, for multi_thread %s is kni->name, for single_thread %s is "single". > + > + 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)) { Same comment as above, lets have a single patch. > 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); What about passing "kni" as parameter, this saves force_bind and core_id values: static struct task_struct * kni_run_thread(int (*threadfn)(void *data), void *data, struct kni_dev *kni, char *thread_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"); Syntax should be updated, not need to have a new line per param. > } > mutex_unlock(&knet->kni_kthread_lock); > + if (knet->kni_kthread == NULL) { Does this needs to be within kthread_lock? > + kni_dev_remove(kni); > + return -ECANCELED; > + } > } > > down_write(&knet->kni_list_lock); >