[PATCH v2] Drivers: hv: vmbus: Expose counters for interrupts and full conditions

[PATCH v2] Drivers: hv: vmbus: Expose counters for interrupts and full conditions

[Kimberly Brown]

Counter values for per-channel interrupts and ring buffer full
conditions are useful for investigating performance.

Expose counters in sysfs for 2 types of guest to host interrupts:
1) Interrupts caused by the channel's outbound ring buffer transitioning
from empty to not empty
2) Interrupts caused by the channel's inbound ring buffer transitioning
from full to not full while a packet is waiting for enough buffer space to
become available

Expose 2 counters in sysfs for the number of times that write operations
encountered a full outbound ring buffer:
1) The total number of write operations that encountered a full
condition
2) The number of write operations that were the first to encounter a
full condition

I tested this patch by confirming that the sysfs files were created and
observing the counter values. The values seemed to increase by a
reasonable amount when the Hyper-v related drivers were in use.

Signed-off-by: Kimberly Brown <kimbrownkd@gmail.com>
---
Changes in v2:
  - Added mailing lists to the cc list
  - Removed the host to guest interrupt counters proposed in v1 because
    they were not accurate
  - Added full condition counters for the channel's outbound ring buffer

 Documentation/ABI/stable/sysfs-bus-vmbus | 33 ++++++++++++++++++++++++
 drivers/hv/ring_buffer.c                 | 14 +++++++++-
 drivers/hv/vmbus_drv.c                   | 32 +++++++++++++++++++++++
 include/linux/hyperv.h                   | 32 +++++++++++++++++++++++
 4 files changed, 110 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/stable/sysfs-bus-vmbus b/Documentation/ABI/stable/sysfs-bus-vmbus
index 3fed8fdb873d..31dc89989032 100644
--- a/Documentation/ABI/stable/sysfs-bus-vmbus
+++ b/Documentation/ABI/stable/sysfs-bus-vmbus
@@ -146,3 +146,36 @@ KernelVersion:	4.16
 Contact:	Stephen Hemminger <sthemmin@microsoft.com>
 Description:	Binary file created by uio_hv_generic for ring buffer
 Users:		Userspace drivers
+
+What:           /sys/bus/vmbus/devices/<UUID>/channels/<N>/intr_in_full
+Date:           January 2019
+KernelVersion:  4.21
+Contact:        Michael Kelley <mikelley@microsoft.com>
+Description:    Number of guest to host interrupts caused by the inbound ring
+		buffer transitioning from full to not full while a packet is
+		waiting for buffer space to become available
+Users:          Debugging tools
+
+What:           /sys/bus/vmbus/devices/<UUID>/channels/<N>/intr_out_empty
+Date:           January 2019
+KernelVersion:  4.21
+Contact:        Michael Kelley <mikelley@microsoft.com>
+Description:    Number of guest to host interrupts caused by the outbound ring
+		buffer transitioning from empty to not empty
+Users:          Debugging tools
+
+What:           /sys/bus/vmbus/devices/<UUID>/channels/<N>/out_full_first
+Date:           January 2019
+KernelVersion:  4.21
+Contact:        Michael Kelley <mikelley@microsoft.com>
+Description:    Number of write operations that were the first to encounter an
+		outbound ring buffer full condition
+Users:          Debugging tools
+
+What:           /sys/bus/vmbus/devices/<UUID>/channels/<N>/out_full_total
+Date:           January 2019
+KernelVersion:  4.21
+Contact:        Michael Kelley <mikelley@microsoft.com>
+Description:    Total number of write operations that encountered an outbound
+		ring buffer full condition
+Users:          Debugging tools
diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
index 64d0c85d5161..be2cbd0bea7d 100644
--- a/drivers/hv/ring_buffer.c
+++ b/drivers/hv/ring_buffer.c
@@ -74,8 +74,10 @@ static void hv_signal_on_write(u32 old_write, struct vmbus_channel *channel)
 	 * This is the only case we need to signal when the
 	 * ring transitions from being empty to non-empty.
 	 */
-	if (old_write == READ_ONCE(rbi->ring_buffer->read_index))
+	if (old_write == READ_ONCE(rbi->ring_buffer->read_index)) {
+		++channel->intr_out_empty;
 		vmbus_setevent(channel);
+	}
 }
 
 /* Get the next write location for the specified ring buffer. */
@@ -273,10 +275,19 @@ int hv_ringbuffer_write(struct vmbus_channel *channel,
 	 * is empty since the read index == write index.
 	 */
 	if (bytes_avail_towrite <= totalbytes_towrite) {
+		++channel->out_full_total;
+
+		if (!channel->out_full_flag) {
+			++channel->out_full_first;
+			channel->out_full_flag = true;
+		}
+
 		spin_unlock_irqrestore(&outring_info->ring_lock, flags);
 		return -EAGAIN;
 	}
 
+	channel->out_full_flag = false;
+
 	/* Write to the ring buffer */
 	next_write_location = hv_get_next_write_location(outring_info);
 
@@ -531,6 +542,7 @@ void hv_pkt_iter_close(struct vmbus_channel *channel)
 	if (curr_write_sz <= pending_sz)
 		return;
 
+	++channel->intr_in_full;
 	vmbus_setevent(channel);
 }
 EXPORT_SYMBOL_GPL(hv_pkt_iter_close);
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 283d184280af..460b7f75251c 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1445,6 +1445,34 @@ static ssize_t channel_events_show(const struct vmbus_channel *channel, char *bu
 }
 static VMBUS_CHAN_ATTR(events, S_IRUGO, channel_events_show, NULL);
 
