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

[Ferruh Yigit <ferruh.yigit@intel.com>]

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