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

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

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.yigit@intel.com] 
Sent: Monday, September 12, 2016 8:08 PM
To: Vladyslav Buslov; dev@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 <vladyslav.buslov@harmonicinc.com>
> ---
> 
> 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);
>