+static ssize_t channel_intr_in_full_show(const struct vmbus_channel *channel,
+					 char *buf)
+{
+	return sprintf(buf, "%llu\n", channel->intr_in_full);
+}
+static VMBUS_CHAN_ATTR(intr_in_full, 0444, channel_intr_in_full_show, NULL);
+
+static ssize_t channel_intr_out_empty_show(const struct vmbus_channel *channel,
+					   char *buf)
+{
+	return sprintf(buf, "%llu\n", channel->intr_out_empty);
+}
+static VMBUS_CHAN_ATTR(intr_out_empty, 0444, channel_intr_out_empty_show, NULL);
+
+static ssize_t channel_out_full_first_show(const struct vmbus_channel *channel,
+					   char *buf)
+{
+	return sprintf(buf, "%llu\n", channel->out_full_first);
+}
+static VMBUS_CHAN_ATTR(out_full_first, 0444, channel_out_full_first_show, NULL);
+
+static ssize_t channel_out_full_total_show(const struct vmbus_channel *channel,
+					   char *buf)
+{
+	return sprintf(buf, "%llu\n", channel->out_full_total);
+}
+static VMBUS_CHAN_ATTR(out_full_total, 0444, channel_out_full_total_show, NULL);
+
 static ssize_t subchannel_monitor_id_show(const struct vmbus_channel *channel,
 					  char *buf)
 {
@@ -1470,6 +1498,10 @@ static struct attribute *vmbus_chan_attrs[] = {
 	&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
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index b3e24368930a..673f58c4bb1d 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -751,6 +751,27 @@ struct vmbus_channel {
 	u64	interrupts;	/* Host to Guest interrupts */
 	u64	sig_events;	/* Guest to Host events */
 
+	/* Interrupt counts for 2 types of Guest to Host interrupts */
+	u64 intr_in_full;	/* in ring buffer, full to not full */
+	u64 intr_out_empty;	/* out ring buffer, empty to not empty */
+
+	/*
+	 * The total number of write operations that encountered a full
+	 * outbound ring buffer.
+	 */
+	u64 out_full_total;
+	/*
+	 * The number of write operations that were the first to encounter a
+	 * full outbound ring buffer.
+	 */
+	u64 out_full_first;
+	/*
+	 * Indicates that a full outbound ring buffer was encountered. The flag
+	 * is set to true when a full outbound ring buffer is encountered and
+	 * set to false when a write to the outbound ring buffer is completed.
+	 */
+	bool out_full_flag;
+
 	/* Channel callback's invoked in softirq context */
 	struct tasklet_struct callback_event;
 	void (*onchannel_callback)(void *context);
@@ -938,6 +959,17 @@ static inline void *get_per_channel_state(struct vmbus_channel *c)
 static inline void set_channel_pending_send_size(struct vmbus_channel *c,
 						 u32 size)
 {
+	if (size) {
+		++c->out_full_total;
+
+		if (!c->out_full_flag) {
+			++c->out_full_first;
+			c->out_full_flag = true;
+		}
+	} else {
+		c->out_full_flag = false;
+	}
+
 	c->outbound.ring_buffer->pending_send_sz = size;
 }
 
-- 
2.17.1

RE: [PATCH v2] Drivers: hv: vmbus: Expose counters for interrupts and full conditions

[Michael Kelley]

From: Kimberly Brown <kimbrownkd@gmail.com>  Sent: Friday, January 4, 2019 8:35 PM

>  static inline void set_channel_pending_send_size(struct vmbus_channel *c,
>  						 u32 size)
>  {
> +	if (size) {
> +		++c->out_full_total;
> +
> +		if (!c->out_full_flag) {
> +			++c->out_full_first;
> +			c->out_full_flag = true;
> +		}
> +	} else {
> +		c->out_full_flag = false;
> +	}
> +
>  	c->outbound.ring_buffer->pending_send_sz = size;
>  }
> 

I think there may be an atomicity problem with the above code.  I looked
in the hv_sock code, and didn't see any locks being held when
set_channel_pending_send_size() is called.   The original code doesn't need
a lock because it is just storing a single value into pending_send_sz.

In the similar code in hv_ringbuffer_write(), the ring buffer spin lock is held
while the counts are incremented and the out_full_flag is maintained, so all is
good there.  But some locking may be needed here.  Dexuan knows the hv_sock
code best and can comment on whether there is any higher level synchronization
that prevents multiple threads from running the above code on the same channel.
Even if there is such higher level synchronization, this code probably shouldn't
depend on it for correctness.

Michael

RE: [PATCH v2] Drivers: hv: vmbus: Expose counters for interrupts and full conditions

[Dexuan Cui]

> From: Kimberly Brown <kimbrownkd@gmail.com>
> Sent: Friday, January 4, 2019 8:35 PM

...
> +What:
> /sys/bus/vmbus/devices/<UUID>/channels/<N>/intr_in_full
> +Date:           January 2019
> +KernelVersion:  4.21

There is no 4.21 version: see https://lkml.org/lkml/2019/1/6/178  :-)

Thanks!
-- Dexuan

RE: [PATCH v2] Drivers: hv: vmbus: Expose counters for interrupts and full conditions

[Dexuan Cui]

> From: Michael Kelley <mikelley@microsoft.com>
> Sent: Saturday, January 5, 2019 1:01 PM
> > From: Kimberly Brown <kimbrownkd@gmail.com>  Sent: Friday, January 4,
>  > 2019 8:35 PM
> >
> >  static inline void set_channel_pending_send_size(struct vmbus_channel *c,
> >  						 u32 size)
> >  {
> > +	if (size) {
> > +		++c->out_full_total;
> > +
> > +		if (!c->out_full_flag) {
> > +			++c->out_full_first;
> > +			c->out_full_flag = true;
> > +		}
> > +	} else {
> > +		c->out_full_flag = false;
> > +	}
> > +
> >  	c->outbound.ring_buffer->pending_send_sz = size;
> >  }
> >
> 
> I think there may be an atomicity problem with the above code.  I looked
> in the hv_sock code, and didn't see any locks being held when
> set_channel_pending_send_size() is called.   The original code doesn't need
> a lock because it is just storing a single value into pending_send_sz.
>
> In the similar code in hv_ringbuffer_write(), the ring buffer spin lock is held
> while the counts are incremented and the out_full_flag is maintained, so all is
> good there.  But some locking may be needed here.  Dexuan knows the
> hv_sock
> code best and can comment on whether there is any higher level
> synchronization
> that prevents multiple threads from running the above code on the same
> channel.
> Even if there is such higher level synchronization, this code probably shouldn't
> depend on it for correctness.
> 
> Michael

Yes, there is indeed a higher level per-"sock" lock, e.g. in the code path
vsock_stream_sendmsg() -> vsock_stream_has_space() -> 
transport->stream_has_space() -> hvs_stream_has_space() -> 
hvs_set_channel_pending_send_size(), we acquire the lock by
lock_sock(sk) at the beginning of vsock_stream_sendmsg(), and call
release_sock(sk) at the end of the function.

So we don't have an atomicity issue here for hv_sock, which is the only user
of set_channel_pending_send_size(), so far.

Thanks,
-- Dexuan

[PATCH v3] Drivers: hv: vmbus: Expose counters for interrupts and full conditions

[Kimberly Brown]

Counter values for per-channel interrupts and ring buffer full
conditions are useful for investigating performance.

Expose counters in sysfs for 2 types of guest to host interrupts:
1) Interrupts caused by the channel's outbound ring buffer transitioning
from empty to not empty
2) Interrupts caused by the channel's inbound ring buffer transitioning
from full to not full while a packet is waiting for enough buffer space to
become available

Expose 2 counters in sysfs for the number of times that write operations
encountered a full outbound ring buffer:
1) The total number of write operations that encountered a full
condition
2) The number of write operations that were the first to encounter a
full condition

I tested this patch by confirming that the sysfs files were created and
observing the counter values. The values seemed to increase by a
reasonable amount when the Hyper-v related drivers were in use.

Signed-off-by: Kimberly Brown <kimbrownkd@gmail.com>
---
Changes in v3:
 - Used the outbound ring buffer spinlock to protect the the full
   condition counters in set_channel_pending_send_size()
 - Corrected the KernelVersion values for the new entries in
   Documentation/ABI/stable/sysfs-bus-vmbus

Changes in v2:
 - Added mailing lists to the cc list
 - Removed the host to guest interrupt counters proposed in v1 because
   they were not accurate
 - Added full condition counters for the channel's outbound ring buffer

 Documentation/ABI/stable/sysfs-bus-vmbus | 33 ++++++++++++++++++++
 drivers/hv/ring_buffer.c                 | 14 ++++++++-
 drivers/hv/vmbus_drv.c                   | 32 ++++++++++++++++++++
 include/linux/hyperv.h                   | 38 ++++++++++++++++++++++++
 4 files changed, 116 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/stable/sysfs-bus-vmbus b/Documentation/ABI/stable/sysfs-bus-vmbus
