[dpdk-dev] [PATCH v2 2/2] kni: add support for core_id param in single threaded mode
Hi Ferruh,
Thanks for reviewing my code.
Regarding creating kthread within locked mutex section: It is executed in
context of ioctl_unlocked so I assume we may have race condition otherwise if
multiple threads in same task try to create KNI interface simultaneously.
My kernel programming knowledge is limited so correct me if I'm wrong.
Regards,
Vlad
-Original Message-
From: Ferruh Yigit [mailto:ferruh.yi...@intel.com]
Sent: Monday, September 12, 2016 8:08 PM
To: Vladyslav Buslov; dev at dpdk.org
Subject: Re: [dpdk-dev] [PATCH v2 2/2] kni: add support for core_id param in
single threaded mode
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(_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, >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[], ...
[dpdk-dev] [PATCH v2 2/2] kni: add support for core_id param in single threaded mode
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(_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, >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
[dpdk-dev] [PATCH v2 2/2] kni: add support for core_id param in single threaded mode
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().
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(_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, >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,
+
