linux-hyperv.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] drivers/hv: make max_num_channels_supported configurable
@ 2020-12-05 17:26 Stefan Eschenbacher
  2020-12-05 17:30 ` [PATCH 1/3] " Stefan Eschenbacher
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Stefan Eschenbacher @ 2020-12-05 17:26 UTC (permalink / raw)
  To: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu
  Cc: Stefan Eschenbacher, linux-hyperv, linux-kernel, linux-kernel,
	Max Stolze

According to the TODO comment in hyperv_vmbus.h the value in macro
MAX_NUM_CHANNELS_SUPPORTED should be configurable. The first patch
accomplishes that by introducting uint max_num_channels_supported as
module parameter.
Also macro MAX_NUM_CHANNELS_SUPPORTED_DEFAULT is introduced with
value 256, which is the currently used macro value.
MAX_NUM_CHANNELS_SUPPORTED was found and replaced in two locations.

During module initialization sanity checks are applied which will result
in EINVAL or ERANGE if the given value is no multiple of 32 or larger than
MAX_NUM_CHANNELS.

While testing, we found a misleading typo in the comment for the macro
MAX_NUM_CHANNELS which is fixed by the second patch.

The third patch makes the added default macro configurable by 
introduction and use of Kconfig parameter HYPERV_VMBUS_DEFAULT_CHANNELS. 
Default value remains at 256.

Two notes on these patches:
1) With above patches it is valid to configure max_num_channels_supported
and MAX_NUM_CHANNELS_SUPPORTED_DEFAULT as 0. We simply don't know if that
is a valid value. Doing so while testing still left us with a working
Debian VM in Hyper-V on Windows 10.
2) To set the Kconfig parameter the user currently has to divide the
desired default number of channels by 32 and enter the result of that
calculation. This way both known constraints we got from the comments in
the code are enforced. It feels a bit unintuitive though. We haven't found
Kconfig options to ensure that the value is a multiple of 32. So if you'd
like us to fix that we'd be happy for some input on how to settle it with
Kconfig.

Signed-off-by: Stefan Eschenbacher <stefan.eschenbacher@fau.de>
Co-developed-by: Max Stolze <max.stolze@fau.de>
Signed-off-by: Max Stolze <max.stolze@fau.de>

Stefan Eschenbacher (3):
  drivers/hv: make max_num_channels_supported configurable
  drivers/hv: fix misleading typo in comment
  drivers/hv: add default number of vmbus channels to Kconfig

 drivers/hv/Kconfig        | 13 +++++++++++++
 drivers/hv/hyperv_vmbus.h |  8 ++++----
 drivers/hv/vmbus_drv.c    | 20 +++++++++++++++++++-
 3 files changed, 36 insertions(+), 5 deletions(-)

-- 
2.20.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/3] drivers/hv: make max_num_channels_supported configurable
  2020-12-05 17:26 [PATCH 0/3] drivers/hv: make max_num_channels_supported configurable Stefan Eschenbacher
@ 2020-12-05 17:30 ` Stefan Eschenbacher
  2020-12-05 17:30 ` [PATCH 2/3] drivers/hv: fix misleading typo in comment Stefan Eschenbacher
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 9+ messages in thread
From: Stefan Eschenbacher @ 2020-12-05 17:30 UTC (permalink / raw)
  To: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu
  Cc: Stefan Eschenbacher, linux-hyperv, linux-kernel, linux-kernel,
	Max Stolze

To make the number of supported channels configurable, this patch
introduces uint max_num_channels_supported as module parameter.
Also macro MAX_NUM_CHANNELS_SUPPORTED_DEFAULT is introduced with
value 256, which is the currently used macro value.
MAX_NUM_CHANNELS_SUPPORTED was found and replaced in two locations.

During module initialization sanity checks are applied which will result
in EINVAL or ERANGE if the given value is no multiple of 32 or larger than
MAX_NUM_CHANNELS.

Signed-off-by: Stefan Eschenbacher <stefan.eschenbacher@fau.de>
Co-developed-by: Max Stolze <max.stolze@fau.de>
Signed-off-by: Max Stolze <max.stolze@fau.de>
---
 drivers/hv/hyperv_vmbus.h |  6 +++---
 drivers/hv/vmbus_drv.c    | 20 +++++++++++++++++++-
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index 02f3e8988836..edeee1c0497d 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -194,11 +194,11 @@ int hv_ringbuffer_read(struct vmbus_channel *channel,
 #define MAX_NUM_CHANNELS	((HV_HYP_PAGE_SIZE >> 1) << 3)
 
 /* The value here must be in multiple of 32 */
-/* TODO: Need to make this configurable */
-#define MAX_NUM_CHANNELS_SUPPORTED	256
+#define MAX_NUM_CHANNELS_SUPPORTED_DEFAULT	256
+extern uint max_num_channels_supported;
 
 #define MAX_CHANNEL_RELIDS					\
-	max(MAX_NUM_CHANNELS_SUPPORTED, HV_EVENT_FLAGS_COUNT)
+	max((ulong) max_num_channels_supported, (ulong) HV_EVENT_FLAGS_COUNT)
 
 enum vmbus_connect_state {
 	DISCONNECTED,
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 502f8cd95f6d..8f1c8a606b4a 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -48,6 +48,11 @@ static int hyperv_cpuhp_online;
 
 static void *hv_panic_page;
 
+uint max_num_channels_supported = MAX_NUM_CHANNELS_SUPPORTED_DEFAULT;
+module_param(max_num_channels_supported, uint, 0);
+MODULE_PARM_DESC(max_num_channels_supported,
+	"Number of supported vmbus channels. Must be a multiple of 32 and equal to or less than 16348");
+
 /* Values parsed from ACPI DSDT */
 static int vmbus_irq;
 int vmbus_interrupt;
@@ -1220,7 +1225,7 @@ static void vmbus_chan_sched(struct hv_per_cpu_context *hv_cpu)
 	u32 maxbits, relid;
 
 	if (vmbus_proto_version < VERSION_WIN8) {
-		maxbits = MAX_NUM_CHANNELS_SUPPORTED;
+		maxbits = max_num_channels_supported;
 		recv_int_page = vmbus_connection.recv_int_page;
 	} else {
 		/*
@@ -2620,6 +2625,17 @@ static int __init hv_acpi_init(void)
 	if (!hv_is_hyperv_initialized())
 		return -ENODEV;
 
+	// Check if max_num_channels_supported is a multiple of 32 and smaller MAX_NUM_CHANNELS
+	if (max_num_channels_supported % 32 != 0) {
+		pr_err("max_num_channels_supported is %u, but must be a multiple of 32\n",
+			max_num_channels_supported);
+		return -EINVAL;
+	} else if (max_num_channels_supported > MAX_NUM_CHANNELS) {
+		pr_err("max_num_channels_supported is %u which exceeds maximum of %lu\n",
+			max_num_channels_supported, MAX_NUM_CHANNELS);
+		return -ERANGE;
+	}
+
 	init_completion(&probe_event);
 
 	/*
@@ -2641,6 +2657,8 @@ static int __init hv_acpi_init(void)
 	if (ret)
 		goto cleanup;
 
+	pr_info("Supporting up to %u channels\n", max_num_channels_supported);
+
 	hv_setup_kexec_handler(hv_kexec_handler);
 	hv_setup_crash_handler(hv_crash_handler);
 
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/3] drivers/hv: fix misleading typo in comment
  2020-12-05 17:26 [PATCH 0/3] drivers/hv: make max_num_channels_supported configurable Stefan Eschenbacher
  2020-12-05 17:30 ` [PATCH 1/3] " Stefan Eschenbacher
