[PATCH 0/2] Drivers: hv: vmbus: Fix sysfs functions that display monitor id and page data

[PATCH 0/2] Drivers: hv: vmbus: Fix sysfs functions that display monitor id and page data

[Kimberly Brown]

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

[PATCH 1/2] Drivers: hv: vmbus: Change server monitor_pages index to 0

[Kimberly Brown]

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 <kimbrownkd@gmail.com>
---
 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

Re: [PATCH 1/2] Drivers: hv: vmbus: Change server monitor_pages index to 0

[Stephen Hemminger]

On Fri, 8 Feb 2019 04:58:52 -0500
Kimberly Brown <kimbrownkd@gmail.com> 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 <kimbrownkd@gmail.com>
> ---
>  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 <stephen@networkplumber.org>

[PATCH 2/2] Drivers: hv: vmbus: Display nothing in sysfs if monitor_allocated not set

[Kimberly Brown]

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 <kimbrownkd@gmail.com>
---
 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 <sthemmin@microsoft.com>
-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 <sthemmin@microsoft.com>
-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 <sthemmin@microsoft.com>
-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

Re: [PATCH 2/2] Drivers: hv: vmbus: Display nothing in sysfs if monitor_allocated not set

[Stephen Hemminger]

On Fri, 8 Feb 2019 05:01:12 -0500
Kimberly Brown <kimbrownkd@gmail.com> 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.

Re: [PATCH 2/2] Drivers: hv: vmbus: Display nothing in sysfs if monitor_allocated not set

[Kimberly Brown]

On Fri, Feb 08, 2019 at 02:32:09PM -0800, Stephen Hemminger wrote:
> On Fri, 8 Feb 2019 05:01:12 -0500
> Kimberly Brown <kimbrownkd@gmail.com> 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.

Re: [PATCH 2/2] Drivers: hv: vmbus: Display nothing in sysfs if monitor_allocated not set

[Stephen Hemminger]

On Mon, 11 Feb 2019 02:01:18 -0500
Kimberly Brown <kimbrownkd@gmail.com> 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 <kimbrownkd@gmail.com> 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.

Re: [PATCH 2/2] Drivers: hv: vmbus: Display nothing in sysfs if monitor_allocated not set

[Kimberly Brown]

On Mon, Feb 11, 2019 at 10:02:47AM -0800, Stephen Hemminger wrote:
> On Mon, 11 Feb 2019 02:01:18 -0500
> Kimberly Brown <kimbrownkd@gmail.com> 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 <kimbrownkd@gmail.com> 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?

Re: [PATCH 2/2] Drivers: hv: vmbus: Display nothing in sysfs if monitor_allocated not set

[Stephen Hemminger]

On Thu, 14 Feb 2019 01:11:03 -0500
Kimberly Brown <kimbrownkd@gmail.com> 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 <kimbrownkd@gmail.com> 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 <kimbrownkd@gmail.com> 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).

[PATCH v2 0/2] Drivers: hv: vmbus: Fix sysfs functions that display monitor id and page data

[Kimberly Brown]

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

[PATCH v2 1/2] Drivers: hv: vmbus: Change server monitor_pages index to 0

[Kimberly Brown]

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 <kimbrownkd@gmail.com>
Acked-by: Stephen Hemminger <stephen@networkplumber.org>
---
 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

RE: [PATCH v2 1/2] Drivers: hv: vmbus: Change server monitor_pages index to 0

[Michael Kelley]

From: Kimberly Brown <kimbrownkd@gmail.com>  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 <kimbrownkd@gmail.com>
> Acked-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  drivers/hv/vmbus_drv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Reviewed-by: Michael Kelley <mikelley@microsoft.com>

[PATCH v2 2/2] Drivers: hv: vmbus: Return -EINVAL if monitor_allocated not set

[Kimberly Brown]

There are two methods for signaling the host: the monitor page mechanism
and hypercalls. The monitor page mechanism is used by performance
critical channels (storage, networking, etc.) because it provides
improved throughput. However, latency is increased. Monitor pages are
allocated to these channels.

Monitor pages are not allocated to channels that do not use the monitor
page mechanism. Therefore, these channels do not have a valid monitor id
or valid monitor page data. In these cases, some of the "_show"
functions return incorrect data. They return an invalid monitor id and
data that is beyond the bounds of the hv_monitor_page array fields.

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, return -EINVAL.

Signed-off-by: Kimberly Brown <kimbrownkd@gmail.com>
---
 Documentation/ABI/stable/sysfs-bus-vmbus | 15 ++++++++--
 drivers/hv/vmbus_drv.c                   | 37 ++++++++++++++++++++++++
 2 files changed, 49 insertions(+), 3 deletions(-)

diff --git a/Documentation/ABI/stable/sysfs-bus-vmbus b/Documentation/ABI/stable/sysfs-bus-vmbus
index 3fed8fdb873d..9e8a7a29b3ff 100644
--- a/Documentation/ABI/stable/sysfs-bus-vmbus
+++ b/Documentation/ABI/stable/sysfs-bus-vmbus
@@ -81,7 +81,10 @@ What:		/sys/bus/vmbus/devices/<UUID>/channels/<N>/latency
 Date:		September. 2017
 KernelVersion:	4.14
 Contact:	Stephen Hemminger <sthemmin@microsoft.com>
-Description:	Channel signaling latency
+Description:	Channel signaling latency. 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.
 Users:		Debugging tools
 
 What:		/sys/bus/vmbus/devices/<UUID>/channels/<N>/out_mask
@@ -95,7 +98,10 @@ What:		/sys/bus/vmbus/devices/<UUID>/channels/<N>/pending
 Date:		September. 2017
 KernelVersion:	4.14
 Contact:	Stephen Hemminger <sthemmin@microsoft.com>
-Description:	Channel interrupt pending state
+Description:	Channel interrupt pending state. 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.
 Users:		Debugging tools
 
 What:		/sys/bus/vmbus/devices/<UUID>/channels/<N>/read_avail
@@ -137,7 +143,10 @@ What:		/sys/bus/vmbus/devices/<UUID>/channels/<N>/monitor_id
 Date:		January. 2018
 KernelVersion:	4.16
 Contact:	Stephen Hemminger <sthemmin@microsoft.com>
-Description:	Monitor bit associated with channel
+Description:	Monitor bit associated with channel. 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.
 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..0ff9a09a3f71 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 -EINVAL;
+
 	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 -EINVAL;
+
 	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 -EINVAL;
+
 	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 -EINVAL;
+
 	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 -EINVAL;
+
 	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 -EINVAL;
+
 	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 -EINVAL;
+
 	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 -EINVAL;
+
 	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 -EINVAL;
+
 	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 -EINVAL;
+
 	return sprintf(buf, "%u\n", channel->offermsg.monitorid);
 }
 static VMBUS_CHAN_ATTR(monitor_id, S_IRUGO, subchannel_monitor_id_show, NULL);
-- 
2.17.1

RE: [PATCH v2 2/2] Drivers: hv: vmbus: Return -EINVAL if monitor_allocated not set

[Michael Kelley]

From: Kimberly Brown <kimbrownkd@gmail.com>  Sent: Monday, February 18, 2019 9:38 PM
> 
> There are two methods for signaling the host: the monitor page mechanism
> and hypercalls. The monitor page mechanism is used by performance
> critical channels (storage, networking, etc.) because it provides
> improved throughput. However, latency is increased. Monitor pages are
> allocated to these channels.
> 
> Monitor pages are not allocated to channels that do not use the monitor
> page mechanism. Therefore, these channels do not have a valid monitor id
> or valid monitor page data. In these cases, some of the "_show"
> functions return incorrect data. They return an invalid monitor id and
> data that is beyond the bounds of the hv_monitor_page array fields.
> 
> 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, return -EINVAL.
> 
> Signed-off-by: Kimberly Brown <kimbrownkd@gmail.com>
>

Reviewed-by: Michael Kelley <mikelley@microsoft.com>

[PATCH v3] Drivers: hv: vmbus: Expose monitor data when channel uses monitor pages

[Kimberly Brown]

There are two methods for signaling the host: the monitor page mechanism
and hypercalls. The monitor page mechanism is used by performance
critical channels (storage, networking, etc.) because it provides
improved throughput. However, latency is increased. Monitor pages are
allocated to these channels.

Monitor pages are not allocated to channels that do not use the monitor
page mechanism. Therefore, these channels do not have a valid monitor id
or valid monitor page data. In these cases, some of the "_show"
functions return incorrect data. They return an invalid monitor id and
data that is beyond the bounds of the hv_monitor_page array fields.

The "channel->offermsg.monitor_allocated" value can be used to determine
whether monitor pages have been allocated to a channel.

Move the device-level monitor page attributes to a separate
attribute_group struct. If the channel uses the monitor page mechanism,
set up the sysfs files for these attributes in vmbus_device_register().

Move the channel-level monitor page attributes to a separate
attribute_group struct. If the channel uses the monitor page mechanism,
set up the sysfs files for these attributes in vmbus_add_channel_kobj().

Signed-off-by: Kimberly Brown <kimbrownkd@gmail.com>
---
Changes in v3:
The monitor "_show" functions no longer return an error when a channel
does not use the monitor page mechanism. Instead, the monitor page sysfs
files are created only when a channel uses the monitor page mechanism.
This change was suggested by G. Kroah-Hartman.

Note: this patch was originally patch 2/2 in a patchset. Patch 1/2 has
already been added to char-misc-testing, so I'm not resending it.

Changes in v2:
 - Changed the return value for cases where monitor_allocated is not set
   to "-EINVAL".
 - Updated the commit message to provide more details about the monitor
   page mechanism.
 - Updated the sysfs documentation to describe the new return value.

 Documentation/ABI/stable/sysfs-bus-vmbus | 12 ++++--
 drivers/hv/vmbus_drv.c                   | 52 +++++++++++++++++++-----
 2 files changed, 51 insertions(+), 13 deletions(-)

