* [PATCH v3] Drivers: hv: vmbus: Expose monitor data when channel uses monitor pages [not found] <7481d15f52427917a5f620e29308c1aa5c63f3eb.1550554279.git.kimbrownkd@gmail.com> @ 2019-02-26 5:35 ` Kimberly Brown 2019-02-26 8:18 ` Greg KH 2019-03-01 19:18 ` [PATCH v4] Drivers: hv: vmbus: Expose monitor data only when monitor pages are used Kimberly Brown 0 siblings, 2 replies; 16+ messages in thread From: Kimberly Brown @ 2019-02-26 5:35 UTC (permalink / raw) To: Michael Kelley, Long Li, Sasha Levin, Stephen Hemminger, Dexuan Cui, gregkh Cc: K. Y. Srinivasan, Haiyang Zhang, linux-hyperv, linux-kernel 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v3] Drivers: hv: vmbus: Expose monitor data when channel uses monitor pages 2019-02-26 5:35 ` [PATCH v3] Drivers: hv: vmbus: Expose monitor data when channel uses monitor pages Kimberly Brown @ 2019-02-26 8:18 ` Greg KH 2019-03-01 19:18 ` [PATCH v4] Drivers: hv: vmbus: Expose monitor data only when monitor pages are used Kimberly Brown 1 sibling, 0 replies; 16+ messages in thread From: Greg KH @ 2019-02-26 8:18 UTC (permalink / raw) To: Kimberly Brown Cc: Michael Kelley, Long Li, Sasha Levin, Stephen Hemminger, Dexuan Cui, K. Y. Srinivasan, Haiyang Zhang, linux-hyperv, linux-kernel 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4] Drivers: hv: vmbus: Expose monitor data only when monitor pages are used 2019-02-26 5:35 ` [PATCH v3] Drivers: hv: vmbus: Expose monitor data when channel uses monitor pages Kimberly Brown 2019-02-26 8:18 ` Greg KH @ 2019-03-01 19:18 ` Kimberly Brown 2019-03-02 18:39 ` Michael Kelley ` (2 more replies) 1 sibling, 3 replies; 16+ messages in thread From: Kimberly Brown @ 2019-03-01 19:18 UTC (permalink / raw) To: Michael Kelley, Long Li, Sasha Levin, Stephen Hemminger, Dexuan Cui, gregkh Cc: K. Y. Srinivasan, Haiyang Zhang, linux-hyperv, linux-kernel 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* RE: [PATCH v4] Drivers: hv: vmbus: Expose monitor data only when monitor pages are used 2019-03-01 19:18 ` [PATCH v4] Drivers: hv: vmbus: Expose monitor data only when monitor pages are used Kimberly Brown @ 2019-03-02 18:39 ` Michael Kelley 2019-03-03 21:40 ` Kimberly Brown 2019-03-03 8:05 ` Greg KH 2019-03-08 22:46 ` [PATCH v5] " Kimberly Brown 2 siblings, 1 reply; 16+ messages in thread From: Michael Kelley @ 2019-03-02 18:39 UTC (permalink / raw) To: kimbrownkd, Long Li, Sasha Levin, Stephen Hemminger, Dexuan Cui, gregkh@linuxfoundation.org Cc: KY Srinivasan, Haiyang Zhang, linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4] Drivers: hv: vmbus: Expose monitor data only when monitor pages are used 2019-03-02 18:39 ` Michael Kelley @ 2019-03-03 21:40 ` Kimberly Brown 0 siblings, 0 replies; 16+ messages in thread From: Kimberly Brown @ 2019-03-03 21:40 UTC (permalink / raw) To: Michael Kelley Cc: Long Li, Sasha Levin, Stephen Hemminger, Dexuan Cui, gregkh@linuxfoundation.org, KY Srinivasan, Haiyang Zhang, linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4] Drivers: hv: vmbus: Expose monitor data only when monitor pages are used 2019-03-01 19:18 ` [PATCH v4] Drivers: hv: vmbus: Expose monitor data only when monitor pages are used Kimberly Brown 2019-03-02 18:39 ` Michael Kelley @ 2019-03-03 8:05 ` Greg KH 2019-03-03 21:11 ` Kimberly Brown 2019-03-08 22:46 ` [PATCH v5] " Kimberly Brown 2 siblings, 1 reply; 16+ messages in thread From: Greg KH @ 2019-03-03 8:05 UTC (permalink / raw) To: Kimberly Brown Cc: Michael Kelley, Long Li, Sasha Levin, Stephen Hemminger, Dexuan Cui, K. Y. Srinivasan, Haiyang Zhang, linux-hyperv, linux-kernel 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4] Drivers: hv: vmbus: Expose monitor data only when monitor pages are used 2019-03-03 8:05 ` Greg KH @ 2019-03-03 21:11 ` Kimberly Brown 2019-03-04 7:38 ` Greg KH 0 siblings, 1 reply; 16+ messages in thread From: Kimberly Brown @ 2019-03-03 21:11 UTC (permalink / raw) To: Greg KH Cc: Michael Kelley, Long Li, Sasha Levin, Stephen Hemminger, Dexuan Cui, K. Y. Srinivasan, Haiyang Zhang, linux-hyperv, linux-kernel 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4] Drivers: hv: vmbus: Expose monitor data only when monitor pages are used 2019-03-03 21:11 ` Kimberly Brown @ 2019-03-04 7:38 ` Greg KH 0 siblings, 0 replies; 16+ messages in thread From: Greg KH @ 2019-03-04 7:38 UTC (permalink / raw) To: Kimberly Brown Cc: Michael Kelley, Long Li, Sasha Levin, Stephen Hemminger, Dexuan Cui, K. Y. Srinivasan, Haiyang Zhang, linux-hyperv, linux-kernel 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v5] Drivers: hv: vmbus: Expose monitor data only when monitor pages are used 2019-03-01 19:18 ` [PATCH v4] Drivers: hv: vmbus: Expose monitor data only when monitor pages are used Kimberly Brown 2019-03-02 18:39 ` Michael Kelley 2019-03-03 8:05 ` Greg KH @ 2019-03-08 22:46 ` Kimberly Brown 2019-03-09 7:21 ` Greg KH 2019-03-19 4:04 ` [PATCH v6] " Kimberly Brown 2 siblings, 2 replies; 16+ messages in thread From: Kimberly Brown @ 2019-03-08 22:46 UTC (permalink / raw) To: Michael Kelley, Long Li, Sasha Levin, Stephen Hemminger, Dexuan Cui, Greg KH Cc: K. Y. Srinivasan, Haiyang Zhang, linux-hyperv, linux-kernel 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v5] Drivers: hv: vmbus: Expose monitor data only when monitor pages are used 2019-03-08 22:46 ` [PATCH v5] " Kimberly Brown @ 2019-03-09 7:21 ` Greg KH 2019-03-12 0:04 ` Kimberly Brown 2019-03-19 4:04 ` [PATCH v6] " Kimberly Brown 1 sibling, 1 reply; 16+ messages in thread From: Greg KH @ 2019-03-09 7:21 UTC (permalink / raw) To: Kimberly Brown Cc: Michael Kelley, Long Li, Sasha Levin, Stephen Hemminger, Dexuan Cui, K. Y. Srinivasan, Haiyang Zhang, linux-hyperv, linux-kernel 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v5] Drivers: hv: vmbus: Expose monitor data only when monitor pages are used 2019-03-09 7:21 ` Greg KH @ 2019-03-12 0:04 ` Kimberly Brown 0 siblings, 0 replies; 16+ messages in thread From: Kimberly Brown @ 2019-03-12 0:04 UTC (permalink / raw) To: Greg KH Cc: Michael Kelley, Long Li, Sasha Levin, Stephen Hemminger, Dexuan Cui, K. Y. Srinivasan, Haiyang Zhang, linux-hyperv, linux-kernel 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v6] Drivers: hv: vmbus: Expose monitor data only when monitor pages are used 2019-03-08 22:46 ` [PATCH v5] " Kimberly Brown 2019-03-09 7:21 ` Greg KH @ 2019-03-19 4:04 ` Kimberly Brown 2019-03-19 9:26 ` Greg KH ` (2 more replies) 1 sibling, 3 replies; 16+ messages in thread From: Kimberly Brown @ 2019-03-19 4:04 UTC (permalink / raw) To: Michael Kelley, Long Li, Sasha Levin, Stephen Hemminger, Dexuan Cui, Greg KH Cc: K. Y. Srinivasan, Haiyang Zhang, linux-hyperv, linux-kernel 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v6] Drivers: hv: vmbus: Expose monitor data only when monitor pages are used 2019-03-19 4:04 ` [PATCH v6] " Kimberly Brown @ 2019-03-19 9:26 ` Greg KH 2019-03-20 20:18 ` Michael Kelley 2019-03-21 3:57 ` Sasha Levin 2 siblings, 0 replies; 16+ messages in thread From: Greg KH @ 2019-03-19 9:26 UTC (permalink / raw) To: Kimberly Brown Cc: Michael Kelley, Long Li, Sasha Levin, Stephen Hemminger, Dexuan Cui, K. Y. Srinivasan, Haiyang Zhang, linux-hyperv, linux-kernel 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> ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH v6] Drivers: hv: vmbus: Expose monitor data only when monitor pages are used 2019-03-19 4:04 ` [PATCH v6] " Kimberly Brown 2019-03-19 9:26 ` Greg KH @ 2019-03-20 20:18 ` Michael Kelley 2019-03-21 3:57 ` Sasha Levin 2 siblings, 0 replies; 16+ messages in thread From: Michael Kelley @ 2019-03-20 20:18 UTC (permalink / raw) To: kimbrownkd, Long Li, Sasha Levin, Stephen Hemminger, Dexuan Cui, Greg KH Cc: KY Srinivasan, Haiyang Zhang, linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org 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> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v6] Drivers: hv: vmbus: Expose monitor data only when monitor pages are used 2019-03-19 4:04 ` [PATCH v6] " Kimberly Brown 2019-03-19 9:26 ` Greg KH 2019-03-20 20:18 ` Michael Kelley @ 2019-03-21 3:57 ` Sasha Levin 2019-03-21 15:55 ` Michael Kelley 2 siblings, 1 reply; 16+ messages in thread From: Sasha Levin @ 2019-03-21 3:57 UTC (permalink / raw) To: Kimberly Brown Cc: Michael Kelley, Long Li, Sasha Levin, Stephen Hemminger, Dexuan Cui, Greg KH, K. Y. Srinivasan, Haiyang Zhang, linux-hyperv, linux-kernel 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH v6] Drivers: hv: vmbus: Expose monitor data only when monitor pages are used 2019-03-21 3:57 ` Sasha Levin @ 2019-03-21 15:55 ` Michael Kelley 0 siblings, 0 replies; 16+ messages in thread From: Michael Kelley @ 2019-03-21 15:55 UTC (permalink / raw) To: Sasha Levin, kimbrownkd Cc: Long Li, Sasha Levin, Stephen Hemminger, Dexuan Cui, Greg KH, KY Srinivasan, Haiyang Zhang, linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2019-03-21 15:55 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <7481d15f52427917a5f620e29308c1aa5c63f3eb.1550554279.git.kimbrownkd@gmail.com>
2019-02-26 5:35 ` [PATCH v3] Drivers: hv: vmbus: Expose monitor data when channel uses monitor pages Kimberly Brown
2019-02-26 8:18 ` Greg KH
2019-03-01 19:18 ` [PATCH v4] Drivers: hv: vmbus: Expose monitor data only when monitor pages are used Kimberly Brown
2019-03-02 18:39 ` Michael Kelley
2019-03-03 21:40 ` Kimberly Brown
2019-03-03 8:05 ` Greg KH
2019-03-03 21:11 ` Kimberly Brown
2019-03-04 7:38 ` Greg KH
2019-03-08 22:46 ` [PATCH v5] " Kimberly Brown
2019-03-09 7:21 ` Greg KH
2019-03-12 0:04 ` Kimberly Brown
2019-03-19 4:04 ` [PATCH v6] " Kimberly Brown
2019-03-19 9:26 ` Greg KH
2019-03-20 20:18 ` Michael Kelley
2019-03-21 3:57 ` Sasha Levin
2019-03-21 15:55 ` Michael Kelley
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).