@ 2020-12-05 17:30 ` Stefan Eschenbacher
  2020-12-05 17:30 ` [PATCH 3/3] drivers/hv: add default number of vmbus channels to Kconfig Stefan Eschenbacher
  2020-12-05 18:27 ` [PATCH 0/3] drivers/hv: make max_num_channels_supported configurable Michael Kelley
  3 siblings, 0 replies; 9+ messages in thread
From: Stefan Eschenbacher @ 2020-12-05 17:30 UTC (permalink / raw)
  To: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu
  Cc: Stefan Eschenbacher, linux-hyperv, linux-kernel, linux-kernel,
	Max Stolze

Fixes a misleading typo in the comment for the macro MAX_NUM_CHANNELS,
where two digits have been twisted.

Signed-off-by: Stefan Eschenbacher <stefan.eschenbacher@fau.de>
Co-developed-by: Max Stolze <max.stolze@fau.de>
Signed-off-by: Max Stolze <max.stolze@fau.de>
---
 drivers/hv/hyperv_vmbus.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index edeee1c0497d..52dcfa1c17ef 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -187,7 +187,7 @@ int hv_ringbuffer_read(struct vmbus_channel *channel,
 		       u64 *requestid, bool raw);
 
 /*
- * The Maximum number of channels (16348) is determined by the size of the
+ * The Maximum number of channels (16384) is determined by the size of the
  * interrupt page, which is HV_HYP_PAGE_SIZE. 1/2 of HV_HYP_PAGE_SIZE is to
  * send endpoint interrupts, and the other is to receive endpoint interrupts.
  */
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 3/3] drivers/hv: add default number of vmbus channels to Kconfig
  2020-12-05 17:26 [PATCH 0/3] drivers/hv: make max_num_channels_supported configurable Stefan Eschenbacher
  2020-12-05 17:30 ` [PATCH 1/3] " Stefan Eschenbacher
  2020-12-05 17:30 ` [PATCH 2/3] drivers/hv: fix misleading typo in comment Stefan Eschenbacher
@ 2020-12-05 17:30 ` Stefan Eschenbacher
  2020-12-05 18:27 ` [PATCH 0/3] drivers/hv: make max_num_channels_supported configurable Michael Kelley
  3 siblings, 0 replies; 9+ messages in thread
From: Stefan Eschenbacher @ 2020-12-05 17:30 UTC (permalink / raw)
  To: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu
  Cc: Stefan Eschenbacher, linux-hyperv, linux-kernel, linux-kernel,
	Max Stolze

The default number of vmbus channels (macro
MAX_NUM_CHANNELS_SUPPORTED_DEFAULT) is made configurable through the new
Kconfig option HYPERV_VMBUS_DEFAULT_CHANNELS.

Signed-off-by: Stefan Eschenbacher <stefan.eschenbacher@fau.de>
Co-developed-by: Max Stolze <max.stolze@fau.de>
Signed-off-by: Max Stolze <max.stolze@fau.de>
---
 drivers/hv/Kconfig        | 13 +++++++++++++
 drivers/hv/hyperv_vmbus.h |  2 +-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/hv/Kconfig b/drivers/hv/Kconfig
index 79e5356a737a..40bee5b05ccd 100644
--- a/drivers/hv/Kconfig
+++ b/drivers/hv/Kconfig
@@ -26,4 +26,17 @@ config HYPERV_BALLOON
 	help
 	  Select this option to enable Hyper-V Balloon driver.
 
+config HYPERV_VMBUS_DEFAULT_CHANNELS
+	int "Default number of VMBus channels (as multiple of 32)"
+	range 0 512
+	default 8
+	depends on HYPERV
+	help
+	  Specify the default number of VMBus channels here as nth multiple of
+	  32. The value must be equal to or less than 512.
+	  The default is 8 meaning the number of channels is 8 * 32 = 256.
+	  A different value can be set when loading the hv_vmbus module by
+	  setting the parameter max_num_channels_supported directly to the
+	  number of channels and not as a multiple of 32.
+
 endmenu
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index 52dcfa1c17ef..47af2c76ce57 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -194,7 +194,7 @@ int hv_ringbuffer_read(struct vmbus_channel *channel,
 #define MAX_NUM_CHANNELS	((HV_HYP_PAGE_SIZE >> 1) << 3)
 
 /* The value here must be in multiple of 32 */
-#define MAX_NUM_CHANNELS_SUPPORTED_DEFAULT	256
+#define MAX_NUM_CHANNELS_SUPPORTED_DEFAULT	(CONFIG_HYPERV_VMBUS_DEFAULT_CHANNELS * 32)
 extern uint max_num_channels_supported;
 
 #define MAX_CHANNEL_RELIDS					\
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* RE: [PATCH 0/3] drivers/hv: make max_num_channels_supported configurable
  2020-12-05 17:26 [PATCH 0/3] drivers/hv: make max_num_channels_supported configurable Stefan Eschenbacher
                   ` (2 preceding siblings ...)
  2020-12-05 17:30 ` [PATCH 3/3] drivers/hv: add default number of vmbus channels to Kconfig Stefan Eschenbacher
@ 2020-12-05 18:27 ` Michael Kelley
  2020-12-05 20:32   ` Max Stolze
  3 siblings, 1 reply; 9+ messages in thread
From: Michael Kelley @ 2020-12-05 18:27 UTC (permalink / raw)
  To: Stefan Eschenbacher, KY Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Wei Liu
  Cc: linux-hyperv, linux-kernel, linux-kernel, Max Stolze

From: Stefan Eschenbacher <stefan.eschenbacher@fau.de>
> 
> According to the TODO comment in hyperv_vmbus.h the value in macro
> MAX_NUM_CHANNELS_SUPPORTED should be configurable. The first patch
> accomplishes that by introducting uint max_num_channels_supported as
> module parameter.
> Also macro MAX_NUM_CHANNELS_SUPPORTED_DEFAULT is introduced with
> value 256, which is the currently used macro value.
> MAX_NUM_CHANNELS_SUPPORTED was found and replaced in two locations.
> 
> During module initialization sanity checks are applied which will result
> in EINVAL or ERANGE if the given value is no multiple of 32 or larger than
> MAX_NUM_CHANNELS.
> 
> While testing, we found a misleading typo in the comment for the macro
> MAX_NUM_CHANNELS which is fixed by the second patch.
> 
> The third patch makes the added default macro configurable by
> introduction and use of Kconfig parameter HYPERV_VMBUS_DEFAULT_CHANNELS.
> Default value remains at 256.
> 
> Two notes on these patches:
> 1) With above patches it is valid to configure max_num_channels_supported
> and MAX_NUM_CHANNELS_SUPPORTED_DEFAULT as 0. We simply don't know if that
> is a valid value. Doing so while testing still left us with a working
> Debian VM in Hyper-V on Windows 10.
> 2) To set the Kconfig parameter the user currently has to divide the
> desired default number of channels by 32 and enter the result of that
> calculation. This way both known constraints we got from the comments in
> the code are enforced. It feels a bit unintuitive though. We haven't found
> Kconfig options to ensure that the value is a multiple of 32. So if you'd
> like us to fix that we'd be happy for some input on how to settle it with
> Kconfig.
> 
> Signed-off-by: Stefan Eschenbacher <stefan.eschenbacher@fau.de>
> Co-developed-by: Max Stolze <max.stolze@fau.de>
> Signed-off-by: Max Stolze <max.stolze@fau.de>
> 
> Stefan Eschenbacher (3):
>   drivers/hv: make max_num_channels_supported configurable
>   drivers/hv: fix misleading typo in comment
>   drivers/hv: add default number of vmbus channels to Kconfig
> 
>  drivers/hv/Kconfig        | 13 +++++++++++++
>  drivers/hv/hyperv_vmbus.h |  8 ++++----
>  drivers/hv/vmbus_drv.c    | 20 +++++++++++++++++++-
>  3 files changed, 36 insertions(+), 5 deletions(-)
> 
> --
> 2.20.1

Stefan -- this cover letter email came through, but it doesn't look like
the individual patch emails did.  So you might want to check your
patch sending process.

Thanks for your interest in this old "TODO" item.  But let me provide some
additional background.  Starting in Windows 8 and Windows Server 2012,
Hyper-V revised the mechanism by which channel interrupt notifications
are made.  The MAX_NUM_CHANNELS_SUPPORTED value is only used
with Windows 7 and Windows Server 2008 R2, neither of which is officially
supported any longer.  See the code at the top of vmbus_chan_sched() where
the VMBus protocol version is checked, and MAX_NUM_CHANNELS_SUPPORTED
is used only when the protocol version indicates we're running on Windows 7
(or the equivalent Windows Server 2008 R2).

Because the old mechanism was superseded, making the value configurable
doesn't have any benefit.   At some point, we will remove this old code path
entirely, including the #define MAX_NUM_CHANNELS_SUPPORTED.  The
comment with the "TODO" could be removed, but other than that, I don't
think we want to make these changes.

Michael

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/3] drivers/hv: make max_num_channels_supported configurable
  2020-12-05 18:27 ` [PATCH 0/3] drivers/hv: make max_num_channels_supported configurable Michael Kelley