diff --git a/Documentation/ABI/stable/sysfs-bus-vmbus b/Documentation/ABI/stable/sysfs-bus-vmbus
index 826689dcc2e6..6d5cb195b119 100644
--- a/Documentation/ABI/stable/sysfs-bus-vmbus
+++ b/Documentation/ABI/stable/sysfs-bus-vmbus
@@ -81,7 +81,9 @@ What:		/sys/bus/vmbus/devices/<UUID>/channels/<N>/latency
 Date:		September. 2017
 KernelVersion:	4.14
 Contact:	Stephen Hemminger <sthemmin@microsoft.com>
-Description:	Channel signaling latency
+Description:	Channel signaling latency. This file is available only for
+		performance critical channels (storage, network, etc.) that use
+		the monitor page mechanism.
 Users:		Debugging tools
 
 What:		/sys/bus/vmbus/devices/<UUID>/channels/<N>/out_mask
@@ -95,7 +97,9 @@ What:		/sys/bus/vmbus/devices/<UUID>/channels/<N>/pending
 Date:		September. 2017
 KernelVersion:	4.14
 Contact:	Stephen Hemminger <sthemmin@microsoft.com>
-Description:	Channel interrupt pending state
+Description:	Channel interrupt pending state. This file is available only for
+                performance critical channels (storage, network, etc.) that use
+                the monitor page mechanism.
 Users:		Debugging tools
 
 What:		/sys/bus/vmbus/devices/<UUID>/channels/<N>/read_avail
@@ -137,7 +141,9 @@ What:		/sys/bus/vmbus/devices/<UUID>/channels/<N>/monitor_id
 Date:		January. 2018
 KernelVersion:	4.16
 Contact:	Stephen Hemminger <sthemmin@microsoft.com>
