hyperv: vmbus: add statistic (stats) for ring buffer

[honzhan_microsoft.com]

Summary

The ring buffer's information, including read_index, write_index, interrupt_mask, read available, and write available is important for debugging, especially on Azure environment. In order to fill the gap, here use sysctl to print those information.

Some notes about why I choose this implementation:

(1) This implementation considers stats for ring buffers in both channels and sub channels.
(2) Here I used SYSCTL_ADD_PROC instead of SYSCTL_ADD_INT, because read/write available needs calculation dynamically. That cannot be achieved through SYSCTL_ADD_INT.
(3) The values of read_index and write_index change dynamically, so both of them should be captured at the same time in order to avoid getting inconsistent values. That is why I used CTLTYPE_STRING instead of CTLTYPE_INT. Thus, all of those inforamation related to a ring buffer can be captured and print out at a special point.
(4) There is no lock to guarantee 100 percent consistent for read_index and write_index. It is possible to get inconsistent values under high pressure. There is no high accurate requirement on statistic. We just use it to detect whether the communication between VM and host is still working.

The output of sysctl is like:

dev.hn.0.channel.14.out.ring_buffer_stats: r_idx:46600 w_idx:46600 int_mask:0 r_avail:0 w_avail:520192
dev.hn.0.channel.14.in.ring_buffer_stats: r_idx:57744 w_idx:57744 int_mask:0 r_avail:0 w_avail:520192
dev.hvkvp.0.channel.1.out.ring_buffer_stats: r_idx:4456 w_idx:4456 int_mask:0 r_avail:0 w_avail:12288
dev.hvkvp.0.channel.1.in.ring_buffer_stats: r_idx:4456 w_idx:4456 int_mask:0 r_avail:0 w_avail:12288
dev.hvtimesync.0.channel.11.out.ring_buffer_stats: r_idx:4000 w_idx:4000 int_mask:0 r_avail:0 w_avail:12288
dev.hvtimesync.0.channel.11.in.ring_buffer_stats: r_idx:4000 w_idx:4000 int_mask:0 r_avail:0 w_avail:12288
dev.hvshutdown.0.channel.10.out.ring_buffer_stats: r_idx:80 w_idx:80 int_mask:0 r_avail:0 w_avail:12288
dev.hvshutdown.0.channel.10.in.ring_buffer_stats: r_idx:80 w_idx:80 int_mask:0 r_avail:0 w_avail:12288
dev.hvheartbeat.0.channel.8.out.ring_buffer_stats: r_idx:4400 w_idx:4400 int_mask:0 r_avail:0 w_avail:12288
dev.hvheartbeat.0.channel.8.in.ring_buffer_stats: r_idx:4400 w_idx:4400 int_mask:0 r_avail:0 w_avail:12288
dev.storvsc.1.channel.15.sub.1.out.ring_buffer_stats: r_idx:0 w_idx:0 int_mask:0 r_avail:0 w_avail:77824
dev.storvsc.1.channel.15.sub.1.in.ring_buffer_stats: r_idx:0 w_idx:0 int_mask:0 r_avail:0 w_avail:77824
dev.storvsc.1.channel.15.out.ring_buffer_stats: r_idx:664 w_idx:664 int_mask:0 r_avail:0 w_avail:77824
dev.storvsc.1.channel.15.in.ring_buffer_stats: r_idx:616 w_idx:616 int_mask:0 r_avail:0 w_avail:77824
dev.storvsc.0.channel.2.out.ring_buffer_stats: r_idx:15128 w_idx:15128 int_mask:0 r_avail:0 w_avail:77824
dev.storvsc.0.channel.2.in.ring_buffer_stats: r_idx:29800 w_idx:29800 int_mask:0 r_avail:0 w_avail:77824

Submitted by: Hongjiang Zhang <honzhan microsoft com>

[sepherosa_gmail.com]

I don't think it deserves its own module and src directory, please put the files under vmbus.

[honzhan_microsoft.com]

hmm, it makes sense to put ring_buffer_stats function into hv_ring_buffer.c since only one function exists in that file.

[howard0su_gmail.com]

I suggest we don't create other node under sysctl. each device already have the node in sysctl tree. dev.hn.0 for example. I suggest to put the stats under it.

Second, naming two nodes of ringbuffer as incoming and outgoing buffer as one is readonly and another is writeonly.

sys/dev/hyperv/vmbus/hv_ring_buffer.c

57 ↗