index 3fed8fdb873d..a0304c563467 100644
--- a/Documentation/ABI/stable/sysfs-bus-vmbus
+++ b/Documentation/ABI/stable/sysfs-bus-vmbus
@@ -146,3 +146,36 @@ KernelVersion:	4.16
 Contact:	Stephen Hemminger <sthemmin@microsoft.com>
 Description:	Binary file created by uio_hv_generic for ring buffer
 Users:		Userspace drivers
+
+What:           /sys/bus/vmbus/devices/<UUID>/channels/<N>/intr_in_full
+Date:           January 2019
+KernelVersion:  5.0
+Contact:        Michael Kelley <mikelley@microsoft.com>
+Description:    Number of guest to host interrupts caused by the inbound ring
+		buffer transitioning from full to not full while a packet is
+		waiting for buffer space to become available
+Users:          Debugging tools
+
+What:           /sys/bus/vmbus/devices/<UUID>/channels/<N>/intr_out_empty
+Date:           January 2019
+KernelVersion:  5.0
+Contact:        Michael Kelley <mikelley@microsoft.com>
+Description:    Number of guest to host interrupts caused by the outbound ring
+		buffer transitioning from empty to not empty
+Users:          Debugging tools
+
+What:           /sys/bus/vmbus/devices/<UUID>/channels/<N>/out_full_first
+Date:           January 2019
+KernelVersion:  5.0
+Contact:        Michael Kelley <mikelley@microsoft.com>
+Description:    Number of write operations that were the first to encounter an
+		outbound ring buffer full condition
+Users:          Debugging tools
+
+What:           /sys/bus/vmbus/devices/<UUID>/channels/<N>/out_full_total
+Date:           January 2019
+KernelVersion:  5.0
+Contact:        Michael Kelley <mikelley@microsoft.com>
+Description:    Total number of write operations that encountered an outbound
+		ring buffer full condition
+Users:          Debugging tools
diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
index 1f1a55e07733..9e8b31ccc142 100644
--- a/drivers/hv/ring_buffer.c
+++ b/drivers/hv/ring_buffer.c
@@ -74,8 +74,10 @@ static void hv_signal_on_write(u32 old_write, struct vmbus_channel *channel)
 	 * This is the only case we need to signal when the
 	 * ring transitions from being empty to non-empty.
 	 */
-	if (old_write == READ_ONCE(rbi->ring_buffer->read_index))
+	if (old_write == READ_ONCE(rbi->ring_buffer->read_index)) {
+		++channel->intr_out_empty;
 		vmbus_setevent(channel);
+	}
 }
 
 /* Get the next write location for the specified ring buffer. */
@@ -272,10 +274,19 @@ int hv_ringbuffer_write(struct vmbus_channel *channel,
 	 * is empty since the read index == write index.
 	 */
 	if (bytes_avail_towrite <= totalbytes_towrite) {
+		++channel->out_full_total;
+
+		if (!channel->out_full_flag) {
+			++channel->out_full_first;
+			channel->out_full_flag = true;
+		}
+
 		spin_unlock_irqrestore(&outring_info->ring_lock, flags);
 		return -EAGAIN;
 	}
 
+	channel->out_full_flag = false;
+
 	/* Write to the ring buffer */
 	next_write_location = hv_get_next_write_location(outring_info);
 
@@ -530,6 +541,7 @@ void hv_pkt_iter_close(struct vmbus_channel *channel)
 	if (curr_write_sz <= pending_sz)
 		return;
 
+	++channel->intr_in_full;
 	vmbus_setevent(channel);
 }
 EXPORT_SYMBOL_GPL(hv_pkt_iter_close);
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 403fee01572c..e291a7d3180c 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1496,6 +1496,34 @@ static ssize_t channel_events_show(const struct vmbus_channel *channel, char *bu
 }
 static VMBUS_CHAN_ATTR(events, S_IRUGO, channel_events_show, NULL);
 