-Description:	Monitor bit associated with channel
+Description:	Monitor bit associated with channel. This file is available only
+		for performance critical channels (storage, network, etc.) that
+		use the monitor page mechanism.
 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 000b53e5a17a..ede858b0ee46 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -601,19 +601,12 @@ static DEVICE_ATTR_RW(driver_override);
 static struct attribute *vmbus_dev_attrs[] = {
 	&dev_attr_id.attr,
 	&dev_attr_state.attr,
-	&dev_attr_monitor_id.attr,
 	&dev_attr_class_id.attr,
 	&dev_attr_device_id.attr,
 	&dev_attr_modalias.attr,
 #ifdef CONFIG_NUMA
 	&dev_attr_numa_node.attr,
 #endif
-	&dev_attr_server_monitor_pending.attr,
-	&dev_attr_client_monitor_pending.attr,
-	&dev_attr_server_monitor_latency.attr,
-	&dev_attr_client_monitor_latency.attr,
-	&dev_attr_server_monitor_conn_id.attr,
-	&dev_attr_client_monitor_conn_id.attr,
 	&dev_attr_out_intr_mask.attr,
 	&dev_attr_out_read_index.attr,
 	&dev_attr_out_write_index.attr,
@@ -632,6 +625,22 @@ static struct attribute *vmbus_dev_attrs[] = {
 };
 ATTRIBUTE_GROUPS(vmbus_dev);
 
+/*
+ *  Set up per device monitor page attributes. These attributes are used only by
+ *  channels that use the monitor page mechanism.
+ */
+static struct attribute *vmbus_dev_monitor_attrs[] = {
+	&dev_attr_monitor_id.attr,
+	&dev_attr_server_monitor_pending.attr,
+	&dev_attr_client_monitor_pending.attr,
+	&dev_attr_server_monitor_latency.attr,
+	&dev_attr_client_monitor_latency.attr,
+	&dev_attr_server_monitor_conn_id.attr,
+	&dev_attr_client_monitor_conn_id.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(vmbus_dev_monitor);
+
 /*
  * vmbus_uevent - add uevent for our device
  *
@@ -1537,19 +1546,30 @@ static struct attribute *vmbus_chan_attrs[] = {
 	&chan_attr_read_avail.attr,
 	&chan_attr_write_avail.attr,
 	&chan_attr_cpu.attr,
-	&chan_attr_pending.attr,
-	&chan_attr_latency.attr,
 	&chan_attr_interrupts.attr,
 	&chan_attr_events.attr,
 	&chan_attr_intr_in_full.attr,
 	&chan_attr_intr_out_empty.attr,
 	&chan_attr_out_full_first.attr,
 	&chan_attr_out_full_total.attr,
-	&chan_attr_monitor_id.attr,
 	&chan_attr_subchannel_id.attr,
 	NULL
 };
 
+/* Set up per channel monitor page attributes. These attributes are used only by
+ * channels that use the monitor page mechanism.
+ */
+static struct attribute *vmbus_chan_monitor_attrs[] = {
+	&chan_attr_pending.attr,
+	&chan_attr_latency.attr,
+	&chan_attr_monitor_id.attr,
+	NULL
+};
+
+static struct attribute_group vmbus_chan_monitor_attr_group = {
+	.attrs = vmbus_chan_monitor_attrs,
+};
+
 static struct kobj_type vmbus_chan_ktype = {
 	.sysfs_ops = &vmbus_chan_sysfs_ops,
 	.release = vmbus_chan_release,
@@ -1571,6 +1591,15 @@ int vmbus_add_channel_kobj(struct hv_device *dev, struct vmbus_channel *channel)
 	if (ret)
 		return ret;
 
+	if (channel->offermsg.monitor_allocated) {
+		ret = sysfs_create_group(kobj, &vmbus_chan_monitor_attr_group);
+		if (ret) {
+			pr_err("Unable to set up monitor page sysfs files");
+			kobject_put(kobj);
+			return ret;
+		}
+	}
+
 	kobject_uevent(kobj, KOBJ_ADD);
 
 	return 0;
@@ -1615,6 +1644,9 @@ int vmbus_device_register(struct hv_device *child_device_obj)
 	child_device_obj->device.parent = &hv_acpi_dev->dev;
 	child_device_obj->device.release = vmbus_device_release;
 
+	if (child_device_obj->channel->offermsg.monitor_allocated)
+		child_device_obj->device.groups = vmbus_dev_monitor_groups;
+
 	/*
 	 * Register with the LDM. This will kick off the driver/device
 	 * binding...which will eventually call vmbus_match() and vmbus_probe()
-- 
2.17.1

Re: [PATCH v3] Drivers: hv: vmbus: Expose monitor data when channel uses monitor pages

[Greg KH]

On Tue, Feb 26, 2019 at 12:35:30AM -0500, Kimberly Brown wrote:
> There are two methods for signaling the host: the monitor page mechanism
> and hypercalls. The monitor page mechanism is used by performance
> critical channels (storage, networking, etc.) because it provides
> improved throughput. However, latency is increased. Monitor pages are
> allocated to these channels.
> 
> Monitor pages are not allocated to channels that do not use the monitor
> page mechanism. Therefore, these channels do not have a valid monitor id
> or valid monitor page data. In these cases, some of the "_show"
> functions return incorrect data. They return an invalid monitor id and
> data that is beyond the bounds of the hv_monitor_page array fields.
> 
> The "channel->offermsg.monitor_allocated" value can be used to determine
> whether monitor pages have been allocated to a channel.
> 
> Move the device-level monitor page attributes to a separate
> attribute_group struct. If the channel uses the monitor page mechanism,
> set up the sysfs files for these attributes in vmbus_device_register().
> 
> Move the channel-level monitor page attributes to a separate
> attribute_group struct. If the channel uses the monitor page mechanism,
> set up the sysfs files for these attributes in vmbus_add_channel_kobj().
> 
> Signed-off-by: Kimberly Brown <kimbrownkd@gmail.com>
> ---
> Changes in v3:
> The monitor "_show" functions no longer return an error when a channel
> does not use the monitor page mechanism. Instead, the monitor page sysfs
> files are created only when a channel uses the monitor page mechanism.
> This change was suggested by G. Kroah-Hartman.
> 
> Note: this patch was originally patch 2/2 in a patchset. Patch 1/2 has
> already been added to char-misc-testing, so I'm not resending it.
> 
> Changes in v2:
>  - Changed the return value for cases where monitor_allocated is not set
>    to "-EINVAL".
>  - Updated the commit message to provide more details about the monitor
>    page mechanism.
>  - Updated the sysfs documentation to describe the new return value.
> 
>  Documentation/ABI/stable/sysfs-bus-vmbus | 12 ++++--
>  drivers/hv/vmbus_drv.c                   | 52 +++++++++++++++++++-----
>  2 files changed, 51 insertions(+), 13 deletions(-)
> 
> diff --git a/Documentation/ABI/stable/sysfs-bus-vmbus b/Documentation/ABI/stable/sysfs-bus-vmbus
> index 826689dcc2e6..6d5cb195b119 100644
> --- a/Documentation/ABI/stable/sysfs-bus-vmbus
> +++ b/Documentation/ABI/stable/sysfs-bus-vmbus
> @@ -81,7 +81,9 @@ What:		/sys/bus/vmbus/devices/<UUID>/channels/<N>/latency
>  Date:		September. 2017
>  KernelVersion:	4.14
>  Contact:	Stephen Hemminger <sthemmin@microsoft.com>
> -Description:	Channel signaling latency
> +Description:	Channel signaling latency. This file is available only for
> +		performance critical channels (storage, network, etc.) that use
> +		the monitor page mechanism.
>  Users:		Debugging tools
>  
>  What:		/sys/bus/vmbus/devices/<UUID>/channels/<N>/out_mask
> @@ -95,7 +97,9 @@ What:		/sys/bus/vmbus/devices/<UUID>/channels/<N>/pending
>  Date:		September. 2017
>  KernelVersion:	4.14
>  Contact:	Stephen Hemminger <sthemmin@microsoft.com>
> -Description:	Channel interrupt pending state
> +Description:	Channel interrupt pending state. This file is available only for
> +                performance critical channels (storage, network, etc.) that use
> +                the monitor page mechanism.
>  Users:		Debugging tools
>  
>  What:		/sys/bus/vmbus/devices/<UUID>/channels/<N>/read_avail
> @@ -137,7 +141,9 @@ What:		/sys/bus/vmbus/devices/<UUID>/channels/<N>/monitor_id
>  Date:		January. 2018
>  KernelVersion:	4.16
>  Contact:	Stephen Hemminger <sthemmin@microsoft.com>
> -Description:	Monitor bit associated with channel
> +Description:	Monitor bit associated with channel. This file is available only
> +		for performance critical channels (storage, network, etc.) that
> +		use the monitor page mechanism.
>  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 000b53e5a17a..ede858b0ee46 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -601,19 +601,12 @@ static DEVICE_ATTR_RW(driver_override);
>  static struct attribute *vmbus_dev_attrs[] = {
>  	&dev_attr_id.attr,
>  	&dev_attr_state.attr,
> -	&dev_attr_monitor_id.attr,
>  	&dev_attr_class_id.attr,
>  	&dev_attr_device_id.attr,
>  	&dev_attr_modalias.attr,
>  #ifdef CONFIG_NUMA
>  	&dev_attr_numa_node.attr,
>  #endif
> -	&dev_attr_server_monitor_pending.attr,
> -	&dev_attr_client_monitor_pending.attr,
> -	&dev_attr_server_monitor_latency.attr,
> -	&dev_attr_client_monitor_latency.attr,
> -	&dev_attr_server_monitor_conn_id.attr,
> -	&dev_attr_client_monitor_conn_id.attr,
>  	&dev_attr_out_intr_mask.attr,
>  	&dev_attr_out_read_index.attr,
>  	&dev_attr_out_write_index.attr,
> @@ -632,6 +625,22 @@ static struct attribute *vmbus_dev_attrs[] = {
>  };
>  ATTRIBUTE_GROUPS(vmbus_dev);
>  
> +/*
> + *  Set up per device monitor page attributes. These attributes are used only by
> + *  channels that use the monitor page mechanism.
> + */
> +static struct attribute *vmbus_dev_monitor_attrs[] = {
> +	&dev_attr_monitor_id.attr,
> +	&dev_attr_server_monitor_pending.attr,
> +	&dev_attr_client_monitor_pending.attr,
> +	&dev_attr_server_monitor_latency.attr,
> +	&dev_attr_client_monitor_latency.attr,
> +	&dev_attr_server_monitor_conn_id.attr,
> +	&dev_attr_client_monitor_conn_id.attr,
> +	NULL
> +};
> +ATTRIBUTE_GROUPS(vmbus_dev_monitor);

No need to create a special group for this and then manually add it if
you need it.  Just the is_visible() callback in the attribute instead, as
that is what it is there for.

Thanks,

greg k-h

[PATCH v4] Drivers: hv: vmbus: Expose monitor data only when monitor pages are used

[Kimberly Brown]

There are two methods for signaling the host: the monitor page mechanism
and hypercalls. The monitor page mechanism is used by performance
critical channels (storage, networking, etc.) because it provides
improved throughput. However, latency is increased. Monitor pages are
allocated to these channels.

Monitor pages are not allocated to channels that do not use the monitor
page mechanism. Therefore, these channels do not have a valid monitor id
or valid monitor page data. In these cases, some of the "_show"
functions return incorrect data. They return an invalid monitor id and
data that is beyond the bounds of the hv_monitor_page array fields.

The "channel->offermsg.monitor_allocated" value can be used to determine
whether monitor pages have been allocated to a channel.

Add "is_visible()" callback functions for the device-level and
channel-level attribute groups. These functions will hide the monitor
sysfs files when the monitor mechanism is not used.

Remove ".default_attributes" from "vmbus_chan_attrs" and create a
channel-level attribute group, "vmbus_chan_group". These changes allow
the new "is_visible()" callback function to be applied to the
channel-level attributes. Call "sysfs_create_group()" to create the
channel sysyfs files.

Signed-off-by: Kimberly Brown <kimbrownkd@gmail.com>
---
Changes in v4:
 - Added “is_visible()” callback functions for the device-level and
   channel-level attribute groups.
 - Removed the separate monitor attribute groups proposed in v3. They’re
   no longer needed because the “is_visible()” callbacks are used to
   determine the attribute visibility.
 - Removed "default_attributes" from "vmbus_chan_attrs" and created a
   channel-level attribute group.
 - Removed the "kobject_put(kobj)" call proposed in v3. The calling
   functions take care of calling "kobject_put(channel->kobj)" if an
   error is returned by "vmbus_add_channel_kobj()".
 - Updated the commit message and subject for clarity and to reflect the
   new changes in v4.

Changes in v3:
 - The monitor "_show" functions no longer return an error when a
   channel does not use the monitor page mechanism. Instead, the monitor
   page sysfs files are created only when a channel uses the monitor
   page mechanism. This change was suggested by G. Kroah-Hartman.

Note: this patch was originally patch 2/2 in a patchset. Patch 1/2 has
already been added to char-misc-testing, so I'm not resending it.

Changes in v2:
 - Changed the return value for cases where monitor_allocated is not set
   to "-EINVAL".
 - Updated the commit message to provide more details about the monitor
   page mechanism.
 - Updated the sysfs documentation to describe the new return value.

 Documentation/ABI/stable/sysfs-bus-vmbus | 12 +++--
 drivers/hv/vmbus_drv.c                   | 63 +++++++++++++++++++++++-
 2 files changed, 70 insertions(+), 5 deletions(-)

diff --git a/Documentation/ABI/stable/sysfs-bus-vmbus b/Documentation/ABI/stable/sysfs-bus-vmbus
index 826689dcc2e6..8e8d167eca31 100644
--- a/Documentation/ABI/stable/sysfs-bus-vmbus
+++ b/Documentation/ABI/stable/sysfs-bus-vmbus
@@ -81,7 +81,9 @@ What:		/sys/bus/vmbus/devices/<UUID>/channels/<N>/latency
 Date:		September. 2017
 KernelVersion:	4.14
 Contact:	Stephen Hemminger <sthemmin@microsoft.com>
-Description:	Channel signaling latency
+Description:	Channel signaling latency. This file is available only for
+		performance critical channels (storage, network, etc.) that use
+		the monitor page mechanism.
 Users:		Debugging tools
 
 What:		/sys/bus/vmbus/devices/<UUID>/channels/<N>/out_mask
@@ -95,7 +97,9 @@ What:		/sys/bus/vmbus/devices/<UUID>/channels/<N>/pending
 Date:		September. 2017
 KernelVersion:	4.14
 Contact:	Stephen Hemminger <sthemmin@microsoft.com>
-Description:	Channel interrupt pending state
+Description:	Channel interrupt pending state. This file is available only for
+		performance critical channels (storage, network, etc.) that use
+		the monitor page mechanism.
 Users:		Debugging tools
 
 What:		/sys/bus/vmbus/devices/<UUID>/channels/<N>/read_avail
@@ -137,7 +141,9 @@ What:		/sys/bus/vmbus/devices/<UUID>/channels/<N>/monitor_id
 Date:		January. 2018
 KernelVersion:	4.16
 Contact:	Stephen Hemminger <sthemmin@microsoft.com>
-Description:	Monitor bit associated with channel
+Description:	Monitor bit associated with channel. This file is available only
+		for performance critical channels (storage, network, etc.) that
+		use the monitor page mechanism.
 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 000b53e5a17a..44308220d540 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -630,7 +630,36 @@ static struct attribute *vmbus_dev_attrs[] = {
 	&dev_attr_driver_override.attr,
 	NULL,
 };
-ATTRIBUTE_GROUPS(vmbus_dev);
+
+/*
+ * Device-level attribute_group callback function. Returns the permission for
+ * each attribute, and returns 0 if an attribute is not visible.
+ */
+static umode_t vmbus_dev_attr_is_visible(struct kobject *kobj,
+					 struct attribute *attr, int idx)
+{
+	struct device *dev = kobj_to_dev(kobj);
+	const struct hv_device *hv_dev = device_to_hv_device(dev);
+
+	/* Hide the monitor attributes if the monitor mechanism is not used. */
+	if (!hv_dev->channel->offermsg.monitor_allocated &&
+	    (attr == &dev_attr_monitor_id.attr ||
+	     attr == &dev_attr_server_monitor_pending.attr ||
+	     attr == &dev_attr_client_monitor_pending.attr ||
+	     attr == &dev_attr_server_monitor_latency.attr ||
+	     attr == &dev_attr_client_monitor_latency.attr ||
+	     attr == &dev_attr_server_monitor_conn_id.attr ||
+	     attr == &dev_attr_client_monitor_conn_id.attr))
+		return 0;
+
+	return attr->mode;
+}
+
+static const struct attribute_group vmbus_dev_group = {
+	.attrs = vmbus_dev_attrs,
+	.is_visible = vmbus_dev_attr_is_visible
+};
+__ATTRIBUTE_GROUPS(vmbus_dev);
 
 /*
  * vmbus_uevent - add uevent for our device
@@ -1550,10 +1579,34 @@ static struct attribute *vmbus_chan_attrs[] = {
 	NULL
 };
 
+/*
+ * Channel-level attribute_group callback function. Returns the permission for
+ * each attribute, and returns 0 if an attribute is not visible.
+ */
+static umode_t vmbus_chan_attr_is_visible(struct kobject *kobj,
+					  struct attribute *attr, int idx)
+{
+	const struct vmbus_channel *channel =
+		container_of(kobj, struct vmbus_channel, kobj);
+
+	/* Hide the monitor attributes if the monitor mechanism is not used. */
+	if (!channel->offermsg.monitor_allocated &&
+	    (attr == &chan_attr_pending.attr ||
+	     attr == &chan_attr_latency.attr ||
+	     attr == &chan_attr_monitor_id.attr))
+		return 0;
+
+	return attr->mode;
+}
+
+static struct attribute_group vmbus_chan_group = {
+	.attrs = vmbus_chan_attrs,
+	.is_visible = vmbus_chan_attr_is_visible
+};
+
 static struct kobj_type vmbus_chan_ktype = {
 	.sysfs_ops = &vmbus_chan_sysfs_ops,
 	.release = vmbus_chan_release,
-	.default_attrs = vmbus_chan_attrs,
 };
 
 /*
@@ -1571,6 +1624,12 @@ int vmbus_add_channel_kobj(struct hv_device *dev, struct vmbus_channel *channel)
 	if (ret)
 		return ret;
 
+	ret = sysfs_create_group(kobj, &vmbus_chan_group);
+	if (ret) {
+		pr_err("Unable to set up channel sysfs files\n");
+		return ret;
+	}
+
 	kobject_uevent(kobj, KOBJ_ADD);
 
 	return 0;
-- 
2.17.1

RE: [PATCH v4] Drivers: hv: vmbus: Expose monitor data only when monitor pages are used

[Michael Kelley]

From: Kimberly Brown <kimbrownkd@gmail.com>  Sent: Friday, March 1, 2019 11:18 AM
> 
> +/*
> + * Channel-level attribute_group callback function. Returns the permission for
> + * each attribute, and returns 0 if an attribute is not visible.
> + */
> +static umode_t vmbus_chan_attr_is_visible(struct kobject *kobj,
> +					  struct attribute *attr, int idx)
> +{
> +	const struct vmbus_channel *channel =
> +		container_of(kobj, struct vmbus_channel, kobj);
> +
> +	/* Hide the monitor attributes if the monitor mechanism is not used. */
> +	if (!channel->offermsg.monitor_allocated &&
> +	    (attr == &chan_attr_pending.attr ||
> +	     attr == &chan_attr_latency.attr ||
> +	     attr == &chan_attr_monitor_id.attr))
> +		return 0;
> +
> +	return attr->mode;
> +}
> +
> +static struct attribute_group vmbus_chan_group = {
> +	.attrs = vmbus_chan_attrs,
> +	.is_visible = vmbus_chan_attr_is_visible
> +};
> +
>  static struct kobj_type vmbus_chan_ktype = {
>  	.sysfs_ops = &vmbus_chan_sysfs_ops,
>  	.release = vmbus_chan_release,
> -	.default_attrs = vmbus_chan_attrs,

Just to double-check my understanding, removing the default_attrs
here means that in vmbus_add_channel_kobj(), the call to
kobject_init_and_add() will only create the sub-directory that is the
channel number.  The sub-directory will be empty.  Then the new
call to sysfs_create_group() that you added below will populate
the subdirectory, as filtered by vmbus_chan_attr_is_visible().

>  };
> 
>  /*
> @@ -1571,6 +1624,12 @@ int vmbus_add_channel_kobj(struct hv_device *dev, struct
> vmbus_channel *channel)
>  	if (ret)
>  		return ret;
> 
> +	ret = sysfs_create_group(kobj, &vmbus_chan_group);
> +	if (ret) {
> +		pr_err("Unable to set up channel sysfs files\n");
> +		return ret;

In this error case, the previously created sub-directory that is the
channel number needs to be deleted/cleaned up.

> +	}
> +

Michael

Re: [PATCH v4] Drivers: hv: vmbus: Expose monitor data only when monitor pages are used

[Kimberly Brown]

On Sat, Mar 02, 2019 at 06:39:30PM +0000, Michael Kelley wrote:
> From: Kimberly Brown <kimbrownkd@gmail.com>  Sent: Friday, March 1, 2019 11:18 AM
> > 
> > +/*
> > + * Channel-level attribute_group callback function. Returns the permission for
> > + * each attribute, and returns 0 if an attribute is not visible.
> > + */
> > +static umode_t vmbus_chan_attr_is_visible(struct kobject *kobj,
> > +					  struct attribute *attr, int idx)
> > +{
> > +	const struct vmbus_channel *channel =
> > +		container_of(kobj, struct vmbus_channel, kobj);
> > +
> > +	/* Hide the monitor attributes if the monitor mechanism is not used. */
> > +	if (!channel->offermsg.monitor_allocated &&
> > +	    (attr == &chan_attr_pending.attr ||
> > +	     attr == &chan_attr_latency.attr ||
> > +	     attr == &chan_attr_monitor_id.attr))
> > +		return 0;
> > +
> > +	return attr->mode;
> > +}
> > +
> > +static struct attribute_group vmbus_chan_group = {
> > +	.attrs = vmbus_chan_attrs,
> > +	.is_visible = vmbus_chan_attr_is_visible
> > +};
> > +
> >  static struct kobj_type vmbus_chan_ktype = {
> >  	.sysfs_ops = &vmbus_chan_sysfs_ops,
> >  	.release = vmbus_chan_release,
> > -	.default_attrs = vmbus_chan_attrs,
> 
> Just to double-check my understanding, removing the default_attrs
> here means that in vmbus_add_channel_kobj(), the call to
> kobject_init_and_add() will only create the sub-directory that is the
> channel number.  The sub-directory will be empty.  Then the new
> call to sysfs_create_group() that you added below will populate
> the subdirectory, as filtered by vmbus_chan_attr_is_visible().

Yes, that's my understanding as well.

> 
> >  };
> > 
> >  /*
> > @@ -1571,6 +1624,12 @@ int vmbus_add_channel_kobj(struct hv_device *dev, struct
> > vmbus_channel *channel)
> >  	if (ret)
> >  		return ret;
> > 
> > +	ret = sysfs_create_group(kobj, &vmbus_chan_group);
> > +	if (ret) {
> > +		pr_err("Unable to set up channel sysfs files\n");
> > +		return ret;
> 
> In this error case, the previously created sub-directory that is the
> channel number needs to be deleted/cleaned up.

I was going to let the kobject and sysfs systems take care of that. If
vmbus_add_channel_kobj() returns an error to the calling functions, they
call kobject_put(), which eventually removes the directory.

There are two functions that call vmbus_add_channel_kobj():
vmbus_add_channel_work() and vmbus_device_register(). If
vmbus_add_channel_kobj() returns an error to vmbus_add_channel_work(),
the function calls are:

free_channel() => kobject_put() (channel kobj ref. count should now be
0) => kobject_release() => kobject_cleanup() => kobject_del() =>
sysfs_remove_dir()

If vmbus_add_channel_kobject() returns an error to
vmbus_device_register(), the device's sub-directory is removed:

device_unregister() => put_device() => kobject_put() (device kobj ref.
count should now be 0) => kobject_release() => ...


So, I don't think its necessary to cleanup the subdirectory created by
kobject_init_and_add() here.

Thanks,
Kim


> 
> > +	}
> > +
> 
> Michael

Re: [PATCH v4] Drivers: hv: vmbus: Expose monitor data only when monitor pages are used

[Greg KH]

On Fri, Mar 01, 2019 at 02:18:24PM -0500, Kimberly Brown wrote:
> +/*
> + * Channel-level attribute_group callback function. Returns the permission for
> + * each attribute, and returns 0 if an attribute is not visible.
> + */
> +static umode_t vmbus_chan_attr_is_visible(struct kobject *kobj,
> +					  struct attribute *attr, int idx)
> +{
> +	const struct vmbus_channel *channel =
> +		container_of(kobj, struct vmbus_channel, kobj);
> +
> +	/* Hide the monitor attributes if the monitor mechanism is not used. */
> +	if (!channel->offermsg.monitor_allocated &&
> +	    (attr == &chan_attr_pending.attr ||
> +	     attr == &chan_attr_latency.attr ||
> +	     attr == &chan_attr_monitor_id.attr))
> +		return 0;
> +
> +	return attr->mode;
> +}
> +
> +static struct attribute_group vmbus_chan_group = {
> +	.attrs = vmbus_chan_attrs,
> +	.is_visible = vmbus_chan_attr_is_visible
> +};
> +
>  static struct kobj_type vmbus_chan_ktype = {
>  	.sysfs_ops = &vmbus_chan_sysfs_ops,
>  	.release = vmbus_chan_release,
> -	.default_attrs = vmbus_chan_attrs,

Why did you remove this line?

>  };
>  
>  /*
> @@ -1571,6 +1624,12 @@ int vmbus_add_channel_kobj(struct hv_device *dev, struct vmbus_channel *channel)
>  	if (ret)
>  		return ret;
>  
> +	ret = sysfs_create_group(kobj, &vmbus_chan_group);

Why are you adding these "by hand"?  What was wrong with using the
default attribute group pointer?  You also are not removing the
attributes :(

greg k-h

Re: [PATCH v4] Drivers: hv: vmbus: Expose monitor data only when monitor pages are used

[Kimberly Brown]

On Sun, Mar 03, 2019 at 09:05:43AM +0100, Greg KH wrote:
> On Fri, Mar 01, 2019 at 02:18:24PM -0500, Kimberly Brown wrote:
> > +/*
> > + * Channel-level attribute_group callback function. Returns the permission for
> > + * each attribute, and returns 0 if an attribute is not visible.
> > + */
> > +static umode_t vmbus_chan_attr_is_visible(struct kobject *kobj,
> > +					  struct attribute *attr, int idx)
> > +{
> > +	const struct vmbus_channel *channel =
> > +		container_of(kobj, struct vmbus_channel, kobj);
> > +
> > +	/* Hide the monitor attributes if the monitor mechanism is not used. */
> > +	if (!channel->offermsg.monitor_allocated &&
> > +	    (attr == &chan_attr_pending.attr ||
> > +	     attr == &chan_attr_latency.attr ||
> > +	     attr == &chan_attr_monitor_id.attr))
> > +		return 0;
> > +
> > +	return attr->mode;
> > +}
> > +
> > +static struct attribute_group vmbus_chan_group = {
> > +	.attrs = vmbus_chan_attrs,
> > +	.is_visible = vmbus_chan_attr_is_visible
> > +};
> > +
> >  static struct kobj_type vmbus_chan_ktype = {
> >  	.sysfs_ops = &vmbus_chan_sysfs_ops,
> >  	.release = vmbus_chan_release,
> > -	.default_attrs = vmbus_chan_attrs,
> 
> Why did you remove this line?
 
I removed the default attributes because vmbus_chan_attrs contains
non-default attributes. You suggested that I use one attribute_group and
an is_visible() callback for the device-level attributes (see
https://lore.kernel.org/lkml/20190226081848.GA15659@kroah.com/). I
assumed (possibly incorrectly) that I should do the same for these
channel-level attributes.


> >  };
> >  
> >  /*
> > @@ -1571,6 +1624,12 @@ int vmbus_add_channel_kobj(struct hv_device *dev, struct vmbus_channel *channel)
> >  	if (ret)
> >  		return ret;
> >  
> > +	ret = sysfs_create_group(kobj, &vmbus_chan_group);
> 
> Why are you adding these "by hand"?  What was wrong with using the
> default attribute group pointer?  You also are not removing the
> attributes :(

Are you referring to default_attrs in kobj_type? It's not an
attribute_group pointer, it's a pointer to an attribute pointer array.
The problem with using default_attrs is that all of the attributes are
visible.

I'm fairly certain that the monitor attributes are being removed.
sysfs_create_group() uses the attribute_group's is_visible() callback to
control the attribute visibility. And, when I look at the sysfs files, I
can see that the monitor sysyfs files are removed.

In v3, I proposed moving the monitor attributes to a special
attribute_group and adding that group manually when needed. Do you
prefer that approach for the channel-level monitor attributes?

Thanks,
Kim


> 
> greg k-h

Re: [PATCH v4] Drivers: hv: vmbus: Expose monitor data only when monitor pages are used

[Greg KH]

On Sun, Mar 03, 2019 at 04:11:28PM -0500, Kimberly Brown wrote:
> On Sun, Mar 03, 2019 at 09:05:43AM +0100, Greg KH wrote:
> > On Fri, Mar 01, 2019 at 02:18:24PM -0500, Kimberly Brown wrote:
> > > +/*
> > > + * Channel-level attribute_group callback function. Returns the permission for
> > > + * each attribute, and returns 0 if an attribute is not visible.
> > > + */
> > > +static umode_t vmbus_chan_attr_is_visible(struct kobject *kobj,
> > > +					  struct attribute *attr, int idx)
> > > +{
> > > +	const struct vmbus_channel *channel =
> > > +		container_of(kobj, struct vmbus_channel, kobj);
> > > +
> > > +	/* Hide the monitor attributes if the monitor mechanism is not used. */
> > > +	if (!channel->offermsg.monitor_allocated &&
> > > +	    (attr == &chan_attr_pending.attr ||
> > > +	     attr == &chan_attr_latency.attr ||
> > > +	     attr == &chan_attr_monitor_id.attr))
> > > +		return 0;
> > > +
> > > +	return attr->mode;
> > > +}
> > > +
> > > +static struct attribute_group vmbus_chan_group = {
> > > +	.attrs = vmbus_chan_attrs,
> > > +	.is_visible = vmbus_chan_attr_is_visible
> > > +};
> > > +
> > >  static struct kobj_type vmbus_chan_ktype = {
> > >  	.sysfs_ops = &vmbus_chan_sysfs_ops,
> > >  	.release = vmbus_chan_release,
> > > -	.default_attrs = vmbus_chan_attrs,
> > 
> > Why did you remove this line?
>  
> I removed the default attributes because vmbus_chan_attrs contains
> non-default attributes. You suggested that I use one attribute_group and
> an is_visible() callback for the device-level attributes (see
> https://lore.kernel.org/lkml/20190226081848.GA15659@kroah.com/). I
> assumed (possibly incorrectly) that I should do the same for these
> channel-level attributes.

That is fine to have a callback, but why did you have to remove the
default attribute pointer?  The two should have nothing to do with each
other.

> > >  };
> > >  
> > >  /*
> > > @@ -1571,6 +1624,12 @@ int vmbus_add_channel_kobj(struct hv_device *dev, struct vmbus_channel *channel)
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > +	ret = sysfs_create_group(kobj, &vmbus_chan_group);
> > 
> > Why are you adding these "by hand"?  What was wrong with using the
> > default attribute group pointer?  You also are not removing the
> > attributes :(
> 
> Are you referring to default_attrs in kobj_type? It's not an
> attribute_group pointer, it's a pointer to an attribute pointer array.
> The problem with using default_attrs is that all of the attributes are
> visible.

It shouldn't, the is_visable() callback will be called on each attribute
when the group is created by the core.  Did that not work properly when
you tested this?

> I'm fairly certain that the monitor attributes are being removed.
> sysfs_create_group() uses the attribute_group's is_visible() callback to
> control the attribute visibility. And, when I look at the sysfs files, I
> can see that the monitor sysyfs files are removed.

I mean you are not removing the group when the device goes away, not
that the individual files are not present.  If you leave the pointer to
default_attributes there, the core will properly remove the sysfs
attributes when the device is removed from the system.  Otherwise you
just "get lucky" if the attributes are removed or not.

> In v3, I proposed moving the monitor attributes to a special
> attribute_group and adding that group manually when needed. Do you
> prefer that approach for the channel-level monitor attributes?

No need for a special group here, just use the is_visable() callback
like you currently are, all should be fine.  I think you are adding more
code than you need to in order to get this to all work properly :)

thanks,

greg k-h

[PATCH v5] Drivers: hv: vmbus: Expose monitor data only when monitor pages are used

[Kimberly Brown]

There are two methods for signaling the host: the monitor page mechanism
and hypercalls. The monitor page mechanism is used by performance
critical channels (storage, networking, etc.) because it provides
improved throughput. However, latency is increased. Monitor pages are
allocated to these channels.

Monitor pages are not allocated to channels that do not use the monitor
page mechanism. Therefore, these channels do not have a valid monitor id
or valid monitor page data. In these cases, some of the "_show"
functions return incorrect data. They return an invalid monitor id and
data that is beyond the bounds of the hv_monitor_page array fields.

The "channel->offermsg.monitor_allocated" value can be used to determine
whether monitor pages have been allocated to a channel.

Add "is_visible()" callback functions for the device-level and
channel-level attribute groups. These functions will hide the monitor
sysfs files when the monitor mechanism is not used.

Remove ".default_attributes" from "vmbus_chan_attrs" and create a
channel-level attribute group, "vmbus_chan_group". These changes allow
the new "is_visible()" callback function to be applied to the
channel-level attributes. Call "sysfs_create_group()" to create the
channel sysyfs files.

Add the “bool sysfs_group_ready” field to the vmbus_channel struct.
This field is used to ensure that the attributes in the
“vmbus_chan_group” attribute group have been created. Add the
“vmbus_remove_channel_attr_group()” function, which calls
"sysfs_remove_group()" on “vmbus_chan_group” if the attributes were
created.

Signed-off-by: Kimberly Brown <kimbrownkd@gmail.com>
---

Changes in v5: 
 - Added the “bool sysfs_group_ready” field to vmbus_channel.
 - Added the “vmbus_remove_channel_attr_group()” function which calls
   "sysfs_remove_group()".
 - Added a comment to "vmbus_add_channel_kobj()" to describe how the
   empty directory is removed if "sysfs_create_group()" returns an
   error.
 - Updated the commit message.

NOTE: “.default_attrs” must be removed from vmbus_chan_ktype in order to
use the is_visible() function because "default_attrs" is an array of
attributes, not an attribute_group.

Changes in v4:
 - Added “is_visible()” callback functions for the device-level and
   channel-level attribute groups.
 - Removed the separate monitor attribute groups proposed in v3. They’re
   no longer needed because the “is_visible()” callbacks are used to
   determine the attribute visibility.
 - Removed "default_attributes" from "vmbus_chan_attrs" and created a
   channel-level attribute group.
 - Removed the "kobject_put(kobj)" call proposed in v3. The calling
   functions take care of calling "kobject_put(channel->kobj)" if an
   error is returned by "vmbus_add_channel_kobj()".
 - Updated the commit message and subject for clarity and to reflect the
   new changes in v4.

Changes in v3:
 - The monitor "_show" functions no longer return an error when a
   channel does not use the monitor page mechanism. Instead, the monitor
   page sysfs files are created only when a channel uses the monitor
   page mechanism. This change was suggested by G. Kroah-Hartman.

Note: this patch was originally patch 2/2 in a patchset. Patch 1/2 has
already been added to char-misc-testing, so I'm not resending it.

Changes in v2:
 - Changed the return value for cases where monitor_allocated is not set
   to "-EINVAL".
 - Updated the commit message to provide more details about the monitor
   page mechanism.
 - Updated the sysfs documentation to describe the new return value.

 Documentation/ABI/stable/sysfs-bus-vmbus | 12 +++-
 drivers/hv/channel_mgmt.c                |  3 +
 drivers/hv/hyperv_vmbus.h                |  2 +
 drivers/hv/vmbus_drv.c                   | 80 +++++++++++++++++++++++-
 include/linux/hyperv.h                   |  6 ++
 5 files changed, 98 insertions(+), 5 deletions(-)

diff --git a/Documentation/ABI/stable/sysfs-bus-vmbus b/Documentation/ABI/stable/sysfs-bus-vmbus
index 826689dcc2e6..8e8d167eca31 100644
--- a/Documentation/ABI/stable/sysfs-bus-vmbus
+++ b/Documentation/ABI/stable/sysfs-bus-vmbus
@@ -81,7 +81,9 @@ What:		/sys/bus/vmbus/devices/<UUID>/channels/<N>/latency
 Date:		September. 2017
 KernelVersion:	4.14
 Contact:	Stephen Hemminger <sthemmin@microsoft.com>
-Description:	Channel signaling latency
+Description:	Channel signaling latency. This file is available only for
+		performance critical channels (storage, network, etc.) that use
+		the monitor page mechanism.
 Users:		Debugging tools
 
 What:		/sys/bus/vmbus/devices/<UUID>/channels/<N>/out_mask
@@ -95,7 +97,9 @@ What:		/sys/bus/vmbus/devices/<UUID>/channels/<N>/pending
 Date:		September. 2017
 KernelVersion:	4.14
 Contact:	Stephen Hemminger <sthemmin@microsoft.com>
-Description:	Channel interrupt pending state
+Description:	Channel interrupt pending state. This file is available only for
+		performance critical channels (storage, network, etc.) that use
+		the monitor page mechanism.
 Users:		Debugging tools
 
 What:		/sys/bus/vmbus/devices/<UUID>/channels/<N>/read_avail
@@ -137,7 +141,9 @@ What:		/sys/bus/vmbus/devices/<UUID>/channels/<N>/monitor_id
 Date:		January. 2018
 KernelVersion:	4.16
 Contact:	Stephen Hemminger <sthemmin@microsoft.com>
-Description:	Monitor bit associated with channel
+Description:	Monitor bit associated with channel. This file is available only
+		for performance critical channels (storage, network, etc.) that
+		use the monitor page mechanism.
 Users:		Debugging tools and userspace drivers
 
 What:		/sys/bus/vmbus/devices/<UUID>/channels/<N>/ring
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 62703b354d6d..e4b1f0db8159 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -346,6 +346,9 @@ static void free_channel(struct vmbus_channel *channel)
 {
 	tasklet_kill(&channel->callback_event);
 
+	/* Remove the channel sysfs attribute group */
+	vmbus_remove_channel_attr_group(channel);
+
 	kobject_put(&channel->kobj);
 }
 
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index cb86b133eb4d..a94aab94e0b5 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -321,6 +321,8 @@ void vmbus_device_unregister(struct hv_device *device_obj);
 int vmbus_add_channel_kobj(struct hv_device *device_obj,
 			   struct vmbus_channel *channel);
 
+void vmbus_remove_channel_attr_group(struct vmbus_channel *channel);
+
 struct vmbus_channel *relid2channel(u32 relid);
 
 void vmbus_free_channels(void);
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 000b53e5a17a..43a17e17f670 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -630,7 +630,36 @@ static struct attribute *vmbus_dev_attrs[] = {
 	&dev_attr_driver_override.attr,
 	NULL,
 };
-ATTRIBUTE_GROUPS(vmbus_dev);
+
+/*
+ * Device-level attribute_group callback function. Returns the permission for
+ * each attribute, and returns 0 if an attribute is not visible.
+ */
+static umode_t vmbus_dev_attr_is_visible(struct kobject *kobj,
+					 struct attribute *attr, int idx)
+{
+	struct device *dev = kobj_to_dev(kobj);
+	const struct hv_device *hv_dev = device_to_hv_device(dev);
+
+	/* Hide the monitor attributes if the monitor mechanism is not used. */
+	if (!hv_dev->channel->offermsg.monitor_allocated &&
+	    (attr == &dev_attr_monitor_id.attr ||
+	     attr == &dev_attr_server_monitor_pending.attr ||
+	     attr == &dev_attr_client_monitor_pending.attr ||
+	     attr == &dev_attr_server_monitor_latency.attr ||
+	     attr == &dev_attr_client_monitor_latency.attr ||
+	     attr == &dev_attr_server_monitor_conn_id.attr ||
+	     attr == &dev_attr_client_monitor_conn_id.attr))
+		return 0;
+
+	return attr->mode;
+}
+
+static const struct attribute_group vmbus_dev_group = {
+	.attrs = vmbus_dev_attrs,
+	.is_visible = vmbus_dev_attr_is_visible
+};
+__ATTRIBUTE_GROUPS(vmbus_dev);
 
 /*
  * vmbus_uevent - add uevent for our device
@@ -1550,10 +1579,34 @@ static struct attribute *vmbus_chan_attrs[] = {
 	NULL
 };
 
+/*
+ * Channel-level attribute_group callback function. Returns the permission for
+ * each attribute, and returns 0 if an attribute is not visible.
+ */
+static umode_t vmbus_chan_attr_is_visible(struct kobject *kobj,
+					  struct attribute *attr, int idx)
+{
+	const struct vmbus_channel *channel =
+		container_of(kobj, struct vmbus_channel, kobj);
+
+	/* Hide the monitor attributes if the monitor mechanism is not used. */
+	if (!channel->offermsg.monitor_allocated &&
+	    (attr == &chan_attr_pending.attr ||
+	     attr == &chan_attr_latency.attr ||
+	     attr == &chan_attr_monitor_id.attr))
+		return 0;
+
+	return attr->mode;
+}
+
+static struct attribute_group vmbus_chan_group = {
+	.attrs = vmbus_chan_attrs,
+	.is_visible = vmbus_chan_attr_is_visible
+};
+
 static struct kobj_type vmbus_chan_ktype = {
 	.sysfs_ops = &vmbus_chan_sysfs_ops,
 	.release = vmbus_chan_release,
-	.default_attrs = vmbus_chan_attrs,
 };
 
 /*
@@ -1571,11 +1624,34 @@ int vmbus_add_channel_kobj(struct hv_device *dev, struct vmbus_channel *channel)
 	if (ret)
 		return ret;
 
+	ret = sysfs_create_group(kobj, &vmbus_chan_group);
+	channel->sysfs_group_ready = !ret;
+
+	if (ret) {
+		/*
+		 * If an error is returned to the calling functions, those
+		 * functions will call kobject_put() on the channel kobject,
+		 * which will cleanup the empty channel directory created by
+		 * kobject_init_and_add().
+		 */
+		pr_err("Unable to set up channel sysfs files\n");
+		return ret;
+	}
+
 	kobject_uevent(kobj, KOBJ_ADD);
 
 	return 0;
 }
 
+/*
+ * vmbus_remove_channel_attr_group - remove the channel's attribute group
+ */
+void vmbus_remove_channel_attr_group(struct vmbus_channel *channel)
+{
+	if (channel->sysfs_group_ready)
+		sysfs_remove_group(&channel->kobj, &vmbus_chan_group);
+}
+
 /*
  * vmbus_device_create - Creates and registers a new child device
  * on the vmbus.
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 64698ec8f2ac..604a2e05af47 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -934,6 +934,12 @@ struct vmbus_channel {
 	 * full outbound ring buffer.
 	 */
 	u64 out_full_first;
+
+	/*
+	 * Indicates whether the channel's attribute group sysfs files have
+	 * been successfully created.
+	 */
+	bool sysfs_group_ready;
 };
 
 static inline bool is_hvsock_channel(const struct vmbus_channel *c)
-- 
2.17.1

Re: [PATCH v5] Drivers: hv: vmbus: Expose monitor data only when monitor pages are used

[Greg KH]

On Fri, Mar 08, 2019 at 05:46:11PM -0500, Kimberly Brown wrote:
>  static struct kobj_type vmbus_chan_ktype = {
>  	.sysfs_ops = &vmbus_chan_sysfs_ops,
>  	.release = vmbus_chan_release,
> -	.default_attrs = vmbus_chan_attrs,

As discussed on IRC, a kobj_type needs to get an attribute group one of
these days, not just a attribute list :)

So thanks for persisting with this, sorry for the earlier review
comments, you were totally right about this, nice work.

Minor review comments below:

>  };
>  
>  /*
> @@ -1571,11 +1624,34 @@ int vmbus_add_channel_kobj(struct hv_device *dev, struct vmbus_channel *channel)
>  	if (ret)
>  		return ret;
>  
> +	ret = sysfs_create_group(kobj, &vmbus_chan_group);
> +	channel->sysfs_group_ready = !ret;

Why do you need this flag?

> +
> +	if (ret) {
> +		/*
> +		 * If an error is returned to the calling functions, those
> +		 * functions will call kobject_put() on the channel kobject,
> +		 * which will cleanup the empty channel directory created by
> +		 * kobject_init_and_add().

Why is this comment needed?

> +		 */
> +		pr_err("Unable to set up channel sysfs files\n");