@ 2020-12-05 20:32   ` Max Stolze
  2020-12-05 21:33     ` Michael Kelley
  0 siblings, 1 reply; 9+ messages in thread
From: Max Stolze @ 2020-12-05 20:32 UTC (permalink / raw)
  To: Michael Kelley, Stefan Eschenbacher, KY Srinivasan,
	Haiyang Zhang, Stephen Hemminger, Wei Liu
  Cc: linux-hyperv, linux-kernel, linux-kernel



On 05/12/2020 19:27, Michael Kelley wrote:
> From: Stefan Eschenbacher <stefan.eschenbacher@fau.de>
>>
>> According to the TODO comment in hyperv_vmbus.h the value in macro
>> MAX_NUM_CHANNELS_SUPPORTED should be configurable. The first patch
>> accomplishes that by introducting uint max_num_channels_supported as
>> module parameter.
>> Also macro MAX_NUM_CHANNELS_SUPPORTED_DEFAULT is introduced with
>> value 256, which is the currently used macro value.
>> MAX_NUM_CHANNELS_SUPPORTED was found and replaced in two locations.
>>
>> During module initialization sanity checks are applied which will result
>> in EINVAL or ERANGE if the given value is no multiple of 32 or larger than
>> MAX_NUM_CHANNELS.
>>
>> While testing, we found a misleading typo in the comment for the macro
>> MAX_NUM_CHANNELS which is fixed by the second patch.
>>
>> The third patch makes the added default macro configurable by
>> introduction and use of Kconfig parameter HYPERV_VMBUS_DEFAULT_CHANNELS.
>> Default value remains at 256.
>>
>> Two notes on these patches:
>> 1) With above patches it is valid to configure max_num_channels_supported
>> and MAX_NUM_CHANNELS_SUPPORTED_DEFAULT as 0. We simply don't know if that
>> is a valid value. Doing so while testing still left us with a working
>> Debian VM in Hyper-V on Windows 10.
>> 2) To set the Kconfig parameter the user currently has to divide the
>> desired default number of channels by 32 and enter the result of that
>> calculation. This way both known constraints we got from the comments in
>> the code are enforced. It feels a bit unintuitive though. We haven't found
>> Kconfig options to ensure that the value is a multiple of 32. So if you'd
>> like us to fix that we'd be happy for some input on how to settle it with
>> Kconfig.
>>
>> Signed-off-by: Stefan Eschenbacher <stefan.eschenbacher@fau.de>
>> Co-developed-by: Max Stolze <max.stolze@fau.de>
>> Signed-off-by: Max Stolze <max.stolze@fau.de>
>>
>> Stefan Eschenbacher (3):
>>   drivers/hv: make max_num_channels_supported configurable
>>   drivers/hv: fix misleading typo in comment
>>   drivers/hv: add default number of vmbus channels to Kconfig
>>
>>  drivers/hv/Kconfig        | 13 +++++++++++++
>>  drivers/hv/hyperv_vmbus.h |  8 ++++----
>>  drivers/hv/vmbus_drv.c    | 20 +++++++++++++++++++-
>>  3 files changed, 36 insertions(+), 5 deletions(-)
>>
>> --
>> 2.20.1
> 
> Stefan -- this cover letter email came through, but it doesn't look like
> the individual patch emails did.  So you might want to check your
> patch sending process.
> 
> Thanks for your interest in this old "TODO" item.  But let me provide some
> additional background.  Starting in Windows 8 and Windows Server 2012,
> Hyper-V revised the mechanism by which channel interrupt notifications
> are made.  The MAX_NUM_CHANNELS_SUPPORTED value is only used
> with Windows 7 and Windows Server 2008 R2, neither of which is officially
> supported any longer.  See the code at the top of vmbus_chan_sched() where
> the VMBus protocol version is checked, and MAX_NUM_CHANNELS_SUPPORTED
> is used only when the protocol version indicates we're running on Windows 7
> (or the equivalent Windows Server 2008 R2).
> 
> Because the old mechanism was superseded, making the value configurable
> doesn't have any benefit.   At some point, we will remove this old code path
> entirely, including the #define MAX_NUM_CHANNELS_SUPPORTED.  The
> comment with the "TODO" could be removed, but other than that, I don't
> think we want to make these changes.
> 
> Michael
> 

