Re: [Xen-devel] [PATCH 1/6] xenbus: prepare data structures and parameter for xenwatch multithreading

[Juergen Gross]

On 14/09/18 15:57, Dongli Zhang wrote:
> Hi Juergen and Paul,
> 
> On 09/14/2018 04:32 PM, Juergen Gross wrote:
>> On 14/09/18 09:34, Dongli Zhang wrote:
>>> This is the 1st patch of a (6-patch) patch set.
>>>
>>> This patch set of six patches introduces xenwatch multithreading (or
>>> multithreaded xenwatch, abbreviated as 'mtwatch') to dom0 kernel. In
>>> addition to the existing single xenwatch thread, each domU has its own
>>> kernel thread ([xen-mtwatch-<domid>]) to process its xenwatch event.
>>>
>>> A kernel parameter 'xen_mtwatch' is introduced to control whether the
>>> feature is enabled or not during dom0 kernel boot. The feature is disabled
>>> by default if 'xen_mtwatch' is not set in grub. In addition, this patch
>>> also introduces the data structures to maintain the status of each per-domU
>>> xenwatch thread. The status of each xenwatch thread (except the default
>>> one) is maintained by a mtwatch domain.
>>>
>>> The feature is available only on dom0.
>>>
>>> Signed-off-by: Dongli Zhang <dongli.zhang@xxxxxxxxxx>
>>> ---
>>>  Documentation/admin-guide/kernel-parameters.txt |  3 ++
>>>  drivers/xen/xenbus/xenbus_xs.c                  | 31 ++++++++++++
>>>  include/xen/xenbus.h                            | 65 +++++++++++++++++++++++++
>>>  3 files changed, 99 insertions(+)
>>>
>>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>>> index 64a3bf5..fc295ef 100644
>>> --- a/Documentation/admin-guide/kernel-parameters.txt
>>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>>> @@ -4992,6 +4992,9 @@
>>>  				the unplug protocol
>>>  			never -- do not unplug even if version check succeeds
>>>  
>>> +	xen_mtwatch     [KNL,XEN]
>>> +			Enables the multithreaded xenwatch (mtwatch).
>>> +
>>>  	xen_nopvspin	[X86,XEN]
>>>  			Disables the ticketlock slowpath using Xen PV
>>>  			optimizations.
>>> diff --git a/drivers/xen/xenbus/xenbus_xs.c b/drivers/xen/xenbus/xenbus_xs.c
>>> index 49a3874..3f137d2 100644
>>> --- a/drivers/xen/xenbus/xenbus_xs.c
>>> +++ b/drivers/xen/xenbus/xenbus_xs.c
>>> @@ -95,6 +95,19 @@ static pid_t xenwatch_pid;
>>>  static DEFINE_MUTEX(xenwatch_mutex);
>>>  static DECLARE_WAIT_QUEUE_HEAD(watch_events_waitq);
>>>  
>>> +bool xen_mtwatch;
>>> +EXPORT_SYMBOL_GPL(xen_mtwatch);
>>> +
>>> +struct mtwatch_info *mtwatch_info;
>>> +
>>> +static bool param_xen_mtwatch;
>>> +static __init int xen_parse_mtwatch(char *arg)
>>> +{
>>> +	param_xen_mtwatch = true;
>>> +	return 0;
>>> +}
>>> +early_param("xen_mtwatch", xen_parse_mtwatch);
>>
>> Add a Kconfig item to set the default when building the kernel? We can
>> start with default "off", but later we might want to enable this feature
>> as the default.
> 
> Is there any weakness/downside configuring the param via early_param? Or is
> there any strength via Kconfig?
> 
> I would prefer to not fix the configuration when building the kernel. The
> administrator/user will not be able to choose the preferred option.

I wouldn't remove the kernel param, but let the config choose the
default to use. The user would still be able to set the value either
way via boot parameter.

> 
> I would prefer to configure the option via kernel param and set default as 'off'
> (false). In a common cloud environment, the administrator is only required to
> enable the param in dom0/driver domain kernel.
> 
> For common domU (not using as backend), this feature is useless. I assume there
> are always more domUs than dom0 (and driver domain), especially in cloud
> environment.
> 
>>
>>> +
>>>  static void xs_suspend_enter(void)
>>>  {
>>>  	spin_lock(&xs_state_lock);
>>> @@ -929,6 +942,24 @@ int xs_init(void)
>>>  	if (err)
>>>  		return err;
>>>  
>>> +	if (xen_initial_domain() && param_xen_mtwatch) {
>>
>> Wouldn't it be better to drop the test for xen_initial_domain() and do
>> this initialization only when a caller (backend) is requesting the
>> multithread mode? This would avoid wasting memory in case it isn't going
>> top be used and - more important - would support driver domains.
> 
> This would save us <4KB.
> 
> Suppose the default 'param_xen_mtwatch' is 'off'. The administrator knows the
> domain is used as pv backend when he/she enables the option manually.
> 
> In such scenario, is there any benefit to make it on-demand to save <4KB memory?

It is a benefit in case a distro ships the kernel with default on in
order to avoid the need to add kernel parameters for dom0 or a driver
domain. In a normal domU there is no backend running so the behavior
would be as today.

If the feature is stable and we know it is better to switch it on for
driver domains (and dom0, of course), we shouldn't require the user to
do so.


Juergen