dev_err() to show who had the problem with the files?

> +		return ret;
> +	}
> +
>  	kobject_uevent(kobj, KOBJ_ADD);
>  
>  	return 0;
>  }
>  
> +/*
> + * vmbus_remove_channel_attr_group - remove the channel's attribute group
> + */
> +void vmbus_remove_channel_attr_group(struct vmbus_channel *channel)
> +{
> +	if (channel->sysfs_group_ready)
> +		sysfs_remove_group(&channel->kobj, &vmbus_chan_group);

You should be able to just always remove these, no need for a flag to
say you have created them or not, right?

thanks,

greg k-h

Re: [PATCH v5] Drivers: hv: vmbus: Expose monitor data only when monitor pages are used

[Kimberly Brown]

On Sat, Mar 09, 2019 at 08:21:35AM +0100, Greg KH wrote:
> On Fri, Mar 08, 2019 at 05:46:11PM -0500, Kimberly Brown wrote:
> >  static struct kobj_type vmbus_chan_ktype = {
> >  	.sysfs_ops = &vmbus_chan_sysfs_ops,
> >  	.release = vmbus_chan_release,
> > -	.default_attrs = vmbus_chan_attrs,
> 
> As discussed on IRC, a kobj_type needs to get an attribute group one of
> these days, not just a attribute list :)
> 
> So thanks for persisting with this, sorry for the earlier review
> comments, you were totally right about this, nice work.
> 
> Minor review comments below:
> 
> >  };
> >  
> >  /*
> > @@ -1571,11 +1624,34 @@ int vmbus_add_channel_kobj(struct hv_device *dev, struct vmbus_channel *channel)
> >  	if (ret)
> >  		return ret;
> >  
> > +	ret = sysfs_create_group(kobj, &vmbus_chan_group);
> > +	channel->sysfs_group_ready = !ret;
> 
> Why do you need this flag?
> 

I added this flag to prevent sysfs_remove_group() from being called if
sysfs_create_group() or the earlier call to kobject_init_and_add()
failed. However, it looks like that would just lead to WARN() being
called, which seems appropriate. I'll remove this flag.


> > +
> > +	if (ret) {
> > +		/*
> > +		 * If an error is returned to the calling functions, those
> > +		 * functions will call kobject_put() on the channel kobject,
> > +		 * which will cleanup the empty channel directory created by
> > +		 * kobject_init_and_add().
> 
> Why is this comment needed?
> 

Another reviewer asked why the empty directory created by
kobject_init_and_add() isn't removed here when sysfs_create_group()
fails. We decided that a comment would help clear up any future
confusion.


> > +		 */
> > +		pr_err("Unable to set up channel sysfs files\n");
> 
> dev_err() to show who had the problem with the files?
>

