RE: [PATCH 0/3] drivers/hv: make max_num_channels_supported configurable

[Michael Kelley <mikelley@microsoft.com>]

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