Some of the monitor id and monitor page data sysfs files display
incorrect data. This patchset provides the following changes:
1) Change the monitor_pages index in server_monitor_pending_show() to
'0', which is the correct index for the server.
2) If monitor pages are not allocated to a channel, display nothing in
the channel's monitor id and monitor page data sysfs files. The data
that is currently shown in sysfs is incorrect.
Kimberly Brown (2):
Drivers: hv: vmbus: Change server monitor_pages index to 0
Drivers: hv: vmbus: Display nothing in sysfs if monitor_allocated not
set
Documentation/ABI/stable/sysfs-bus-vmbus | 9 ++++--
drivers/hv/vmbus_drv.c | 39 +++++++++++++++++++++++-
2 files changed, 44 insertions(+), 4 deletions(-)
--
2.17.1
Change the monitor_pages index in server_monitor_pending_show() to '0'.
'0' is the correct monitor_pages index for the server. A comment for the
monitor_pages field in the vmbus_connection struct definition indicates
that the 1st page is for parent->child notifications. In addition, the
server_monitor_latency_show() and server_monitor_conn_id_show()
functions use monitor_pages index '0'.
Signed-off-by: Kimberly Brown <[email protected]>
---
drivers/hv/vmbus_drv.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 403fee01572c..f2a79f5129d7 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -234,7 +234,7 @@ static ssize_t server_monitor_pending_show(struct device *dev,
return -ENODEV;
return sprintf(buf, "%d\n",
channel_pending(hv_dev->channel,
- vmbus_connection.monitor_pages[1]));
+ vmbus_connection.monitor_pages[0]));
}
static DEVICE_ATTR_RO(server_monitor_pending);
--
2.17.1
If monitor pages are not allocated to a channel, the channel does not
have a valid monitor id or valid monitor page data. In these cases, some
of the "_show" functions display incorrect data. The "_show" functions
that display monitor page data access and display data that is beyond
the bounds of the hv_monitor_page array fields, which is obviously
incorrect. The "_show" functions that display the monitor id display an
invalid monitor id.
The "channel->offermsg.monitor_allocated" value can be used to determine
whether monitor pages have been allocated to a channel. In the affected
"_show" functions, verify that "channel->offermsg.monitor_allocated" is
set before accessing the monitor id or the monitor_page data. If
"channel->offermsg.monitor_allocated" is not set, display nothing.
Signed-off-by: Kimberly Brown <[email protected]>
---
Documentation/ABI/stable/sysfs-bus-vmbus | 9 ++++--
drivers/hv/vmbus_drv.c | 37 ++++++++++++++++++++++++
2 files changed, 43 insertions(+), 3 deletions(-)
diff --git a/Documentation/ABI/stable/sysfs-bus-vmbus b/Documentation/ABI/stable/sysfs-bus-vmbus
index 3fed8fdb873d..af52be4ffc5d 100644
--- a/Documentation/ABI/stable/sysfs-bus-vmbus
+++ b/Documentation/ABI/stable/sysfs-bus-vmbus
@@ -81,7 +81,8 @@ What: /sys/bus/vmbus/devices/<UUID>/channels/<N>/latency
Date: September. 2017
KernelVersion: 4.14
Contact: Stephen Hemminger <[email protected]>
-Description: Channel signaling latency
+Description: Channel signaling latency. If monitor pages are not allocated
+ to the channel, nothing is displayed.
Users: Debugging tools
What: /sys/bus/vmbus/devices/<UUID>/channels/<N>/out_mask
@@ -95,7 +96,8 @@ What: /sys/bus/vmbus/devices/<UUID>/channels/<N>/pending
Date: September. 2017
KernelVersion: 4.14
Contact: Stephen Hemminger <[email protected]>
-Description: Channel interrupt pending state
+Description: Channel interrupt pending state. If monitor pages are not
+ allocated to the channel, nothing is displayed.
Users: Debugging tools
What: /sys/bus/vmbus/devices/<UUID>/channels/<N>/read_avail
@@ -137,7 +139,8 @@ What: /sys/bus/vmbus/devices/<UUID>/channels/<N>/monitor_id
Date: January. 2018
KernelVersion: 4.16
Contact: Stephen Hemminger <[email protected]>
-Description: Monitor bit associated with channel
+Description: Monitor bit associated with channel. If monitor pages are not
+ allocated to the channel, nothing is displayed.
Users: Debugging tools and userspace drivers
What: /sys/bus/vmbus/devices/<UUID>/channels/<N>/ring
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index f2a79f5129d7..c88a3623be56 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -171,6 +171,10 @@ static ssize_t monitor_id_show(struct device *dev,
if (!hv_dev->channel)
return -ENODEV;
+
+ if (!hv_dev->channel->offermsg.monitor_allocated)
+ return sprintf(buf, "\n");
+
return sprintf(buf, "%d\n", hv_dev->channel->offermsg.monitorid);
}
static DEVICE_ATTR_RO(monitor_id);
@@ -232,6 +236,10 @@ static ssize_t server_monitor_pending_show(struct device *dev,
if (!hv_dev->channel)
return -ENODEV;
+
+ if (!hv_dev->channel->offermsg.monitor_allocated)
+ return sprintf(buf, "\n");
+
return sprintf(buf, "%d\n",
channel_pending(hv_dev->channel,
vmbus_connection.monitor_pages[0]));
@@ -246,6 +254,10 @@ static ssize_t client_monitor_pending_show(struct device *dev,
if (!hv_dev->channel)
return -ENODEV;
+
+ if (!hv_dev->channel->offermsg.monitor_allocated)
+ return sprintf(buf, "\n");
+
return sprintf(buf, "%d\n",
channel_pending(hv_dev->channel,
vmbus_connection.monitor_pages[1]));
@@ -260,6 +272,10 @@ static ssize_t server_monitor_latency_show(struct device *dev,
if (!hv_dev->channel)
return -ENODEV;
+
+ if (!hv_dev->channel->offermsg.monitor_allocated)
+ return sprintf(buf, "\n");
+
return sprintf(buf, "%d\n",
channel_latency(hv_dev->channel,
vmbus_connection.monitor_pages[0]));
@@ -274,6 +290,10 @@ static ssize_t client_monitor_latency_show(struct device *dev,
if (!hv_dev->channel)
return -ENODEV;
+
+ if (!hv_dev->channel->offermsg.monitor_allocated)
+ return sprintf(buf, "\n");
+
return sprintf(buf, "%d\n",
channel_latency(hv_dev->channel,
vmbus_connection.monitor_pages[1]));
@@ -288,6 +308,10 @@ static ssize_t server_monitor_conn_id_show(struct device *dev,
if (!hv_dev->channel)
return -ENODEV;
+
+ if (!hv_dev->channel->offermsg.monitor_allocated)
+ return sprintf(buf, "\n");
+
return sprintf(buf, "%d\n",
channel_conn_id(hv_dev->channel,
vmbus_connection.monitor_pages[0]));
@@ -302,6 +326,10 @@ static ssize_t client_monitor_conn_id_show(struct device *dev,
if (!hv_dev->channel)
return -ENODEV;
+
+ if (!hv_dev->channel->offermsg.monitor_allocated)
+ return sprintf(buf, "\n");
+
return sprintf(buf, "%d\n",
channel_conn_id(hv_dev->channel,
vmbus_connection.monitor_pages[1]));
@@ -1469,6 +1497,9 @@ static VMBUS_CHAN_ATTR(cpu, S_IRUGO, show_target_cpu, NULL);
static ssize_t channel_pending_show(const struct vmbus_channel *channel,
char *buf)
{
+ if (!channel->offermsg.monitor_allocated)
+ return sprintf(buf, "\n");
+
return sprintf(buf, "%d\n",
channel_pending(channel,
vmbus_connection.monitor_pages[1]));
@@ -1478,6 +1509,9 @@ static VMBUS_CHAN_ATTR(pending, S_IRUGO, channel_pending_show, NULL);
static ssize_t channel_latency_show(const struct vmbus_channel *channel,
char *buf)
{
+ if (!channel->offermsg.monitor_allocated)
+ return sprintf(buf, "\n");
+
return sprintf(buf, "%d\n",
channel_latency(channel,
vmbus_connection.monitor_pages[1]));
@@ -1499,6 +1533,9 @@ static VMBUS_CHAN_ATTR(events, S_IRUGO, channel_events_show, NULL);
static ssize_t subchannel_monitor_id_show(const struct vmbus_channel *channel,
char *buf)
{
+ if (!channel->offermsg.monitor_allocated)
+ return sprintf(buf, "\n");
+
return sprintf(buf, "%u\n", channel->offermsg.monitorid);
}
static VMBUS_CHAN_ATTR(monitor_id, S_IRUGO, subchannel_monitor_id_show, NULL);
--
2.17.1
On Fri, 8 Feb 2019 04:58:52 -0500
Kimberly Brown <[email protected]> wrote:
> Change the monitor_pages index in server_monitor_pending_show() to '0'.
> '0' is the correct monitor_pages index for the server. A comment for the
> monitor_pages field in the vmbus_connection struct definition indicates
> that the 1st page is for parent->child notifications. In addition, the
> server_monitor_latency_show() and server_monitor_conn_id_show()
> functions use monitor_pages index '0'.
>
> Signed-off-by: Kimberly Brown <[email protected]>
> ---
> drivers/hv/vmbus_drv.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 403fee01572c..f2a79f5129d7 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -234,7 +234,7 @@ static ssize_t server_monitor_pending_show(struct device *dev,
> return -ENODEV;
> return sprintf(buf, "%d\n",
> channel_pending(hv_dev->channel,
> - vmbus_connection.monitor_pages[1]));
> + vmbus_connection.monitor_pages[0]));
> }
> static DEVICE_ATTR_RO(server_monitor_pending);
Looks good.
I wonder if ever gets used though since it returned incorrect data...
Acked-by: Stephen Hemminger <[email protected]>
On Fri, 8 Feb 2019 05:01:12 -0500
Kimberly Brown <[email protected]> wrote:
You are right, the current behavior is broken.
It would be good to add a description of under what conditions
monitor is not used. Is this some part of a project emulating
Hyper-V?
> +
> + if (!hv_dev->channel->offermsg.monitor_allocated)
> + return sprintf(buf, "\n");
If monitor is not used, why not return an error instead of empty
data. Any program (or user) would have to handle that already.
On Fri, Feb 08, 2019 at 02:32:09PM -0800, Stephen Hemminger wrote:
> On Fri, 8 Feb 2019 05:01:12 -0500
> Kimberly Brown <[email protected]> wrote:
>
> You are right, the current behavior is broken.
> It would be good to add a description of under what conditions
> monitor is not used. Is this some part of a project emulating
> Hyper-V?
>
I'm not sure which conditions determine whether the monitor mechanism is
used. I've searched the Hypervisor TLFS, and I couldn't find any
information. If you have any suggestions for where I can find this
information, please let me know.
No, I'm not working on a project emulating Hyper-V.
>
> > +
> > + if (!hv_dev->channel->offermsg.monitor_allocated)
> > + return sprintf(buf, "\n");
>
> If monitor is not used, why not return an error instead of empty
> data. Any program (or user) would have to handle that already.
I think that returning an error instead is fine. I'll make this change
in the next version of the patch.
On Mon, 11 Feb 2019 02:01:18 -0500
Kimberly Brown <[email protected]> wrote:
> On Fri, Feb 08, 2019 at 02:32:09PM -0800, Stephen Hemminger wrote:
> > On Fri, 8 Feb 2019 05:01:12 -0500
> > Kimberly Brown <[email protected]> wrote:
> >
> > You are right, the current behavior is broken.
> > It would be good to add a description of under what conditions
> > monitor is not used. Is this some part of a project emulating
> > Hyper-V?
> >
>
> I'm not sure which conditions determine whether the monitor mechanism is
> used. I've searched the Hypervisor TLFS, and I couldn't find any
> information. If you have any suggestions for where I can find this
> information, please let me know.
The monitor page stuff pre-dates my involvement with Hyper-V. KY might know.
But based on comments it looks like it was added to avoid hypercalls
for each message. It probably showed up in Windows Server 2012 timeframe.
To test you might want to dig up Windows Server 2008.
> No, I'm not working on a project emulating Hyper-V.
OK, I had heard that KVM project was doing something with QEMU.
> >
> > > +
> > > + if (!hv_dev->channel->offermsg.monitor_allocated)
> > > + return sprintf(buf, "\n");
> >
> > If monitor is not used, why not return an error instead of empty
> > data. Any program (or user) would have to handle that already.
>
> I think that returning an error instead is fine. I'll make this change
> in the next version of the patch.
On Mon, Feb 11, 2019 at 10:02:47AM -0800, Stephen Hemminger wrote:
> On Mon, 11 Feb 2019 02:01:18 -0500
> Kimberly Brown <[email protected]> wrote:
>
> > On Fri, Feb 08, 2019 at 02:32:09PM -0800, Stephen Hemminger wrote:
> > > On Fri, 8 Feb 2019 05:01:12 -0500
> > > Kimberly Brown <[email protected]> wrote:
> > >
> > > You are right, the current behavior is broken.
> > > It would be good to add a description of under what conditions
> > > monitor is not used. Is this some part of a project emulating
> > > Hyper-V?
> > >
> >
> > I'm not sure which conditions determine whether the monitor mechanism is
> > used. I've searched the Hypervisor TLFS, and I couldn't find any
> > information. If you have any suggestions for where I can find this
> > information, please let me know.
>
> The monitor page stuff pre-dates my involvement with Hyper-V. KY might know.
> But based on comments it looks like it was added to avoid hypercalls
> for each message. It probably showed up in Windows Server 2012 timeframe.
>
> To test you might want to dig up Windows Server 2008.
>
It looks like the monitor mechanism has always been used. It's present in the
earliest commit that I can find: 3e7ee4902fe6 ("add the Hyper-V virtual bus")
from 2009.
I propose that the following sentences be added to the sysfs documentation for
the affected attributes:
"The monitor page mechanism is used for performance critical channels (storage,
network, etc.). Channels that do not use the monitor page mechanism will return
EINVAL."
I think that this provides sufficient information for a user to understand why
opening an affected file can return EINVAL. What do you think?
On Thu, 14 Feb 2019 01:11:03 -0500
Kimberly Brown <[email protected]> wrote:
> On Mon, Feb 11, 2019 at 10:02:47AM -0800, Stephen Hemminger wrote:
> > On Mon, 11 Feb 2019 02:01:18 -0500
> > Kimberly Brown <[email protected]> wrote:
> >
> > > On Fri, Feb 08, 2019 at 02:32:09PM -0800, Stephen Hemminger wrote:
> > > > On Fri, 8 Feb 2019 05:01:12 -0500
> > > > Kimberly Brown <[email protected]> wrote:
> > > >
> > > > You are right, the current behavior is broken.
> > > > It would be good to add a description of under what conditions
> > > > monitor is not used. Is this some part of a project emulating
> > > > Hyper-V?
> > > >
> > >
> > > I'm not sure which conditions determine whether the monitor mechanism is
> > > used. I've searched the Hypervisor TLFS, and I couldn't find any
> > > information. If you have any suggestions for where I can find this
> > > information, please let me know.
> >
> > The monitor page stuff pre-dates my involvement with Hyper-V. KY might know.
> > But based on comments it looks like it was added to avoid hypercalls
> > for each message. It probably showed up in Windows Server 2012 timeframe.
> >
> > To test you might want to dig up Windows Server 2008.
> >
>
> It looks like the monitor mechanism has always been used. It's present in the
> earliest commit that I can find: 3e7ee4902fe6 ("add the Hyper-V virtual bus")
> from 2009.
>
> I propose that the following sentences be added to the sysfs documentation for
> the affected attributes:
>
> "The monitor page mechanism is used for performance critical channels (storage,
> network, etc.). Channels that do not use the monitor page mechanism will return
> EINVAL."
>
> I think that this provides sufficient information for a user to understand why
> opening an affected file can return EINVAL. What do you think?
Thanks for following up. I agree with you EINVAL works as a solution.
My understanding is that their are two ways a channel can work. The first one is
for the guest to send a hyper call to the host to indicate when data is available.
The other is for the guest to indicate by setting a bit in shared memory with host.
The shared memory approach reduces host/guest overhead and allows for more opportunities
for batching in the ring. The host checks the shared memory on a polling interval
defined in the latency field.
The hypercall method does not use the monitor page. It has lower latency (no polling).
Some of the monitor id and monitor page data sysfs files display
incorrect data. This patchset provides the following changes:
1) Change the monitor_pages index in server_monitor_pending_show() to
'0', which is the correct index for the server.
2) If monitor pages are not allocated to a channel, return -EINVAL when
the channel's monitor id and monitor page data sysfs files are opened.
The data that is currently shown in sysfs is incorrect.
Changes in v2:
Patch 1: "Change server monitor_pages index to 0"
- Added the "Acked-by" line received for v1.
Patch 2: "Return -EINVAL if monitor_allocated not set"
- Changed the return value for cases where monitor_allocated is not set
to "-EINVAL" as suggested by S. Hemminger.
- Updated the commit message to provide more details about the monitor
page mechanism as suggested by S. Hemminger.
- Updated the sysfs documentation to describe the new return value.
Kimberly Brown (2):
Drivers: hv: vmbus: Change server monitor_pages index to 0
Drivers: hv: vmbus: Return -EINVAL if monitor_allocated not set
Documentation/ABI/stable/sysfs-bus-vmbus | 15 +++++++--
drivers/hv/vmbus_drv.c | 39 +++++++++++++++++++++++-
2 files changed, 50 insertions(+), 4 deletions(-)
--
2.17.1
Change the monitor_pages index in server_monitor_pending_show() to '0'.
'0' is the correct monitor_pages index for the server. A comment for the
monitor_pages field in the vmbus_connection struct definition indicates
that the 1st page is for parent->child notifications. In addition, the
server_monitor_latency_show() and server_monitor_conn_id_show()
functions use monitor_pages index '0'.
Signed-off-by: Kimberly Brown <[email protected]>
Acked-by: Stephen Hemminger <[email protected]>
---
drivers/hv/vmbus_drv.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 403fee01572c..f2a79f5129d7 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -234,7 +234,7 @@ static ssize_t server_monitor_pending_show(struct device *dev,
return -ENODEV;
return sprintf(buf, "%d\n",
channel_pending(hv_dev->channel,
- vmbus_connection.monitor_pages[1]));
+ vmbus_connection.monitor_pages[0]));
}
static DEVICE_ATTR_RO(server_monitor_pending);
--
2.17.1
From: Kimberly Brown <[email protected]> Sent: Monday, February 18, 2019 9:38 PM
>
> Change the monitor_pages index in server_monitor_pending_show() to '0'.
> '0' is the correct monitor_pages index for the server. A comment for the
> monitor_pages field in the vmbus_connection struct definition indicates
> that the 1st page is for parent->child notifications. In addition, the
> server_monitor_latency_show() and server_monitor_conn_id_show()
> functions use monitor_pages index '0'.
>
> Signed-off-by: Kimberly Brown <[email protected]>
> Acked-by: Stephen Hemminger <[email protected]>
> ---
> drivers/hv/vmbus_drv.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
Reviewed-by: Michael Kelley <[email protected]>