Yes, good point. I'll change this.


> > +		return ret;
> > +	}
> > +
> >  	kobject_uevent(kobj, KOBJ_ADD);
> >  
> >  	return 0;
> >  }
> >  
> > +/*
> > + * vmbus_remove_channel_attr_group - remove the channel's attribute group
> > + */
> > +void vmbus_remove_channel_attr_group(struct vmbus_channel *channel)
> > +{
> > +	if (channel->sysfs_group_ready)
> > +		sysfs_remove_group(&channel->kobj, &vmbus_chan_group);
> 
> You should be able to just always remove these, no need for a flag to
> say you have created them or not, right?
> 
> thanks,
> 
> greg k-h

[PATCH v6] Drivers: hv: vmbus: Expose monitor data only when monitor pages are used

[Kimberly Brown]

There are two methods for signaling the host: the monitor page mechanism
and hypercalls. The monitor page mechanism is used by performance
critical channels (storage, networking, etc.) because it provides
improved throughput. However, latency is increased. Monitor pages are
allocated to these channels.

Monitor pages are not allocated to channels that do not use the monitor
page mechanism. Therefore, these channels do not have a valid monitor id
or valid monitor page data. In these cases, some of the "_show"
functions return incorrect data. They return an invalid monitor id and
data that is beyond the bounds of the hv_monitor_page array fields.

The "channel->offermsg.monitor_allocated" value can be used to determine
whether monitor pages have been allocated to a channel.

Add "is_visible()" callback functions for the device-level and
channel-level attribute groups. These functions will hide the monitor
sysfs files when the monitor mechanism is not used.

Remove ".default_attributes" from "vmbus_chan_attrs" and create a
channel-level attribute group. These changes allow the new
"is_visible()" callback function to be applied to the channel-level
attributes.

Call "sysfs_create_group()" in "vmbus_add_channel_kobj()" to create the
channel's sysfs files. Add a new function,
“vmbus_remove_channel_attr_group()”, and call it in "free_channel()" to
remove the channel's sysfs files when the channel is closed.

Signed-off-by: Kimberly Brown <kimbrownkd@gmail.com>
---

Changes in v6:
 - Removed the “sysfs_group_ready” flag proposed in v5. The flag was
   used to verify that the attribute group was created, which is
   unnecessary; sysfs_remove_group() can be called even when
   sysfs_create_group() fails.
 - Replaced pr_err() with dev_err() as suggested by G. Kroah-Hartman.
 - Shortened the error path comment in vmbus_add_channel_kobj().
 - Updated the commit message.

Changes in v5:
 - Added the “bool sysfs_group_ready” field to vmbus_channel.
 - Added the “vmbus_remove_channel_attr_group()” function which calls
   "sysfs_remove_group()".
 - Added a comment to "vmbus_add_channel_kobj()" to describe how the
   empty directory is removed if "sysfs_create_group()" returns an
   error.
 - Updated the commit message.

NOTE: “.default_attrs” must be removed from vmbus_chan_ktype in order to
use the is_visible() function because "default_attrs" is an array of
attributes, not an attribute_group.

Changes in v4:
 - Added “is_visible()” callback functions for the device-level and
   channel-level attribute groups.
 - Removed the separate monitor attribute groups proposed in v3. They’re
   no longer needed because the “is_visible()” callbacks are used to
   determine the attribute visibility.
 - Removed "default_attributes" from "vmbus_chan_attrs" and created a
   channel-level attribute group.
 - Removed the "kobject_put(kobj)" call proposed in v3. The calling
   functions take care of calling "kobject_put(channel->kobj)" if an
   error is returned by "vmbus_add_channel_kobj()".
 - Updated the commit message and subject for clarity and to reflect the
   new changes in v4.

Changes in v3:
 - The monitor "_show" functions no longer return an error when a
   channel does not use the monitor page mechanism. Instead, the monitor
   page sysfs files are created only when a channel uses the monitor
   page mechanism. This change was suggested by G. Kroah-Hartman.

Note: this patch was originally patch 2/2 in a patchset. Patch 1/2 has
already been added to char-misc-testing, so I'm not resending it.

Changes in v2:
 - Changed the return value for cases where monitor_allocated is not set
   to "-EINVAL".
 - Updated the commit message to provide more details about the monitor
   page mechanism.
 - Updated the sysfs documentation to describe the new return value.


Documentation/ABI/stable/sysfs-bus-vmbus | 12 +++-
 drivers/hv/channel_mgmt.c                |  1 +
 drivers/hv/hyperv_vmbus.h                |  2 +
 drivers/hv/vmbus_drv.c                   | 77 +++++++++++++++++++++++-
 4 files changed, 87 insertions(+), 5 deletions(-)

diff --git a/Documentation/ABI/stable/sysfs-bus-vmbus b/Documentation/ABI/stable/sysfs-bus-vmbus
index 826689dcc2e6..8e8d167eca31 100644
--- a/Documentation/ABI/stable/sysfs-bus-vmbus
+++ b/Documentation/ABI/stable/sysfs-bus-vmbus
@@ -81,7 +81,9 @@ What:		/sys/bus/vmbus/devices/<UUID>/channels/<N>/latency
 Date:		September. 2017
 KernelVersion:	4.14
 Contact:	Stephen Hemminger <sthemmin@microsoft.com>
-Description:	Channel signaling latency
+Description:	Channel signaling latency. This file is available only for
+		performance critical channels (storage, network, etc.) that use
+		the monitor page mechanism.
 Users:		Debugging tools
 
 What:		/sys/bus/vmbus/devices/<UUID>/channels/<N>/out_mask
@@ -95,7 +97,9 @@ What:		/sys/bus/vmbus/devices/<UUID>/channels/<N>/pending
 Date:		September. 2017
 KernelVersion:	4.14
 Contact:	Stephen Hemminger <sthemmin@microsoft.com>
-Description:	Channel interrupt pending state
+Description:	Channel interrupt pending state. This file is available only for
+		performance critical channels (storage, network, etc.) that use
+		the monitor page mechanism.
 Users:		Debugging tools
 
 What:		/sys/bus/vmbus/devices/<UUID>/channels/<N>/read_avail
@@ -137,7 +141,9 @@ What:		/sys/bus/vmbus/devices/<UUID>/channels/<N>/monitor_id
 Date:		January. 2018
 KernelVersion:	4.16
 Contact:	Stephen Hemminger <sthemmin@microsoft.com>
-Description:	Monitor bit associated with channel
+Description:	Monitor bit associated with channel. This file is available only
+		for performance critical channels (storage, network, etc.) that
+		use the monitor page mechanism.
 Users:		Debugging tools and userspace drivers
 
 What:		/sys/bus/vmbus/devices/<UUID>/channels/<N>/ring
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index f174b38f390f..3fc0b247a807 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -347,6 +347,7 @@ static struct vmbus_channel *alloc_channel(void)
 static void free_channel(struct vmbus_channel *channel)
 {
 	tasklet_kill(&channel->callback_event);
+	vmbus_remove_channel_attr_group(channel);
 
 	kobject_put(&channel->kobj);
 }
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index 50fabf4dcb75..e5467b821f41 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -322,6 +322,8 @@ void vmbus_device_unregister(struct hv_device *device_obj);
 int vmbus_add_channel_kobj(struct hv_device *device_obj,
 			   struct vmbus_channel *channel);
 
+void vmbus_remove_channel_attr_group(struct vmbus_channel *channel);
+
 struct vmbus_channel *relid2channel(u32 relid);
 
 void vmbus_free_channels(void);
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 41f782af73ea..aa25f3bcbdea 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -630,7 +630,36 @@ static struct attribute *vmbus_dev_attrs[] = {
 	&dev_attr_driver_override.attr,
 	NULL,
 };
-ATTRIBUTE_GROUPS(vmbus_dev);
+
+/*
+ * Device-level attribute_group callback function. Returns the permission for
+ * each attribute, and returns 0 if an attribute is not visible.
+ */
+static umode_t vmbus_dev_attr_is_visible(struct kobject *kobj,
+					 struct attribute *attr, int idx)
+{
+	struct device *dev = kobj_to_dev(kobj);
+	const struct hv_device *hv_dev = device_to_hv_device(dev);
+
+	/* Hide the monitor attributes if the monitor mechanism is not used. */
+	if (!hv_dev->channel->offermsg.monitor_allocated &&
+	    (attr == &dev_attr_monitor_id.attr ||
+	     attr == &dev_attr_server_monitor_pending.attr ||
+	     attr == &dev_attr_client_monitor_pending.attr ||
+	     attr == &dev_attr_server_monitor_latency.attr ||
+	     attr == &dev_attr_client_monitor_latency.attr ||
+	     attr == &dev_attr_server_monitor_conn_id.attr ||
+	     attr == &dev_attr_client_monitor_conn_id.attr))
+		return 0;
+
+	return attr->mode;
+}
+
+static const struct attribute_group vmbus_dev_group = {
+	.attrs = vmbus_dev_attrs,
+	.is_visible = vmbus_dev_attr_is_visible
+};
+__ATTRIBUTE_GROUPS(vmbus_dev);
 
 /*
  * vmbus_uevent - add uevent for our device
@@ -1583,10 +1612,34 @@ static struct attribute *vmbus_chan_attrs[] = {
 	NULL
 };
 
+/*
+ * Channel-level attribute_group callback function. Returns the permission for
+ * each attribute, and returns 0 if an attribute is not visible.
+ */
+static umode_t vmbus_chan_attr_is_visible(struct kobject *kobj,
+					  struct attribute *attr, int idx)
+{
+	const struct vmbus_channel *channel =
+		container_of(kobj, struct vmbus_channel, kobj);
+
+	/* Hide the monitor attributes if the monitor mechanism is not used. */
+	if (!channel->offermsg.monitor_allocated &&
+	    (attr == &chan_attr_pending.attr ||
+	     attr == &chan_attr_latency.attr ||
+	     attr == &chan_attr_monitor_id.attr))
+		return 0;
+
+	return attr->mode;
+}
+
+static struct attribute_group vmbus_chan_group = {
+	.attrs = vmbus_chan_attrs,
+	.is_visible = vmbus_chan_attr_is_visible
+};
+
 static struct kobj_type vmbus_chan_ktype = {
 	.sysfs_ops = &vmbus_chan_sysfs_ops,
 	.release = vmbus_chan_release,
-	.default_attrs = vmbus_chan_attrs,
 };
 
 /*
@@ -1594,6 +1647,7 @@ static struct kobj_type vmbus_chan_ktype = {
  */
 int vmbus_add_channel_kobj(struct hv_device *dev, struct vmbus_channel *channel)
 {
+	const struct device *device = &dev->device;
 	struct kobject *kobj = &channel->kobj;
 	u32 relid = channel->offermsg.child_relid;
 	int ret;
@@ -1604,11 +1658,30 @@ int vmbus_add_channel_kobj(struct hv_device *dev, struct vmbus_channel *channel)
 	if (ret)
 		return ret;
 
+	ret = sysfs_create_group(kobj, &vmbus_chan_group);
+
+	if (ret) {
+		/*
+		 * The calling functions' error handling paths will cleanup the
+		 * empty channel directory.
+		 */
+		dev_err(device, "Unable to set up channel sysfs files\n");
+		return ret;
+	}
+
 	kobject_uevent(kobj, KOBJ_ADD);
 
 	return 0;
 }
 
+/*
+ * vmbus_remove_channel_attr_group - remove the channel's attribute group
+ */
+void vmbus_remove_channel_attr_group(struct vmbus_channel *channel)
+{
+	sysfs_remove_group(&channel->kobj, &vmbus_chan_group);
+}
+
 /*
  * vmbus_device_create - Creates and registers a new child device
  * on the vmbus.
-- 
2.17.1

Re: [PATCH v6] Drivers: hv: vmbus: Expose monitor data only when monitor pages are used

[Greg KH]

On Tue, Mar 19, 2019 at 12:04:01AM -0400, Kimberly Brown wrote:
> There are two methods for signaling the host: the monitor page mechanism
> and hypercalls. The monitor page mechanism is used by performance
> critical channels (storage, networking, etc.) because it provides
> improved throughput. However, latency is increased. Monitor pages are
> allocated to these channels.
> 
> Monitor pages are not allocated to channels that do not use the monitor
> page mechanism. Therefore, these channels do not have a valid monitor id
> or valid monitor page data. In these cases, some of the "_show"
> functions return incorrect data. They return an invalid monitor id and
> data that is beyond the bounds of the hv_monitor_page array fields.
> 
> The "channel->offermsg.monitor_allocated" value can be used to determine
> whether monitor pages have been allocated to a channel.
> 
> Add "is_visible()" callback functions for the device-level and
> channel-level attribute groups. These functions will hide the monitor
> sysfs files when the monitor mechanism is not used.
> 
> Remove ".default_attributes" from "vmbus_chan_attrs" and create a
> channel-level attribute group. These changes allow the new
> "is_visible()" callback function to be applied to the channel-level
> attributes.
> 
> Call "sysfs_create_group()" in "vmbus_add_channel_kobj()" to create the
> channel's sysfs files. Add a new function,
> “vmbus_remove_channel_attr_group()”, and call it in "free_channel()" to
> remove the channel's sysfs files when the channel is closed.
> 
> Signed-off-by: Kimberly Brown <kimbrownkd@gmail.com>

Nice work, thanks for all of the changes you have made to this over
time.

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

RE: [PATCH v6] Drivers: hv: vmbus: Expose monitor data only when monitor pages are used

[Michael Kelley]

From: Kimberly Brown <kimbrownkd@gmail.com> Sent: Monday, March 18, 2019 9:04 PM
> 
> There are two methods for signaling the host: the monitor page mechanism
> and hypercalls. The monitor page mechanism is used by performance
> critical channels (storage, networking, etc.) because it provides
> improved throughput. However, latency is increased. Monitor pages are
> allocated to these channels.
> 
> Monitor pages are not allocated to channels that do not use the monitor
> page mechanism. Therefore, these channels do not have a valid monitor id
> or valid monitor page data. In these cases, some of the "_show"
> functions return incorrect data. They return an invalid monitor id and
> data that is beyond the bounds of the hv_monitor_page array fields.
> 
> The "channel->offermsg.monitor_allocated" value can be used to determine
> whether monitor pages have been allocated to a channel.
> 
> Add "is_visible()" callback functions for the device-level and
> channel-level attribute groups. These functions will hide the monitor
> sysfs files when the monitor mechanism is not used.
> 
> Remove ".default_attributes" from "vmbus_chan_attrs" and create a
> channel-level attribute group. These changes allow the new
> "is_visible()" callback function to be applied to the channel-level
> attributes.
> 
> Call "sysfs_create_group()" in "vmbus_add_channel_kobj()" to create the
> channel's sysfs files. Add a new function,
> “vmbus_remove_channel_attr_group()”, and call it in "free_channel()" to
> remove the channel's sysfs files when the channel is closed.
> 
> Signed-off-by: Kimberly Brown <kimbrownkd@gmail.com>

Reviewed-by: Michael Kelley <mikelley@microsoft.com>

Re: [PATCH v6] Drivers: hv: vmbus: Expose monitor data only when monitor pages are used

[Sasha Levin]

On Tue, Mar 19, 2019 at 12:04:01AM -0400, Kimberly Brown wrote:
>There are two methods for signaling the host: the monitor page mechanism
>and hypercalls. The monitor page mechanism is used by performance
>critical channels (storage, networking, etc.) because it provides
>improved throughput. However, latency is increased. Monitor pages are
>allocated to these channels.
>
>Monitor pages are not allocated to channels that do not use the monitor
>page mechanism. Therefore, these channels do not have a valid monitor id
>or valid monitor page data. In these cases, some of the "_show"
>functions return incorrect data. They return an invalid monitor id and
>data that is beyond the bounds of the hv_monitor_page array fields.
>
>The "channel->offermsg.monitor_allocated" value can be used to determine
>whether monitor pages have been allocated to a channel.
>
>Add "is_visible()" callback functions for the device-level and
>channel-level attribute groups. These functions will hide the monitor
>sysfs files when the monitor mechanism is not used.
>
>Remove ".default_attributes" from "vmbus_chan_attrs" and create a
>channel-level attribute group. These changes allow the new
>"is_visible()" callback function to be applied to the channel-level
>attributes.
>
>Call "sysfs_create_group()" in "vmbus_add_channel_kobj()" to create the
>channel's sysfs files. Add a new function,
>“vmbus_remove_channel_attr_group()”, and call it in "free_channel()" to
>remove the channel's sysfs files when the channel is closed.
>
>Signed-off-by: Kimberly Brown <kimbrownkd@gmail.com>

I've queued it up for hyperv-fixes, thanks Kimberly.

Should this go in stable as well?

--
Thanks,
Sasha

RE: [PATCH v6] Drivers: hv: vmbus: Expose monitor data only when monitor pages are used

[Michael Kelley]

From: Sasha Levin <sashal@kernel.org> Sent: Wednesday, March 20, 2019 8:57 PM
> 
> I've queued it up for hyperv-fixes, thanks Kimberly.
> 
> Should this go in stable as well?
> 

My take:  these changes lean more toward being a "clean up" rather
than fixing a problem that has significant impact.  The data in the sysfs
entries is bogus, but isn't actually hurting anything. The current code does
index off the end of an array in some cases, but the accesses are reads
and there's plenty of padding so that actually making an invalid memory
reference won't happen.   In the interest of reducing churn in the
stable branches, I would lean toward *not* backporting this to stable.

Michael