+static ssize_t channel_intr_in_full_show(const struct vmbus_channel *channel,
+					 char *buf)
+{
+	return sprintf(buf, "%llu\n", channel->intr_in_full);
+}
+static VMBUS_CHAN_ATTR(intr_in_full, 0444, channel_intr_in_full_show, NULL);
+
+static ssize_t channel_intr_out_empty_show(const struct vmbus_channel *channel,
+					   char *buf)
+{
+	return sprintf(buf, "%llu\n", channel->intr_out_empty);
+}
+static VMBUS_CHAN_ATTR(intr_out_empty, 0444, channel_intr_out_empty_show, NULL);
+
+static ssize_t channel_out_full_first_show(const struct vmbus_channel *channel,
+					   char *buf)
+{
+	return sprintf(buf, "%llu\n", channel->out_full_first);
+}
+static VMBUS_CHAN_ATTR(out_full_first, 0444, channel_out_full_first_show, NULL);
+
+static ssize_t channel_out_full_total_show(const struct vmbus_channel *channel,
+					   char *buf)
+{
+	return sprintf(buf, "%llu\n", channel->out_full_total);
+}
+static VMBUS_CHAN_ATTR(out_full_total, 0444, channel_out_full_total_show, NULL);
+
 static ssize_t subchannel_monitor_id_show(const struct vmbus_channel *channel,
 					  char *buf)
 {
@@ -1521,6 +1549,10 @@ static struct attribute *vmbus_chan_attrs[] = {
 	&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
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index dcb6977afce9..7e5239123276 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -751,6 +751,27 @@ struct vmbus_channel {
 	u64	interrupts;	/* Host to Guest interrupts */
 	u64	sig_events;	/* Guest to Host events */
 
+	/* Interrupt counts for 2 types of Guest to Host interrupts */
+	u64 intr_in_full;	/* in ring buffer, full to not full */
+	u64 intr_out_empty;	/* out ring buffer, empty to not empty */
+
+	/*
+	 * The total number of write operations that encountered a full
+	 * outbound ring buffer.
+	 */
+	u64 out_full_total;
+	/*
+	 * The number of write operations that were the first to encounter a
+	 * full outbound ring buffer.
+	 */
+	u64 out_full_first;
+	/*
+	 * Indicates that a full outbound ring buffer was encountered. The flag
+	 * is set to true when a full outbound ring buffer is encountered and
+	 * set to false when a write to the outbound ring buffer is completed.
+	 */
+	bool out_full_flag;
+
 	/* Channel callback's invoked in softirq context */
 	struct tasklet_struct callback_event;
 	void (*onchannel_callback)(void *context);
@@ -936,6 +957,23 @@ static inline void *get_per_channel_state(struct vmbus_channel *c)
 static inline void set_channel_pending_send_size(struct vmbus_channel *c,
 						 u32 size)
 {
+	unsigned long flags;
+
+	spin_lock_irqsave(&c->outbound.ring_lock, flags);
+
+	if (size) {
+		++c->out_full_total;
+
+		if (!c->out_full_flag) {
+			++c->out_full_first;
+			c->out_full_flag = true;
+		}
+	} else {
+		c->out_full_flag = false;
+	}
+
+	spin_unlock_irqrestore(&c->outbound.ring_lock, flags);
+
 	c->outbound.ring_buffer->pending_send_sz = size;
 }
 
-- 
2.17.1

RE: [PATCH v3] Drivers: hv: vmbus: Expose counters for interrupts and full conditions

[Dexuan Cui]

> From: Kimberly Brown <kimbrownkd@gmail.com>
> Sent: Wednesday, January 16, 2019 8:38 PM
> To: Michael Kelley <mikelley@microsoft.com>; Long Li
> <longli@microsoft.com>; Sasha Levin <Alexander.Levin@microsoft.com>;
> Dexuan Cui <decui@microsoft.com>
> Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; Stephen Hemminger
> <sthemmin@microsoft.com>; devel@linuxdriverproject.org;
> linux-kernel@vger.kernel.org
> Subject: [PATCH v3] Drivers: hv: vmbus: Expose counters for interrupts and full
> conditions
> 
> Counter values for per-channel interrupts and ring buffer full
> conditions are useful for investigating performance.
> 
> Expose counters in sysfs for 2 types of guest to host interrupts:
> 1) Interrupts caused by the channel's outbound ring buffer transitioning
> from empty to not empty
> 2) Interrupts caused by the channel's inbound ring buffer transitioning
> from full to not full while a packet is waiting for enough buffer space to
> become available
> 
> Expose 2 counters in sysfs for the number of times that write operations
> encountered a full outbound ring buffer:
> 1) The total number of write operations that encountered a full
> condition
> 2) The number of write operations that were the first to encounter a
> full condition
> 
> I tested this patch by confirming that the sysfs files were created and
> observing the counter values. The values seemed to increase by a
> reasonable amount when the Hyper-v related drivers were in use.
> 
> Signed-off-by: Kimberly Brown <kimbrownkd@gmail.com>
> ---
> Changes in v3:
>  - Used the outbound ring buffer spinlock to protect the the full
>    condition counters in set_channel_pending_send_size()
>  - Corrected the KernelVersion values for the new entries in
>    Documentation/ABI/stable/sysfs-bus-vmbus
> 
> Changes in v2:
>  - Added mailing lists to the cc list
>  - Removed the host to guest interrupt counters proposed in v1 because
>    they were not accurate
>  - Added full condition counters for the channel's outbound ring buffer
> 
>  Documentation/ABI/stable/sysfs-bus-vmbus | 33 ++++++++++++++++++++
>  drivers/hv/ring_buffer.c                 | 14 ++++++++-
>  drivers/hv/vmbus_drv.c                   | 32 ++++++++++++++++++++
>  include/linux/hyperv.h                   | 38
> ++++++++++++++++++++++++
>  4 files changed, 116 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/ABI/stable/sysfs-bus-vmbus
> b/Documentation/ABI/stable/sysfs-bus-vmbus
> index 3fed8fdb873d..a0304c563467 100644
> --- a/Documentation/ABI/stable/sysfs-bus-vmbus
> +++ b/Documentation/ABI/stable/sysfs-bus-vmbus
> @@ -146,3 +146,36 @@ KernelVersion:	4.16
>  Contact:	Stephen Hemminger <sthemmin@microsoft.com>
>  Description:	Binary file created by uio_hv_generic for ring buffer
>  Users:		Userspace drivers
> +
> +What:
> /sys/bus/vmbus/devices/<UUID>/channels/<N>/intr_in_full
> +Date:           January 2019
> +KernelVersion:  5.0
> +Contact:        Michael Kelley <mikelley@microsoft.com>
> +Description:    Number of guest to host interrupts caused by the inbound
> ring
> +		buffer transitioning from full to not full while a packet is
> +		waiting for buffer space to become available
> +Users:          Debugging tools
> +
> +What:
> /sys/bus/vmbus/devices/<UUID>/channels/<N>/intr_out_empty
> +Date:           January 2019
> +KernelVersion:  5.0
> +Contact:        Michael Kelley <mikelley@microsoft.com>
> +Description:    Number of guest to host interrupts caused by the outbound
> ring
> +		buffer transitioning from empty to not empty
> +Users:          Debugging tools
> +
> +What:
> /sys/bus/vmbus/devices/<UUID>/channels/<N>/out_full_first
> +Date:           January 2019
> +KernelVersion:  5.0
> +Contact:        Michael Kelley <mikelley@microsoft.com>
> +Description:    Number of write operations that were the first to encounter
> an
> +		outbound ring buffer full condition
> +Users:          Debugging tools
> +
> +What:
> /sys/bus/vmbus/devices/<UUID>/channels/<N>/out_full_total
> +Date:           January 2019
> +KernelVersion:  5.0
> +Contact:        Michael Kelley <mikelley@microsoft.com>
> +Description:    Total number of write operations that encountered an
> outbound
> +		ring buffer full condition
> +Users:          Debugging tools
> diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
> index 1f1a55e07733..9e8b31ccc142 100644
> --- a/drivers/hv/ring_buffer.c
> +++ b/drivers/hv/ring_buffer.c
> @@ -74,8 +74,10 @@ static void hv_signal_on_write(u32 old_write, struct
> vmbus_channel *channel)
>  	 * This is the only case we need to signal when the
>  	 * ring transitions from being empty to non-empty.
>  	 */
> -	if (old_write == READ_ONCE(rbi->ring_buffer->read_index))
> +	if (old_write == READ_ONCE(rbi->ring_buffer->read_index)) {
> +		++channel->intr_out_empty;
>  		vmbus_setevent(channel);
> +	}
>  }
> 
>  /* Get the next write location for the specified ring buffer. */
> @@ -272,10 +274,19 @@ int hv_ringbuffer_write(struct vmbus_channel
> *channel,
>  	 * is empty since the read index == write index.
>  	 */
>  	if (bytes_avail_towrite <= totalbytes_towrite) {
> +		++channel->out_full_total;
> +
> +		if (!channel->out_full_flag) {
> +			++channel->out_full_first;
> +			channel->out_full_flag = true;
> +		}
> +
>  		spin_unlock_irqrestore(&outring_info->ring_lock, flags);
>  		return -EAGAIN;
>  	}
> 
> +	channel->out_full_flag = false;
> +
>  	/* Write to the ring buffer */
>  	next_write_location = hv_get_next_write_location(outring_info);
> 
> @@ -530,6 +541,7 @@ void hv_pkt_iter_close(struct vmbus_channel
> *channel)
>  	if (curr_write_sz <= pending_sz)
>  		return;
> 
> +	++channel->intr_in_full;
>  	vmbus_setevent(channel);
>  }
>  EXPORT_SYMBOL_GPL(hv_pkt_iter_close);
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 403fee01572c..e291a7d3180c 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -1496,6 +1496,34 @@ static ssize_t channel_events_show(const struct
> vmbus_channel *channel, char *bu
>  }
>  static VMBUS_CHAN_ATTR(events, S_IRUGO, channel_events_show, NULL);
> 
> +static ssize_t channel_intr_in_full_show(const struct vmbus_channel
> *channel,
> +					 char *buf)
> +{
> +	return sprintf(buf, "%llu\n", channel->intr_in_full);
> +}
> +static VMBUS_CHAN_ATTR(intr_in_full, 0444, channel_intr_in_full_show,
> NULL);
> +
> +static ssize_t channel_intr_out_empty_show(const struct vmbus_channel
> *channel,
> +					   char *buf)
> +{
> +	return sprintf(buf, "%llu\n", channel->intr_out_empty);
> +}
> +static VMBUS_CHAN_ATTR(intr_out_empty, 0444,
> channel_intr_out_empty_show, NULL);
> +
> +static ssize_t channel_out_full_first_show(const struct vmbus_channel
> *channel,
> +					   char *buf)
> +{
> +	return sprintf(buf, "%llu\n", channel->out_full_first);
> +}
> +static VMBUS_CHAN_ATTR(out_full_first, 0444, channel_out_full_first_show,
> NULL);
> +
> +static ssize_t channel_out_full_total_show(const struct vmbus_channel
> *channel,
> +					   char *buf)
> +{
> +	return sprintf(buf, "%llu\n", channel->out_full_total);
> +}
> +static VMBUS_CHAN_ATTR(out_full_total, 0444, channel_out_full_total_show,
> NULL);
> +
>  static ssize_t subchannel_monitor_id_show(const struct vmbus_channel
> *channel,
>  					  char *buf)
>  {
> @@ -1521,6 +1549,10 @@ static struct attribute *vmbus_chan_attrs[] = {
>  	&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
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index dcb6977afce9..7e5239123276 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -751,6 +751,27 @@ struct vmbus_channel {
>  	u64	interrupts;	/* Host to Guest interrupts */
>  	u64	sig_events;	/* Guest to Host events */
> 
> +	/* Interrupt counts for 2 types of Guest to Host interrupts */
> +	u64 intr_in_full;	/* in ring buffer, full to not full */
> +	u64 intr_out_empty;	/* out ring buffer, empty to not empty */
> +
> +	/*
> +	 * The total number of write operations that encountered a full
> +	 * outbound ring buffer.
> +	 */
> +	u64 out_full_total;
> +	/*
> +	 * The number of write operations that were the first to encounter a
> +	 * full outbound ring buffer.
> +	 */
> +	u64 out_full_first;
> +	/*
> +	 * Indicates that a full outbound ring buffer was encountered. The flag
> +	 * is set to true when a full outbound ring buffer is encountered and
> +	 * set to false when a write to the outbound ring buffer is completed.
> +	 */
> +	bool out_full_flag;
> +
>  	/* Channel callback's invoked in softirq context */
>  	struct tasklet_struct callback_event;
>  	void (*onchannel_callback)(void *context);
> @@ -936,6 +957,23 @@ static inline void *get_per_channel_state(struct
> vmbus_channel *c)
>  static inline void set_channel_pending_send_size(struct vmbus_channel *c,
>  						 u32 size)
>  {
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&c->outbound.ring_lock, flags);
> +
> +	if (size) {
> +		++c->out_full_total;
> +
> +		if (!c->out_full_flag) {
> +			++c->out_full_first;
> +			c->out_full_flag = true;
> +		}
> +	} else {
> +		c->out_full_flag = false;
> +	}
> +
> +	spin_unlock_irqrestore(&c->outbound.ring_lock, flags);
> +
>  	c->outbound.ring_buffer->pending_send_sz = size;
>  }
> 
> --
> 2.17.1

Looks good to me. 

Reviewed-by: Dexuan Cui <decui@microsoft.com>

-- Dexuan

RE: [PATCH v3] Drivers: hv: vmbus: Expose counters for interrupts and full conditions

[Michael Kelley]

From: Kimberly Brown <kimbrownkd@gmail.com> Sent: Wednesday, January 16, 2019 8:38 PM
> 
> Counter values for per-channel interrupts and ring buffer full
> conditions are useful for investigating performance.
> 
> Expose counters in sysfs for 2 types of guest to host interrupts:
> 1) Interrupts caused by the channel's outbound ring buffer transitioning
> from empty to not empty
> 2) Interrupts caused by the channel's inbound ring buffer transitioning
> from full to not full while a packet is waiting for enough buffer space to
> become available
> 
> Expose 2 counters in sysfs for the number of times that write operations
> encountered a full outbound ring buffer:
> 1) The total number of write operations that encountered a full
> condition
> 2) The number of write operations that were the first to encounter a
> full condition
> 
> I tested this patch by confirming that the sysfs files were created and
> observing the counter values. The values seemed to increase by a
> reasonable amount when the Hyper-v related drivers were in use.
> 
> Signed-off-by: Kimberly Brown <kimbrownkd@gmail.com>
> ---
> Changes in v3:
>  - Used the outbound ring buffer spinlock to protect the the full
>    condition counters in set_channel_pending_send_size()
>  - Corrected the KernelVersion values for the new entries in
>    Documentation/ABI/stable/sysfs-bus-vmbus
> 
> Changes in v2:
>  - Added mailing lists to the cc list
>  - Removed the host to guest interrupt counters proposed in v1 because
>    they were not accurate
>  - Added full condition counters for the channel's outbound ring buffer
> 
>  Documentation/ABI/stable/sysfs-bus-vmbus | 33 ++++++++++++++++++++
>  drivers/hv/ring_buffer.c                 | 14 ++++++++-
>  drivers/hv/vmbus_drv.c                   | 32 ++++++++++++++++++++
>  include/linux/hyperv.h                   | 38 ++++++++++++++++++++++++
>  4 files changed, 116 insertions(+), 1 deletion(-)
> 

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

