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@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxx>
> -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@xxxxxxxxxxxxx>
> -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@xxxxxxxxxxxxx>
> -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