Re: [dpdk-dev] [PATCH] Adding RTE_KNI_PREEMPT configuration option

[Marc Sune <marc.sune@bisdn.de>]

On 11/02/15 15:27, Bruce Richardson wrote:
> On Wed, Feb 11, 2015 at 01:26:41PM +0100, Marc Sune wrote:
>> On 11/02/15 02:54, Zhang, Helin wrote:
>>>> -----Original Message-----
>>>> From: Richardson, Bruce
>>>> Sent: Tuesday, February 10, 2015 9:24 PM
>>>> To: Marc Sune
>>>> Cc: dev@dpdk.org; Zhang, Helin
>>>> Subject: Re: [dpdk-dev] [PATCH] Adding RTE_KNI_PREEMPT configuration
>>>> option
>>>>
>>>> On Tue, Feb 10, 2015 at 12:59:29PM +0100, Marc Sune wrote:
>>>>> This patch of Nov 2014 hasn't been yet ACKed/NACKed. Could someone
>>>>> please give some quick feedback?
>>>>>
>>>>> Thanks
>>>>> marc
>>>>>
>>>>> On 07/11/14 12:00, Marc Sune wrote:
>>>>>> This patch introduces CONFIG_RTE_KNI_PREEMPT flag. When set to 'no',
>>>>>> KNI kernel thread(s) do not call schedule_timeout_interruptible(),
>>>>>> which improves overall KNI performance at the expense of CPU cycles
>>>> (polling).
>>>>>> Default values is 'yes', maintaining the same behaviour as of now.
>>>>>>
>>>>>> Signed-off-by: Marc Sune <marc.sune@bisdn.de>
>>>> Although a better option would be to have a runtime setting, this is still an
>>>> improvement over what we have.
>>>>
>>>> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
>>>>
>>>>>> ---
>>>>>>   config/common_linuxapp                 |    1 +
>>>>>>   lib/librte_eal/linuxapp/kni/kni_misc.c |    4 ++++
>>>>>>   2 files changed, 5 insertions(+)
>>>>>>
>>>>>> diff --git a/config/common_linuxapp b/config/common_linuxapp index
>>>>>> 57b61c9..24b529d 100644
>>>>>> --- a/config/common_linuxapp
>>>>>> +++ b/config/common_linuxapp
>>>>>> @@ -380,6 +380,7 @@ CONFIG_RTE_LIBRTE_PIPELINE=y
>>>>>>   # Compile librte_kni
>>>>>>   #
>>>>>>   CONFIG_RTE_LIBRTE_KNI=y
>>>>>> +CONFIG_RTE_KNI_PREEMPT=y
>>>>>>   CONFIG_RTE_KNI_KO_DEBUG=n
>>>>>>   CONFIG_RTE_KNI_VHOST=n
>>>>>>   CONFIG_RTE_KNI_VHOST_MAX_CACHE_SIZE=1024
>>>>>> diff --git a/lib/librte_eal/linuxapp/kni/kni_misc.c
>>>>>> b/lib/librte_eal/linuxapp/kni/kni_misc.c
>>>>>> index ba77776..e7e6c27 100644
>>>>>> --- a/lib/librte_eal/linuxapp/kni/kni_misc.c
>>>>>> +++ b/lib/librte_eal/linuxapp/kni/kni_misc.c
>>>>>> @@ -229,9 +229,11 @@ kni_thread_single(void *unused)
>>>>>>   			}
>>>>>>   		}
>>>>>>   		up_read(&kni_list_lock);
>>>>>> +#ifdef RTE_KNI_PREEMPT
>>>>>>   		/* reschedule out for a while */
>>>>>>   		schedule_timeout_interruptible(usecs_to_jiffies( \
>>>>>>   				KNI_KTHREAD_RESCHEDULE_INTERVAL));
>>>>>> +#endif
>>>>>>   	}
>>>>>>   	return 0;
>>>>>> @@ -252,8 +254,10 @@ kni_thread_multiple(void *param)
>>>>>>   #endif
>>>>>>   			kni_net_poll_resp(dev);
>>>>>>   		}
>>>>>> +#ifdef RTE_KNI_PREEMPT
>>>>>>   		schedule_timeout_interruptible(usecs_to_jiffies( \
>>>>>>   				KNI_KTHREAD_RESCHEDULE_INTERVAL));
>>>>>> +#endif
>>>>>>   	}
>>>>>>   	return 0;
>>> As Bruce indicated, it would be better to do that at runtime, we can
>>> add a config in struct rte_kni_conf, which will be copied to
>>> struct rte_kni_device_info, then kernel space will know the configuration.
>>> This way, we can enable/disable PREEMPT during KNI instance allocation time.
>> As I said before, I see the point on having it at runtime and I agree.
>>
>> However, rte_kni_device_info is a struct that is per interface, not for the
>> entire KNI subsystem, so it is kind of abusing to add a flag there when only
>> will be used once, at bootstrap. To do it "properly" we should create a new
>> ioctl() call IMHO.
>>
>>> Anyway, it can be done now or later.
>> So, do we integrate the current patch, as acked by Bruce before?
>>
>> Marc
> I'd like to see it go in, as a step forward, rather than not having it at all
> because it's not quite perfect.
>
> One suggestion might be to have the compile time setting be
> "CONFIG_RTE_KNI_PREEMPT_DEFAULT" rather than just "CONFIG_RTE_KNI_PREEMPT". This
> would mean that we would not need to remove the setting later if we do add a
> runtime option, as the setting only specified the default value rather than
> the final value. :-)

Ok. I can do this small change and sent v2 if we plan to integrate it 
and no one else wants to work on the runtime approach. I have no time 
right now to do a runtime setting patch.

Marc
>
> /Bruce
>