Re: [PATCH v3] Drivers: hv: vmbus: Expose counters for interrupts and full conditions

[Stephen Hemminger]

> +static ssize_t channel_intr_in_full_show(const struct vmbus_channel
> *channel,
> +					 char *buf)
> +{
> +	return sprintf(buf, "%llu\n", channel->intr_in_full);
> +}


intr_in_full is u64, which is not the same as unsigned long long.
to be correct you need a cast here.

> > diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> > index dcb6977afce9..7e5239123276 100644
> > --- a/include/linux/hyperv.h
> > +++ b/include/linux/hyperv.h
> > @@ -751,6 +751,27 @@ struct vmbus_channel {
> >  	u64	interrupts;	/* Host to Guest interrupts */
> >  	u64	sig_events;	/* Guest to Host events */
> > 
> > +	/* Interrupt counts for 2 types of Guest to Host interrupts */
> > +	u64 intr_in_full;	/* in ring buffer, full to not full */
> > +	u64 intr_out_empty;	/* out ring buffer, empty to not empty */
> > +
> > +	/*
> > +	 * The total number of write operations that encountered a full
> > +	 * outbound ring buffer.
> > +	 */
> > +	u64 out_full_total;
> > +	/*
> > +	 * The number of write operations that were the first to encounter a
> > +	 * full outbound ring buffer.
> > +	 */
> > +	u64 out_full_first;

Adding more fields changes cache layout which can cause
additional cache miss in the hot path.  

> > +	/*
> > +	 * Indicates that a full outbound ring buffer was encountered. The flag
> > +	 * is set to true when a full outbound ring buffer is encountered and
> > +	 * set to false when a write to the outbound ring buffer is completed.
> > +	 */
> > +	bool out_full_flag;

Discussion on kernel mailing list. Recommends against putting bool
in structures since that pads to full sizeof(int).  Could this be
part of a bitfield?

> >  	/* Channel callback's invoked in softirq context */
> >  	struct tasklet_struct callback_event;
> >  	void (*onchannel_callback)(void *context);
> > @@ -936,6 +957,23 @@ static inline void *get_per_channel_state(struct
> > vmbus_channel *c)
> >  static inline void set_channel_pending_send_size(struct vmbus_channel *c,
> >  						 u32 size)
> >  {
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&c->outbound.ring_lock, flags);
> > +
> > +	if (size) {
> > +		++c->out_full_total;
> > +
> > +		if (!c->out_full_flag) {
> > +			++c->out_full_first;
> > +			c->out_full_flag = true;
> > +		}
> > +	} else {
> > +		c->out_full_flag = false;
> > +	}
> > +
> > +	spin_unlock_irqrestore(&c->outbound.ring_lock, flags);

If this is called often, the additional locking will impact performance.

> >  	c->outbound.ring_buffer->pending_send_sz = size;
> >  }
> > 

