[dpdk-dev] kni: add support for core_id param in single threaded mode
Commit Message
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>
---
doc/guides/prog_guide/kernel_nic_interface.rst | 3 +
lib/librte_eal/linuxapp/kni/kni_misc.c | 101 ++++++++++++++++---------
2 files changed, 67 insertions(+), 37 deletions(-)
Comments
On Tue, 20 Sep 2016 21:16:37 +0300
Vladyslav Buslov <vladyslav.buslov@harmonicinc.com> wrote:
> @@ -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;
> +
Why not just use kzalloc() here? You would still need to init the mutex
etc, but it would be safer.
On 9/20/2016 7:16 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>
Thanks Vladyslav!
Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>
On 9/20/2016 7:36 PM, Stephen Hemminger wrote:
> On Tue, 20 Sep 2016 21:16:37 +0300
> Vladyslav Buslov <vladyslav.buslov@harmonicinc.com> wrote:
>
>> @@ -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;
>> +
>
> Why not just use kzalloc() here? You would still need to init the mutex
> etc, but it would be safer.
>
Hi Vladyslav,
This is good suggestion, if you send a new version for this update,
please keep my Ack.
Thanks,
ferruh
> On 9/20/2016 7:36 PM, Stephen Hemminger wrote:
> > On Tue, 20 Sep 2016 21:16:37 +0300
> > Vladyslav Buslov <vladyslav.buslov@harmonicinc.com> wrote:
> >
> >> @@ -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;
> >> +
> >
> > Why not just use kzalloc() here? You would still need to init the
> > mutex etc, but it would be safer.
> >
>
> Hi Vladyslav,
>
> This is good suggestion, if you send a new version for this update, please
> keep my Ack.
>
> Thanks,
> ferruh
Hi Ferruh, Stephen,
Could you please elaborate on using kzalloc for this code.
Currently kni_thread_lock is value member of kni_net structure and never explicitly allocated or deallocated.
Kni_kthread is pointer member of kni_net and is implicitly created and destroyed by kthread_run, kthread_stop functions.
Which one of those do you suggest to allocate with kzalloc() and how would it improve safety?
Sorry for not being able to follow your code review but my Kernel programming experience is somewhat limited.
Thanks,
Vladyslav
On 9/21/2016 6:15 PM, Vladyslav Buslov wrote:
>> On 9/20/2016 7:36 PM, Stephen Hemminger wrote:
>>> On Tue, 20 Sep 2016 21:16:37 +0300
>>> Vladyslav Buslov <vladyslav.buslov@harmonicinc.com> wrote:
>>>
>>>> @@ -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;
>>>> +
>>>
>>> Why not just use kzalloc() here? You would still need to init the
>>> mutex etc, but it would be safer.
>>>
>>
>> Hi Vladyslav,
>>
>> This is good suggestion, if you send a new version for this update, please
>> keep my Ack.
>>
>> Thanks,
>> ferruh
>
> Hi Ferruh, Stephen,
>
> Could you please elaborate on using kzalloc for this code.
> Currently kni_thread_lock is value member of kni_net structure and never explicitly allocated or deallocated.
> Kni_kthread is pointer member of kni_net and is implicitly created and destroyed by kthread_run, kthread_stop functions.
> Which one of those do you suggest to allocate with kzalloc() and how would it improve safety?
>
Currently:
kni_init_net() {
knet = kmalloc(..);
..
mutex_init(..);
knet->kni_thread = NULL;
}
If you allocate knet via kzalloc(), no need to assign NULL to
kni_thread. Also this is safer because any uninitialized knet field will
be zero instead of random value.
This is what I understood at least J
Thanks,
ferruh
On Wed, 21 Sep 2016 19:23:47 +0100
Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> On 9/21/2016 6:15 PM, Vladyslav Buslov wrote:
> >> On 9/20/2016 7:36 PM, Stephen Hemminger wrote:
> >>> On Tue, 20 Sep 2016 21:16:37 +0300
> >>> Vladyslav Buslov <vladyslav.buslov@harmonicinc.com> wrote:
> >>>
> >>>> @@ -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;
> >>>> +
> >>>
> >>> Why not just use kzalloc() here? You would still need to init the
> >>> mutex etc, but it would be safer.
> >>>
> >>
> >> Hi Vladyslav,
> >>
> >> This is good suggestion, if you send a new version for this update, please
> >> keep my Ack.
> >>
> >> Thanks,
> >> ferruh
> >
> > Hi Ferruh, Stephen,
> >
> > Could you please elaborate on using kzalloc for this code.
> > Currently kni_thread_lock is value member of kni_net structure and never explicitly allocated or deallocated.
> > Kni_kthread is pointer member of kni_net and is implicitly created and destroyed by kthread_run, kthread_stop functions.
> > Which one of those do you suggest to allocate with kzalloc() and how would it improve safety?
> >
>
> Currently:
>
> kni_init_net() {
> knet = kmalloc(..);
> ..
> mutex_init(..);
> knet->kni_thread = NULL;
> }
>
> If you allocate knet via kzalloc(), no need to assign NULL to
> kni_thread. Also this is safer because any uninitialized knet field will
> be zero instead of random value.
>
> This is what I understood at least J
Also any additional fields in knet will be set, avoiding any present
or future uninitialized memory bugs.
> On Wed, 21 Sep 2016 19:23:47 +0100
> Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>
> > On 9/21/2016 6:15 PM, Vladyslav Buslov wrote:
> > >> On 9/20/2016 7:36 PM, Stephen Hemminger wrote:
> > >>> On Tue, 20 Sep 2016 21:16:37 +0300 Vladyslav Buslov
> > >>> <vladyslav.buslov@harmonicinc.com> wrote:
> > >>>
> > >>>> @@ -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;
> > >>>> +
> > >>>
> > >>> Why not just use kzalloc() here? You would still need to init the
> > >>> mutex etc, but it would be safer.
> > >>>
> > >>
> > >> Hi Vladyslav,
> > >>
> > >> This is good suggestion, if you send a new version for this update,
> > >> please keep my Ack.
> > >>
> > >> Thanks,
> > >> ferruh
> > >
> > > Hi Ferruh, Stephen,
> > >
> > > Could you please elaborate on using kzalloc for this code.
> > > Currently kni_thread_lock is value member of kni_net structure and
> never explicitly allocated or deallocated.
> > > Kni_kthread is pointer member of kni_net and is implicitly created and
> destroyed by kthread_run, kthread_stop functions.
> > > Which one of those do you suggest to allocate with kzalloc() and how
> would it improve safety?
> > >
> >
> > Currently:
> >
> > kni_init_net() {
> > knet = kmalloc(..);
> > ..
> > mutex_init(..);
> > knet->kni_thread = NULL;
> > }
> >
> > If you allocate knet via kzalloc(), no need to assign NULL to
> > kni_thread. Also this is safer because any uninitialized knet field
> > will be zero instead of random value.
> >
> > This is what I understood at least J
>
> Also any additional fields in knet will be set, avoiding any present or future
> uninitialized memory bugs.
>
What about net_generic which is used instead of kmalloc in KNI code for newer kernels?
Quick skim through Linux code indicates that it doesn't zero out memory and people memset it manually.
Just add memset(0) in HAVE_SIMPLIFIED_PERNET_OPERATIONS code?
On 9/22/2016 10:29 AM, Vladyslav Buslov wrote:
>> On Wed, 21 Sep 2016 19:23:47 +0100
>> Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>>
>>> On 9/21/2016 6:15 PM, Vladyslav Buslov wrote:
>>>>> On 9/20/2016 7:36 PM, Stephen Hemminger wrote:
>>>>>> On Tue, 20 Sep 2016 21:16:37 +0300 Vladyslav Buslov
>>>>>> <vladyslav.buslov@harmonicinc.com> wrote:
>>>>>>
>>>>>>> @@ -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;
>>>>>>> +
>>>>>>
>>>>>> Why not just use kzalloc() here? You would still need to init the
>>>>>> mutex etc, but it would be safer.
>>>>>>
>>>>>
>>>>> Hi Vladyslav,
>>>>>
>>>>> This is good suggestion, if you send a new version for this update,
>>>>> please keep my Ack.
>>>>>
>>>>> Thanks,
>>>>> ferruh
>>>>
>>>> Hi Ferruh, Stephen,
>>>>
>>>> Could you please elaborate on using kzalloc for this code.
>>>> Currently kni_thread_lock is value member of kni_net structure and
>> never explicitly allocated or deallocated.
>>>> Kni_kthread is pointer member of kni_net and is implicitly created and
>> destroyed by kthread_run, kthread_stop functions.
>>>> Which one of those do you suggest to allocate with kzalloc() and how
>> would it improve safety?
>>>>
>>>
>>> Currently:
>>>
>>> kni_init_net() {
>>> knet = kmalloc(..);
>>> ..
>>> mutex_init(..);
>>> knet->kni_thread = NULL;
>>> }
>>>
>>> If you allocate knet via kzalloc(), no need to assign NULL to
>>> kni_thread. Also this is safer because any uninitialized knet field
>>> will be zero instead of random value.
>>>
>>> This is what I understood at least J
>>
>> Also any additional fields in knet will be set, avoiding any present or future
>> uninitialized memory bugs.
>>
>
> What about net_generic which is used instead of kmalloc in KNI code for newer kernels?
> Quick skim through Linux code indicates that it doesn't zero out memory and people memset it manually.
You are right, for that path memset required.
> Just add memset(0) in HAVE_SIMPLIFIED_PERNET_OPERATIONS code?
>
Yes, I think that is good.
Thanks,
ferruh
@@ -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).
@@ -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
}
@@ -167,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(&kni_net_ops);
#else
@@ -235,19 +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");
- /* 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
- KNI_PRINT("Multiple kernel thread mode enabled\n");
-
file->private_data = get_net(net);
KNI_PRINT("/dev/kni opened\n");
@@ -263,9 +260,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);
@@ -390,6 +391,47 @@ kni_check_param(struct kni_dev *kni, struct rte_kni_device_info *dev)
}
static int
+kni_run_thread(struct kni_net *knet, struct kni_dev *kni, uint8_t force_bind)
+{
+ /**
+ * Create a new kernel thread for multiple mode, set its core affinity,
+ * 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_dev_remove(kni);
+ return -ECANCELED;
+ }
+
+ if (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)) {
+ mutex_unlock(&knet->kni_kthread_lock);
+ kni_dev_remove(kni);
+ return -ECANCELED;
+ }
+
+ if (force_bind)
+ kthread_bind(knet->kni_kthread, kni->core_id);
+ wake_up_process(knet->kni_kthread);
+ }
+
+ mutex_unlock(&knet->kni_kthread_lock);
+ }
+
+ return 0;
+}
+
+static int
kni_ioctl_create(struct net *net,
unsigned int ioctl_num, unsigned long ioctl_param)
{
@@ -415,11 +457,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 &&
- !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;
}
@@ -566,22 +606,9 @@ kni_ioctl_create(struct net *net,
kni_vhost_init(kni);
#endif
- /**
- * Create a new kernel thread for multiple mode, set its core affinity,
- * 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_dev_remove(kni);
- return -ECANCELED;
- }
- if (dev_info.force_bind)
- kthread_bind(kni->pthread, kni->core_id);
- wake_up_process(kni->pthread);
- }
+ ret = kni_run_thread(knet, kni, dev_info.force_bind);
+ if (ret != 0)
+ return ret;
down_write(&knet->kni_list_lock);
list_add(&kni->list, &knet->kni_list_head);