(On Diff #13639)

do you consider to acquire lock here to get a stable snapshot?

sys/dev/hyperv/vmbus/hv_vmbus_drv_freebsd.c

75 ↗

(On Diff #13639)

Please complete the comments like "The stats for Hyper-V drivers."

sys/dev/hyperv/vmbus/hv_vmbus_priv.h

642 ↗

(On Diff #13639)

Please follow naming conversation in this file. Also I would suggest to consider put this into hv_vmbus_ring_buffer_init instead of open another interface.

[honzhan_microsoft.com]

My intension of creating more nodes is for printing clear message. We have to distinguish ring buffers on different channels, so they belong to the same node. Maybe we also needs to distinguish subchannels. Now I have not done this.

"naming two nodes of ringbuffer as incoming and outgoing buffer as one is readonly and another is writeonly"
What is the purpose of creating writeonly node? IMHO, the statistic information should be readonly.

sys/dev/hyperv/vmbus/hv_ring_buffer.c

57 ↗

(On Diff #13639)

I do not want to hurt any performance in current stage since we are tuning. If we require it in the future, we can add code to acquire lock here.

sys/dev/hyperv/vmbus/hv_vmbus_drv_freebsd.c

75 ↗

(On Diff #13639)

Sure, I'll update it.

sys/dev/hyperv/vmbus/hv_vmbus_priv.h

642 ↗

(On Diff #13639)

You are right. It makes more sense to put it into hv_vmbus_ring_buffer_init.

[sepherosa_gmail.com]

sys/dev/hyperv/vmbus/hv_ring_buffer.c

44 ↗

(On Diff #13639)

You could declare it on the rbi_sysctl_stats() stack, i think 128B should be enough. Use a global variable like this is calling for trouble later.

57 ↗

(On Diff #13639)

It's fine, they are just for debugging, _once_ there are issues.

62 ↗

(On Diff #13639)

I'd suggest to strip the white space between : and %d, so it will look like:
r_idx:XXX w_idx:XXXX ...

So they are much easier to parse in scripts or using other programmatic way.

[honzhan_microsoft.com]

The update includes:

1. Reuse "dev.DEV.UNIT." as sysctl root node instead of defining a new node.
2. Separate the statisitc into two parts: hv_vmbus_channel_stat and hv_ring_buffer_stat. The former is owned by channel module, and the later is for ring_buffer module. Because the statistic needs to know which ring buffer it is: inbound or outbound, that is only known in channel level. It is not suitable to bind all logic to hv_vmbus_ring_buffer_init function even though I tried it but found the logic is not clean.
3. Add the support for sub channel. There is a node "sub" in the output.

dev.hn.0.channel.14.out.ring_buffer_stats: r_idx:46600 w_idx:46600 int_mask:0 r_avail:0 w_avail:520192
dev.hn.0.channel.14.in.ring_buffer_stats: r_idx:57744 w_idx:57744 int_mask:0 r_avail:0 w_avail:520192
dev.hvkvp.0.channel.1.out.ring_buffer_stats: r_idx:4456 w_idx:4456 int_mask:0 r_avail:0 w_avail:12288
dev.hvkvp.0.channel.1.in.ring_buffer_stats: r_idx:4456 w_idx:4456 int_mask:0 r_avail:0 w_avail:12288
dev.hvtimesync.0.channel.11.out.ring_buffer_stats: r_idx:4000 w_idx:4000 int_mask:0 r_avail:0 w_avail:12288
dev.hvtimesync.0.channel.11.in.ring_buffer_stats: r_idx:4000 w_idx:4000 int_mask:0 r_avail:0 w_avail:12288
dev.hvshutdown.0.channel.10.out.ring_buffer_stats: r_idx:80 w_idx:80 int_mask:0 r_avail:0 w_avail:12288
dev.hvshutdown.0.channel.10.in.ring_buffer_stats: r_idx:80 w_idx:80 int_mask:0 r_avail:0 w_avail:12288
dev.hvheartbeat.0.channel.8.out.ring_buffer_stats: r_idx:4400 w_idx:4400 int_mask:0 r_avail:0 w_avail:12288
dev.hvheartbeat.0.channel.8.in.ring_buffer_stats: r_idx:4400 w_idx:4400 int_mask:0 r_avail:0 w_avail:12288
dev.storvsc.1.channel.15.sub.1.out.ring_buffer_stats: r_idx:0 w_idx:0 int_mask:0 r_avail:0 w_avail:77824
dev.storvsc.1.channel.15.sub.1.in.ring_buffer_stats: r_idx:0 w_idx:0 int_mask:0 r_avail:0 w_avail:77824
dev.storvsc.1.channel.15.out.ring_buffer_stats: r_idx:664 w_idx:664 int_mask:0 r_avail:0 w_avail:77824
dev.storvsc.1.channel.15.in.ring_buffer_stats: r_idx:616 w_idx:616 int_mask:0 r_avail:0 w_avail:77824
dev.storvsc.0.channel.2.out.ring_buffer_stats: r_idx:15128 w_idx:15128 int_mask:0 r_avail:0 w_avail:77824
dev.storvsc.0.channel.2.in.ring_buffer_stats: r_idx:29800 w_idx:29800 int_mask:0 r_avail:0 w_avail:77824

[honzhan_microsoft.com]

sys/dev/hyperv/vmbus/hv_ring_buffer.c

62 ↗

(On Diff #13716)

Ok.

[sepherosa_gmail.com]

It looks ok to me. If no objection comes, it will be committed next Monday.

sys/dev/hyperv/vmbus/hv_channel.c

107 ↗

(On Diff #13716)

Do you still create dev.DEVNAME.DEVUNIT.hyperv.channel? It looks like dev.DEVNAME.DEVUNIT.channel now :)

[honzhan_microsoft.com]

sys/dev/hyperv/vmbus/hv_channel.c

107 ↗

(On Diff #13716)

Oh, I need to change the comments. I found "hyperv" is not necessary for searching, so I removed it but forget to change the comments here. Let me change it.