Could I propose another alternative.

It might be more useful to count the guest to host interaction events
rather than the ring buffer.

For example the number of calls to:
	vmbus_set_event which means host exit call
	vmbus_setevent fastpath using sync_set_bit
	calls to rinbuffer_write that returned -EAGAIN

These would require less locking, reuse existing code paths
and not require additional state.

Re: [PATCH v3] Drivers: hv: vmbus: Expose counters for interrupts and full conditions

[Kimberly Brown]

On Thu, Jan 17, 2019 at 09:11:03AM -0800, Stephen Hemminger wrote:
> 
> 
> > +static ssize_t channel_intr_in_full_show(const struct vmbus_channel
> > *channel,
> > +					 char *buf)
> > +{
> > +	return sprintf(buf, "%llu\n", channel->intr_in_full);
> > +}
> 
> 
> intr_in_full is u64, which is not the same as unsigned long long.
> to be correct you need a cast here.
>

Thanks for the feedback. I'll fix this issue in all four of the "_show"
functions that are added in this patch.


> > > diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> > > index dcb6977afce9..7e5239123276 100644
> > > --- a/include/linux/hyperv.h
> > > +++ b/include/linux/hyperv.h
> > > @@ -751,6 +751,27 @@ struct vmbus_channel {
> > >  	u64	interrupts;	/* Host to Guest interrupts */
> > >  	u64	sig_events;	/* Guest to Host events */
> > > 
> > > +	/* Interrupt counts for 2 types of Guest to Host interrupts */
> > > +	u64 intr_in_full;	/* in ring buffer, full to not full */
> > > +	u64 intr_out_empty;	/* out ring buffer, empty to not empty */
> > > +
> > > +	/*
> > > +	 * The total number of write operations that encountered a full
> > > +	 * outbound ring buffer.
> > > +	 */
> > > +	u64 out_full_total;
> > > +	/*
> > > +	 * The number of write operations that were the first to encounter a
> > > +	 * full outbound ring buffer.
> > > +	 */
> > > +	u64 out_full_first;
> 
> Adding more fields changes cache layout which can cause
> additional cache miss in the hot path.  
>

Good point. I think that the "intr_out_empty" field is in a good
location, but the "intr_in_full", "out_full_first", and "out_full_total"
fields could be moved to the end of the struct. These variables are used
only when ring buffer full conditions occur. Ring buffer full conditions
shouldn't be encountered often, and, if they are, they're a signal that
changes should probably be made to prevent them.

If you have any other suggestions for this, please let me know.


> > > +	/*
> > > +	 * Indicates that a full outbound ring buffer was encountered. The flag
> > > +	 * is set to true when a full outbound ring buffer is encountered and
> > > +	 * set to false when a write to the outbound ring buffer is completed.
> > > +	 */
> > > +	bool out_full_flag;
> 
> Discussion on kernel mailing list. Recommends against putting bool
> in structures since that pads to full sizeof(int).  Could this be
> part of a bitfield?
> 

There are currently 4 other bool variables in this struct. Maybe some or
all of the bool variables could be placed adjacent to each other and
changed into bitfields. I'll need to look into this.


> > >  	/* Channel callback's invoked in softirq context */
> > >  	struct tasklet_struct callback_event;
> > >  	void (*onchannel_callback)(void *context);
> > > @@ -936,6 +957,23 @@ static inline void *get_per_channel_state(struct
> > > vmbus_channel *c)
> > >  static inline void set_channel_pending_send_size(struct vmbus_channel *c,
> > >  						 u32 size)
> > >  {
> > > +	unsigned long flags;
> > > +
> > > +	spin_lock_irqsave(&c->outbound.ring_lock, flags);
> > > +
> > > +	if (size) {
> > > +		++c->out_full_total;
> > > +
> > > +		if (!c->out_full_flag) {
> > > +			++c->out_full_first;
> > > +			c->out_full_flag = true;
> > > +		}
> > > +	} else {
> > > +		c->out_full_flag = false;
> > > +	}
> > > +
> > > +	spin_unlock_irqrestore(&c->outbound.ring_lock, flags);
> 
> If this is called often, the additional locking will impact performance.
>

In hv_sock, each call of "hvs_stream_has_space()" results in a call to
"channel_set_pending_send_size()", so this could be a concern. I'll work
on addressing this issue.


> > >  	c->outbound.ring_buffer->pending_send_sz = size;
> > >  }
> > > 
> 
> Could I propose another alternative.
> 
> It might be more useful to count the guest to host interaction events
> rather than the ring buffer.
> 
> For example the number of calls to:
> 	vmbus_set_event which means host exit call
> 	vmbus_setevent fastpath using sync_set_bit
> 	calls to rinbuffer_write that returned -EAGAIN
> 
> These would require less locking, reuse existing code paths
> and not require additional state.
> 

I'm not sure that this approach would provide the data that we're
looking for. For example, we're interested in evaluating how often ring
buffer write operations encounter full conditions. For this, we need to
know how many interaction events were caused by the ring buffer being
full. Counting the number of calls to "vmbus_set_event()" and
"vmbus_setevent()" wouldn't allow us to determine what caused the events.

For counting the full conditions, the number of calls to
"ring_buffer_write()" that returned -EAGAIN isn't sufficient because
hv_sock doesn't use the -EAGAIN path to determine that the ring buffer is
full. Therefore, we need to count the number of full conditions in both
"ring_buffer_write()" and "set_channel_pending_send_size()".

[PATCH v4] Drivers: hv: vmbus: Expose counters for interrupts and full conditions

[Kimberly Brown]

Counter values for per-channel interrupts and ring buffer full
conditions are useful for investigating performance.

Expose counters in sysfs for 2 types of guest to host interrupts:
1) Interrupts caused by the channel's outbound ring buffer transitioning
from empty to not empty
2) Interrupts caused by the channel's inbound ring buffer transitioning
from full to not full while a packet is waiting for enough buffer space to
become available

