[dpdk-dev] Adding RTE_KNI_PREEMPT configuration option

  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>
---
 config/common_linuxapp                 |    1 +
 lib/librte_eal/linuxapp/kni/kni_misc.c |    4 ++++
 2 files changed, 5 insertions(+)
  

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>
> ---
>   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;
  

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
> 
Idea is good, any chance it could be added as a run-time rather than
compile-time option?

/Bruce

> 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>
> >---
> >  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;
>
  

On 10/02/15 13:02, Bruce Richardson wrote:
> 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
>>
> Idea is good, any chance it could be added as a run-time rather than
> compile-time option?

It is also an option. I wasn't really thinking someone would want to 
change this behaviour at runtime. If we think it is worth, I can have a 
closer look on it. Any other opinions on this?

If we would go for a runtime flag, we would either have to add a config 
parameter to rte_kni_init() or add a specific call to turn on/off this 
knob, depending on whether it is sufficient to change this behaviour at 
bootstrapping time, or we want to also change it during 'operation'. In 
either case we would require some communication, probably via ioctl(), 
from user-space to kernel space.

Thanks
Marc

>
> /Bruce
>
>> 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>
>>> ---
>>>   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;
  

On Tue, Feb 10, 2015 at 01:21:55PM +0100, Marc Sune wrote:
> 
> On 10/02/15 13:02, Bruce Richardson wrote:
> >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
> >>
> >Idea is good, any chance it could be added as a run-time rather than
> >compile-time option?
> 
> It is also an option. I wasn't really thinking someone would want to change
> this behaviour at runtime. If we think it is worth, I can have a closer look
> on it. Any other opinions on this?
> 
> If we would go for a runtime flag, we would either have to add a config
> parameter to rte_kni_init() or add a specific call to turn on/off this knob,
> depending on whether it is sufficient to change this behaviour at
> bootstrapping time, or we want to also change it during 'operation'. In
> either case we would require some communication, probably via ioctl(), from
> user-space to kernel space.
> 
> Thanks
> Marc
>
Yes, I can't see it needing to be changed much at runtime, however, we may need
to take account of those who are using precompiled DPDK libs. For those using
prebuilt DPDK libs, they won't have any ability to modify compile-time values.

However, that being said, I have no particular objection to taking this change
in as-is for now. It only adds something, rather than taking it away.

Regards,
/Bruce
  

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;
>
  

On 10/02/15 14:22, Bruce Richardson wrote:
> On Tue, Feb 10, 2015 at 01:21:55PM +0100, Marc Sune wrote:
>> On 10/02/15 13:02, Bruce Richardson wrote:
>>> 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
>>>>
>>> Idea is good, any chance it could be added as a run-time rather than
>>> compile-time option?
>> It is also an option. I wasn't really thinking someone would want to change
>> this behaviour at runtime. If we think it is worth, I can have a closer look
>> on it. Any other opinions on this?
>>
>> If we would go for a runtime flag, we would either have to add a config
>> parameter to rte_kni_init() or add a specific call to turn on/off this knob,
>> depending on whether it is sufficient to change this behaviour at
>> bootstrapping time, or we want to also change it during 'operation'. In
>> either case we would require some communication, probably via ioctl(), from
>> user-space to kernel space.
>>
>> Thanks
>> Marc
>>
> Yes, I can't see it needing to be changed much at runtime, however, we may need
> to take account of those who are using precompiled DPDK libs. For those using
> prebuilt DPDK libs, they won't have any ability to modify compile-time values.

I see the point. So it should be enough to improve rte_kni_init() with 
an extra argument, but this means add some additional ioctl() calls, as 
far as I see.

>
> However, that being said, I have no particular objection to taking this change
> in as-is for now. It only adds something, rather than taking it away.

Yes, we can improve it in the future, I have no time right now.

Thanks
marc

>
> Regards,
> /Bruce
>
>
  

> -----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.

Anyway, it can be done now or later.

Regards,
Helin
  

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
>
> Regards,
> Helin
>
  

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. :-)

/Bruce
  

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
>