hyperv/vmbus: replace hv_guid with uuid

[howard0su_gmail.com]

Summary

hv_guid shares same meaning as uuid. Use sys/uuid.h functions to
do printf, snprintf. And remove redundant code.

While I am here, add pnpinfo_str to vmbus to print device info.
Reimplement bind cpu in open_channel so device can decide instead
of a global list.

[howard0su_gmail.com]

Update the change for utility drivers

[howard0su_gmail.com]

Implement is_perf_channel parameter in open_channel call

[howard0su_gmail.com]

sys/dev/hyperv/utilities/hv_kvp.c

342

this is line to debug, however I didn't find a way to trigger this check.

[decui_microsoft.com]

sys/dev/hyperv/include/hyperv.h

124

This is defined but not used?

I would suggest we change it to
typedef struct uuid hv_guid;
so we can avoid many lines of change below.

And IMHO we'd better not use the name "uuid" everywhere -- uuid is different from guid after all. :-)
And it seems a little confusing to have
" struct uuid instance_guid" in vmbus_channel_process_offer().
(hv_guid instance_guid seems much better).

It looks here we make non-trivial changes just to make use of the built-in API snprintf_uuid().
I suggest we define a new function snprintf_guid(it only needs a few lines of code, I think) so we can avoid the changes?

827

We won't need this change if we have the above typedef and snprintf_guid().

sys/dev/hyperv/utilities/hv_heartbeat.c

41

Can you please add a comment string of the GUID here so it can be easily grep-ed? :-)

sys/dev/hyperv/utilities/hv_kvp.c

89

Add a comment of the GUID string?

339

change this to snprintf_guid(), which can be added by us?

342

It's in the HV_KVP_OP_SET_IP_INFO case, which is used for guest failover (when a guest fails on host1 or host1 fails, the guest will be started on host2 with a static IP injected into the guest. To enable this, the replica feature needs to be enabled on both hosts.

I can help to test this.

sys/dev/hyperv/utilities/hv_shutdown.c

45

Add a comment of the string GUID too?

sys/dev/hyperv/utilities/hv_timesync.c

57

Ditto

sys/dev/hyperv/vmbus/hv_channel.c

59

IMO it should be renamed to "next_cpu".

105

We'd better use this

channel->target_vcpu = hv_vmbus_g_context.hv_vcpu_index[0];

sys/dev/hyperv/vmbus/hv_channel_mgmt.c

262

Here you convert the GUID to UUID and store the UUID into a variable named type_GUID... :-)

[howard0su_gmail.com]

Let me split this change into several smaller ones for non-guid related changes.

[sepherosa_gmail.com]

sys/dev/hyperv/vmbus/hv_channel.c

131

I don't think we need this change. Yes, we want to remove the ugly 'is perf' testing when the channel is created. But we will move the cpu selection to the drivers (nail channel to cpu0 when the channel is created). I think the only driver needs conversion is stor.

[decui_microsoft.com]

sys/dev/hyperv/vmbus/hv_channel.c

131

Both storVSC and netVSC needs "is_perf".

In the future, we'll add Hyper-V sockets driver, RDMA driver and PCIe driver (for device pass-through), which need that too.

So I suggest we keep the logic in the vmbus driver, rather than the individual drivers.

[sepherosa_gmail.com]

sys/dev/hyperv/vmbus/hv_channel.c

131

Nope, netvsc already uses its own CPU mapping logic, which nullifies the requirement of the is_perf parameter; well the code in this patch actually interferes netvsc's mapping. To be frank, cpu mapping logic is driver specific for drivers really need performance. VMBUS should or even can not aware of those.

[decui_microsoft.com]

sys/dev/hyperv/vmbus/hv_channel.c

131

Hmm, I wasn't reading the latest code.

I have a concern about the latest logic in netvsc code: assuming hn_chan_cnt==8 and the VM has 32 vCPUs and there are 2 NICs:

the first NIC's sc->hn_cpu == 0 and the the 8 channels are bound to CPU 0~7;
the second NIC's sc->hn_cpu == 1 and the the 8 channels are bound to CPU 1~8;
We can see cpu 1~7 are overloaded and cpu 9~31 are idle...

If we do the logic in VMBus driver, we can have a central place where the global channel-to-cpu binding info is available, e.g., NIC1 can use cpu 0~7 and NIC2 can use 8~16, and storVSC can use 17~21, etc.

[sepherosa_gmail.com]

sys/dev/hyperv/vmbus/hv_channel.c

131

Nope, you read the code wrong. For netvsc:

static uint32_t netvsc_gidx = 0

for hn0
(ring_cnt == 8 in your case)
lead_cpu = atomic_fetchadd_int(&netvsc_gidx, ring_cnt);
/* lead_cpu is 0 here */
chan_sub0 -> cpu0
chan_sub1 -> cpu1
...
chan_sub7 -> cpu7

for hn1
(ring_cnt == 8 in your case)
lead_cpu = atomic_fetchadd_int(&netvsc_gidx, ring_cnt);
/* lead_cpu is 8 here */
chan_sub0 -> cpu8
chan_sub1 -> cpu9
...
chan_sub7 -> cpu15

And so on, and so forth.

The old way actually is wrong: the channel/multi-channel offers are mixed together and messed up netvsc ring/cpu mapping badly, causes uneven load distribution.

[decui_microsoft.com]

sys/dev/hyperv/vmbus/hv_channel.c

131

Oh, sorry for the case of netvsc. I missed the ring_cnt... :-)

Yes, the old logic in VMBus driver is bad, but I think we should fix that in VMBus side, otherwise, we'll have duplicate the same logic to storvsc and the future hyper-v sockets, RDMA & PCIe drivers, and even if we duplicate the logic, the drivers always tend to use CPUs with smaller numbers, because now the individual drivers don't share the binding info.

[sepherosa_gmail.com]

sys/dev/hyperv/vmbus/hv_channel.c

131

Sharing the global index is easy part. But we do need to have APIs for drivers to configure their cpu mapping based on their own needs, e.g. options RSS will need special network ring binding. Well, and we do need to have tunables to let user do their own mapping, instead of making the decisions for them, e.g. network gate/switch application will never care about the cpu mapping of stors.

I think the main point of this patch is to get rid of the hv_guid. Let's just do it; but please leave the cpu mapping logic out. We could fix/rework it in another patch.

[howard0su_gmail.com]

sys/dev/hyperv/include/hyperv.h

124

What I want to do is making the code more clean via decouple guid in the protocol v.s. uuid in the memory representation. Introduce a guid and a set of operations are overkill from my POV.

[decui_microsoft.com]

sys/dev/hyperv/include/hyperv.h

124

IMO we only need to add a snprintf_guid(), which is basically a wrapper of snprintf_uuid() + le_uuid_dec().

We don't need to introduce hv_guid -- it's already there and we can just update its definition to "typedef struct uuid hv_guid".

This way, the patch can be much smaller, I think.