Expose 2 counters in sysfs for the number of times that write operations
encountered a full outbound ring buffer:
1) The total number of write operations that encountered a full
condition
2) The number of write operations that were the first to encounter a
full condition

Increment the outbound full condition counters in the
hv_ringbuffer_write() function because, for most drivers, a full
outbound ring buffer is detected in that function. Also increment the
outbound full condition counters in the set_channel_pending_send_size()
function. In the hv_sock driver, a full outbound ring buffer is detected
and set_channel_pending_send_size() is called before
hv_ringbuffer_write() is called.

I tested this patch by confirming that the sysfs files were created and
observing the counter values. The values seemed to increase by a
reasonable amount when the Hyper-v related drivers were in use.

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

Changes in v4:
 - In the commit message, added a paragraph describing why the full
   condition counters are incremented in two functions.

 - In the four new "_show" functions, added a cast to "(unsigned long
   long)" in the sprintf() calls. This problem was reported by S.
   Hemminger.

 - In the vmbus_channel struct definition, moved the three new fields
   pertaining to full ring buffers ("intr_in_full", "out_full_total",
   "out_full_first") to the bottom of the struct.  These fields should 
   not be used often because full ring buffers should not be encountered 
   often. This change addresses S. Hemminger's concern about the new 
   fields causing additional cache misses in the hot path.

 - In the set_channel_pending_send_size() function, moved the
   acquire/release of the spinlock to inside the if-statement. The
   spinlock needs to protect only the incrementing variables. Since full
   ring buffers should not be encountered often, the addition of this
   spinlock acquire/release should not affect performance. This change
   addresses S. Hemminger's concern that additional locking will affect
   performance.

 NOTE: S. Hemminger also requested that I consider placing the new bool
       field, "out_full_flag", in a bitfield. I chose not to make this 
       change because setting an individual bit in a bitfield is less 
       efficient than setting the value of a bool.

Changes in v3:
 - Used the outbound ring buffer spinlock to protect the the full
   condition counters in set_channel_pending_send_size()
 - Corrected the KernelVersion values for the new entries in
   Documentation/ABI/stable/sysfs-bus-vmbus

Changes in v2:
 - Added mailing lists to the cc list
 - Removed the host to guest interrupt counters proposed in v1 because
   they were not accurate
 - Added full condition counters for the channel's outbound ring buffer


Documentation/ABI/stable/sysfs-bus-vmbus | 33 +++++++++++++++++
 drivers/hv/ring_buffer.c                 | 14 +++++++-
 drivers/hv/vmbus_drv.c                   | 36 +++++++++++++++++++
 include/linux/hyperv.h                   | 46 ++++++++++++++++++++++++
 4 files changed, 128 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/stable/sysfs-bus-vmbus b/Documentation/ABI/stable/sysfs-bus-vmbus
index 3fed8fdb873d..826689dcc2e6 100644
--- a/Documentation/ABI/stable/sysfs-bus-vmbus
+++ b/Documentation/ABI/stable/sysfs-bus-vmbus
@@ -146,3 +146,36 @@ KernelVersion:	4.16
 Contact:	Stephen Hemminger <sthemmin@microsoft.com>
 Description:	Binary file created by uio_hv_generic for ring buffer
 Users:		Userspace drivers
+
+What:           /sys/bus/vmbus/devices/<UUID>/channels/<N>/intr_in_full
+Date:           February 2019
+KernelVersion:  5.0
+Contact:        Michael Kelley <mikelley@microsoft.com>
+Description:    Number of guest to host interrupts caused by the inbound ring
+		buffer transitioning from full to not full while a packet is
+		waiting for buffer space to become available
+Users:          Debugging tools
+
+What:           /sys/bus/vmbus/devices/<UUID>/channels/<N>/intr_out_empty
+Date:           February 2019
+KernelVersion:  5.0
+Contact:        Michael Kelley <mikelley@microsoft.com>
+Description:    Number of guest to host interrupts caused by the outbound ring
+		buffer transitioning from empty to not empty
+Users:          Debugging tools
+
+What:           /sys/bus/vmbus/devices/<UUID>/channels/<N>/out_full_first
+Date:           February 2019
+KernelVersion:  5.0
+Contact:        Michael Kelley <mikelley@microsoft.com>
+Description:    Number of write operations that were the first to encounter an
+		outbound ring buffer full condition
+Users:          Debugging tools
+
+What:           /sys/bus/vmbus/devices/<UUID>/channels/<N>/out_full_total
+Date:           February 2019
+KernelVersion:  5.0
+Contact:        Michael Kelley <mikelley@microsoft.com>
+Description:    Total number of write operations that encountered an outbound
+		ring buffer full condition
+Users:          Debugging tools
diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
index 1f1a55e07733..9e8b31ccc142 100644
--- a/drivers/hv/ring_buffer.c
+++ b/drivers/hv/ring_buffer.c
@@ -74,8 +74,10 @@ static void hv_signal_on_write(u32 old_write, struct vmbus_channel *channel)
 	 * This is the only case we need to signal when the
 	 * ring transitions from being empty to non-empty.
 	 */
-	if (old_write == READ_ONCE(rbi->ring_buffer->read_index))
+	if (old_write == READ_ONCE(rbi->ring_buffer->read_index)) {
+		++channel->intr_out_empty;
 		vmbus_setevent(channel);
+	}
 }
 
 /* Get the next write location for the specified ring buffer. */
@@ -272,10 +274,19 @@ int hv_ringbuffer_write(struct vmbus_channel *channel,
 	 * is empty since the read index == write index.
 	 */
 	if (bytes_avail_towrite <= totalbytes_towrite) {
+		++channel->out_full_total;
+
+		if (!channel->out_full_flag) {
+			++channel->out_full_first;
+			channel->out_full_flag = true;
+		}
+
 		spin_unlock_irqrestore(&outring_info->ring_lock, flags);
 		return -EAGAIN;
 	}
 
+	channel->out_full_flag = false;
+
 	/* Write to the ring buffer */
 	next_write_location = hv_get_next_write_location(outring_info);
 
@@ -530,6 +541,7 @@ void hv_pkt_iter_close(struct vmbus_channel *channel)
 	if (curr_write_sz <= pending_sz)
 		return;
 
+	++channel->intr_in_full;
 	vmbus_setevent(channel);
 }
 EXPORT_SYMBOL_GPL(hv_pkt_iter_close);
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 403fee01572c..b9fa30f0fc52 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1496,6 +1496,38 @@ static ssize_t channel_events_show(const struct vmbus_channel *channel, char *bu
 }
 static VMBUS_CHAN_ATTR(events, S_IRUGO, channel_events_show, NULL);
 