Hi Michael, 
thanks for the insight and explanation.
It's a pity that the rest of the series did not make it trough.
However, given what you wrote it doesn't seem to be of 
utmost importance.

As irrelevant as it may be we'd still be glad to see the TODO gone.
Given that we found it by looking for things that can be done around
the kernel I don't see why somebody else would not find it while looking
for the same.

Max

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH 0/3] drivers/hv: make max_num_channels_supported configurable
  2020-12-05 20:32   ` Max Stolze
@ 2020-12-05 21:33     ` Michael Kelley
  2020-12-06 10:48       ` [PATCH] drivers/hv: remove obsolete TODO and fix misleading typo in comment Stefan Eschenbacher
  0 siblings, 1 reply; 9+ messages in thread
From: Michael Kelley @ 2020-12-05 21:33 UTC (permalink / raw)
  To: Max Stolze, Stefan Eschenbacher, KY Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Wei Liu
  Cc: linux-hyperv, linux-kernel, linux-kernel

From: Max Stolze <max.stolze@fau.de> Sent: Saturday, December 5, 2020 12:32 PM
> 
> On 05/12/2020 19:27, Michael Kelley wrote:
> > From: Stefan Eschenbacher <stefan.eschenbacher@fau.de>
> >>
> >> According to the TODO comment in hyperv_vmbus.h the value in macro
> >> MAX_NUM_CHANNELS_SUPPORTED should be configurable. The first patch
> >> accomplishes that by introducting uint max_num_channels_supported as
> >> module parameter.
> >> Also macro MAX_NUM_CHANNELS_SUPPORTED_DEFAULT is introduced with
> >> value 256, which is the currently used macro value.
> >> MAX_NUM_CHANNELS_SUPPORTED was found and replaced in two locations.
> >>
> >> During module initialization sanity checks are applied which will result
> >> in EINVAL or ERANGE if the given value is no multiple of 32 or larger than
> >> MAX_NUM_CHANNELS.
> >>
> >> While testing, we found a misleading typo in the comment for the macro
> >> MAX_NUM_CHANNELS which is fixed by the second patch.
> >>
> >> The third patch makes the added default macro configurable by
> >> introduction and use of Kconfig parameter HYPERV_VMBUS_DEFAULT_CHANNELS.
> >> Default value remains at 256.
> >>
> >> Two notes on these patches:
> >> 1) With above patches it is valid to configure max_num_channels_supported
> >> and MAX_NUM_CHANNELS_SUPPORTED_DEFAULT as 0. We simply don't know if that
> >> is a valid value. Doing so while testing still left us with a working
> >> Debian VM in Hyper-V on Windows 10.
> >> 2) To set the Kconfig parameter the user currently has to divide the
> >> desired default number of channels by 32 and enter the result of that
> >> calculation. This way both known constraints we got from the comments in
> >> the code are enforced. It feels a bit unintuitive though. We haven't found
> >> Kconfig options to ensure that the value is a multiple of 32. So if you'd
> >> like us to fix that we'd be happy for some input on how to settle it with
> >> Kconfig.
> >>
> >> Signed-off-by: Stefan Eschenbacher <stefan.eschenbacher@fau.de>
> >> Co-developed-by: Max Stolze <max.stolze@fau.de>
> >> Signed-off-by: Max Stolze <max.stolze@fau.de>
> >>
> >> Stefan Eschenbacher (3):
> >>   drivers/hv: make max_num_channels_supported configurable
> >>   drivers/hv: fix misleading typo in comment
> >>   drivers/hv: add default number of vmbus channels to Kconfig
> >>
> >>  drivers/hv/Kconfig        | 13 +++++++++++++
> >>  drivers/hv/hyperv_vmbus.h |  8 ++++----
> >>  drivers/hv/vmbus_drv.c    | 20 +++++++++++++++++++-
> >>  3 files changed, 36 insertions(+), 5 deletions(-)
> >>
> >> --
> >> 2.20.1
> >
> > Stefan -- this cover letter email came through, but it doesn't look like
> > the individual patch emails did.  So you might want to check your
> > patch sending process.
> >
> > Thanks for your interest in this old "TODO" item.  But let me provide some
> > additional background.  Starting in Windows 8 and Windows Server 2012,
> > Hyper-V revised the mechanism by which channel interrupt notifications
> > are made.  The MAX_NUM_CHANNELS_SUPPORTED value is only used
> > with Windows 7 and Windows Server 2008 R2, neither of which is officially
> > supported any longer.  See the code at the top of vmbus_chan_sched() where
> > the VMBus protocol version is checked, and MAX_NUM_CHANNELS_SUPPORTED
> > is used only when the protocol version indicates we're running on Windows 7
> > (or the equivalent Windows Server 2008 R2).
> >
> > Because the old mechanism was superseded, making the value configurable
> > doesn't have any benefit.   At some point, we will remove this old code path
> > entirely, including the #define MAX_NUM_CHANNELS_SUPPORTED.  The
> > comment with the "TODO" could be removed, but other than that, I don't
> > think we want to make these changes.
> >
> > Michael
> >
> 
> Hi Michael,
> thanks for the insight and explanation.
> It's a pity that the rest of the series did not make it trough.
> However, given what you wrote it doesn't seem to be of
> utmost importance.
> 
> As irrelevant as it may be we'd still be glad to see the TODO gone.
> Given that we found it by looking for things that can be done around
> the kernel I don't see why somebody else would not find it while looking
> for the same.
> 
> Max

The additional patches did finally show up -- about 45 minutes later.  The same
delay occurred in the patches appearing on lkml.org, so there must be have
been some delay on the sending side.  No matter.

I would be fine if you want to submit a patch that removes the "TODO" and
fixes the 16348 vs. 16384 typo in the comment for MAX_NUM_CHANNELS.

Michael



^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH] drivers/hv: remove obsolete TODO and fix misleading typo in comment
  2020-12-05 21:33     ` Michael Kelley