+static ssize_t channel_intr_in_full_show(const struct vmbus_channel *channel,
+					 char *buf)
+{
+	return sprintf(buf, "%llu\n",
+		       (unsigned long long)channel->intr_in_full);
+}
+static VMBUS_CHAN_ATTR(intr_in_full, 0444, channel_intr_in_full_show, NULL);
+
+static ssize_t channel_intr_out_empty_show(const struct vmbus_channel *channel,
+					   char *buf)
+{
+	return sprintf(buf, "%llu\n",
+		       (unsigned long long)channel->intr_out_empty);
+}
+static VMBUS_CHAN_ATTR(intr_out_empty, 0444, channel_intr_out_empty_show, NULL);
+
+static ssize_t channel_out_full_first_show(const struct vmbus_channel *channel,
+					   char *buf)
+{
+	return sprintf(buf, "%llu\n",
+		       (unsigned long long)channel->out_full_first);
+}
+static VMBUS_CHAN_ATTR(out_full_first, 0444, channel_out_full_first_show, NULL);
+
+static ssize_t channel_out_full_total_show(const struct vmbus_channel *channel,
+					   char *buf)
+{
+	return sprintf(buf, "%llu\n",
+		       (unsigned long long)channel->out_full_total);
+}
+static VMBUS_CHAN_ATTR(out_full_total, 0444, channel_out_full_total_show, NULL);
+
 static ssize_t subchannel_monitor_id_show(const struct vmbus_channel *channel,
 					  char *buf)
 {
@@ -1521,6 +1553,10 @@ static struct attribute *vmbus_chan_attrs[] = {
 	&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
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index dcb6977afce9..f1a2daece298 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -751,6 +751,19 @@ struct vmbus_channel {
 	u64	interrupts;	/* Host to Guest interrupts */
 	u64	sig_events;	/* Guest to Host events */
 
+	/*
+	 * Guest to host interrupts caused by the outbound ring buffer changing
+	 * from empty to not empty.
+	 */
+	u64 intr_out_empty;
+
+	/*
+	 * Indicates that a full outbound ring buffer was encountered. The flag
+	 * is set to true when a full outbound ring buffer is encountered and
+	 * set to false when a write to the outbound ring buffer is completed.
+	 */
+	bool out_full_flag;
+
 	/* Channel callback's invoked in softirq context */
 	struct tasklet_struct callback_event;
 	void (*onchannel_callback)(void *context);
@@ -903,6 +916,24 @@ struct vmbus_channel {
 	 * vmbus_connection.work_queue and hang: see vmbus_process_offer().
 	 */
 	struct work_struct add_channel_work;
+
+	/*
+	 * Guest to host interrupts caused by the inbound ring buffer changing
+	 * from full to not full while a packet is waiting.
+	 */
+	u64 intr_in_full;
+
+	/*
+	 * The total number of write operations that encountered a full
+	 * outbound ring buffer.
+	 */
+	u64 out_full_total;
+
+	/*
+	 * The number of write operations that were the first to encounter a
+	 * full outbound ring buffer.
+	 */
+	u64 out_full_first;
 };
 
 static inline bool is_hvsock_channel(const struct vmbus_channel *c)
@@ -936,6 +967,21 @@ static inline void *get_per_channel_state(struct vmbus_channel *c)
 static inline void set_channel_pending_send_size(struct vmbus_channel *c,
 						 u32 size)
 {
+	unsigned long flags;
+
+	if (size) {
+		spin_lock_irqsave(&c->outbound.ring_lock, flags);
+		++c->out_full_total;
+
+		if (!c->out_full_flag) {
+			++c->out_full_first;
+			c->out_full_flag = true;
+		}
+		spin_unlock_irqrestore(&c->outbound.ring_lock, flags);
+	} else {
+		c->out_full_flag = false;
+	}
+
 	c->outbound.ring_buffer->pending_send_sz = size;
 }
 
-- 
2.17.1

RE: [PATCH v4] Drivers: hv: vmbus: Expose counters for interrupts and full conditions

[Michael Kelley]

From: Kimberly Brown <kimbrownkd@gmail.com> Sent: Sunday, February 3, 2019 11:13 PM
> 
> Counter values for per-channel interrupts and ring buffer full
> conditions are useful for investigating performance.
> 
> Expose counters in sysfs for 2 types of guest to host interrupts:
> 1) Interrupts caused by the channel's outbound ring buffer transitioning
> from empty to not empty
> 2) Interrupts caused by the channel's inbound ring buffer transitioning
> from full to not full while a packet is waiting for enough buffer space to
> become available
> 
> Expose 2 counters in sysfs for the number of times that write operations
> encountered a full outbound ring buffer:
> 1) The total number of write operations that encountered a full
> condition
> 2) The number of write operations that were the first to encounter a
> full condition
> 
> Increment the outbound full condition counters in the
> hv_ringbuffer_write() function because, for most drivers, a full
> outbound ring buffer is detected in that function. Also increment the
> outbound full condition counters in the set_channel_pending_send_size()
> function. In the hv_sock driver, a full outbound ring buffer is detected
> and set_channel_pending_send_size() is called before
> hv_ringbuffer_write() is called.
> 
> I tested this patch by confirming that the sysfs files were created and
> observing the counter values. The values seemed to increase by a
> reasonable amount when the Hyper-v related drivers were in use.
> 
> Signed-off-by: Kimberly Brown <kimbrownkd@gmail.com>
> ---
> 
Reviewed-by:  Michael Kelley <mikelley@microsoft.com>

Re: [PATCH v4] Drivers: hv: vmbus: Expose counters for interrupts and full conditions

[Sasha Levin]

On Mon, Feb 04, 2019 at 04:25:34PM +0000, Michael Kelley wrote:
>From: Kimberly Brown <kimbrownkd@gmail.com> Sent: Sunday, February 3, 2019 11:13 PM
>>
>> Counter values for per-channel interrupts and ring buffer full
>> conditions are useful for investigating performance.
>>
>> Expose counters in sysfs for 2 types of guest to host interrupts:
>> 1) Interrupts caused by the channel's outbound ring buffer transitioning
>> from empty to not empty
>> 2) Interrupts caused by the channel's inbound ring buffer transitioning
>> from full to not full while a packet is waiting for enough buffer space to
>> become available
>>
>> Expose 2 counters in sysfs for the number of times that write operations
>> encountered a full outbound ring buffer:
>> 1) The total number of write operations that encountered a full
>> condition
>> 2) The number of write operations that were the first to encounter a
>> full condition
>>
>> Increment the outbound full condition counters in the
>> hv_ringbuffer_write() function because, for most drivers, a full
>> outbound ring buffer is detected in that function. Also increment the
>> outbound full condition counters in the set_channel_pending_send_size()
>> function. In the hv_sock driver, a full outbound ring buffer is detected
>> and set_channel_pending_send_size() is called before
>> hv_ringbuffer_write() is called.
>>
>> I tested this patch by confirming that the sysfs files were created and
>> observing the counter values. The values seemed to increase by a
>> reasonable amount when the Hyper-v related drivers were in use.
>>
>> Signed-off-by: Kimberly Brown <kimbrownkd@gmail.com>
>> ---
>>
>Reviewed-by:  Michael Kelley <mikelley@microsoft.com>

Queued for hyperv-next, thanks Kimberly!

--
Thanks,
Sasha