@ 2020-12-06 10:48       ` Stefan Eschenbacher
  2020-12-07 11:25         ` Wei Liu
  0 siblings, 1 reply; 9+ messages in thread
From: Stefan Eschenbacher @ 2020-12-06 10:48 UTC (permalink / raw)
  To: Michael Kelley, Max Stolze, KY Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Wei Liu
  Cc: Stefan Eschenbacher, linux-hyperv, linux-kernel, linux-kernel

Removes an obsolete TODO in the VMBus module and fixes a misleading typo
in the comment for the macro MAX_NUM_CHANNELS, where two digits have been
twisted.

Signed-off-by: Stefan Eschenbacher <stefan.eschenbacher@fau.de>
Co-developed-by: Max Stolze <max.stolze@fau.de>
Signed-off-by: Max Stolze <max.stolze@fau.de>
---
 drivers/hv/hyperv_vmbus.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index 02f3e8988836..9416e09ebd58 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -187,14 +187,13 @@ int hv_ringbuffer_read(struct vmbus_channel *channel,
 		       u64 *requestid, bool raw);
 
 /*
- * The Maximum number of channels (16348) is determined by the size of the
+ * The Maximum number of channels (16384) is determined by the size of the
  * interrupt page, which is HV_HYP_PAGE_SIZE. 1/2 of HV_HYP_PAGE_SIZE is to
  * send endpoint interrupts, and the other is to receive endpoint interrupts.
  */
 #define MAX_NUM_CHANNELS	((HV_HYP_PAGE_SIZE >> 1) << 3)
 
 /* The value here must be in multiple of 32 */
-/* TODO: Need to make this configurable */
 #define MAX_NUM_CHANNELS_SUPPORTED	256
 
 #define MAX_CHANNEL_RELIDS					\
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] drivers/hv: remove obsolete TODO and fix misleading typo in comment
  2020-12-06 10:48       ` [PATCH] drivers/hv: remove obsolete TODO and fix misleading typo in comment Stefan Eschenbacher
@ 2020-12-07 11:25         ` Wei Liu
  0 siblings, 0 replies; 9+ messages in thread
From: Wei Liu @ 2020-12-07 11:25 UTC (permalink / raw)
  To: Stefan Eschenbacher
  Cc: Michael Kelley, Max Stolze, KY Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Wei Liu, linux-hyperv, linux-kernel,
	linux-kernel

On Sun, Dec 06, 2020 at 11:48:50AM +0100, Stefan Eschenbacher wrote:
> Removes an obsolete TODO in the VMBus module and fixes a misleading typo
> in the comment for the macro MAX_NUM_CHANNELS, where two digits have been
> twisted.
> 
> Signed-off-by: Stefan Eschenbacher <stefan.eschenbacher@fau.de>
> Co-developed-by: Max Stolze <max.stolze@fau.de>
> Signed-off-by: Max Stolze <max.stolze@fau.de>

Applied to hyperv-next. Thanks.

Wei.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2020-12-07 11:26 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-05 17:26 [PATCH 0/3] drivers/hv: make max_num_channels_supported configurable Stefan Eschenbacher
2020-12-05 17:30 ` [PATCH 1/3] " Stefan Eschenbacher
2020-12-05 17:30 ` [PATCH 2/3] drivers/hv: fix misleading typo in comment Stefan Eschenbacher
2020-12-05 17:30 ` [PATCH 3/3] drivers/hv: add default number of vmbus channels to Kconfig Stefan Eschenbacher
2020-12-05 18:27 ` [PATCH 0/3] drivers/hv: make max_num_channels_supported configurable Michael Kelley
2020-12-05 20:32   ` Max Stolze
2020-12-05 21:33     ` Michael Kelley
2020-12-06 10:48       ` [PATCH] drivers/hv: remove obsolete TODO and fix misleading typo in comment Stefan Eschenbacher
2020-12-07 11:25         ` Wei Liu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).