hyperv: vmbus: Optimize channel lookup through id through a map table

[honzhan_microsoft.com]

Summary

vmbus event handler has to find the channel object through relative id
when software interrupt for event happens. The lookup process was to iterate
the channel object list to find the matched id, so it is not efficient.
Now, a map table which supports to find a channel object through relative id
can improve the lookup efficiency.

Submitted by: Hongjiang Zhang <honzhan microsoft com>

[howard0su_gmail.com]

sys/dev/hyperv/vmbus/hv_connection.c

302 ↗

(On Diff #11960)

free(9) accept NULL

335 ↗

(On Diff #11960)

Ditto

[howard0su_gmail.com]

sys/dev/hyperv/vmbus/hv_vmbus_priv.h

367 ↗

(On Diff #11960)

hv_vmbus_channel **channels;

[decui_microsoft.com]

hv_vmbus_g_connection.channels = malloc(sizeof(unsigned long) * HV_CHANNEL_MAX_COUNT,

sizeof(unsigned long) should be sizeof(hv_vmbus_channel *).

[decui_microsoft.com]

In VmbusProcessChannelEvent(), hv_vmbus_g_connection.channels[relid] still lacks the necessary protection, and in vmbus_channel_on_offer_rescind(), hv_vmbus_g_connection.channels[relid] is not reset to NULL, but I think we can address these in another patch.

[royger]

sys/dev/hyperv/vmbus/hv_connection.c

234 ↗

(On Diff #11960)

AFAICT this is called from the newbus attach routine, in which case IIRC you can safely use M_WAITOK.

302 ↗

(On Diff #11960)

Yup, and in any case this check should be against != NULL, because it's not a boolean type.

[decui_microsoft.com]

LGTM

[sepherosa_gmail.com]

sys/dev/hyperv/vmbus/hv_channel_mgmt.c

283 ↗

(On Diff #12400)

The void * cast is not needed

528–535 ↗

(On Diff #12400)

This one probably should be:

lock;
if (channels[id] == NULL) {
    unlock;
    return;
}
channels[id] = NULL;
unlock;

sys/dev/hyperv/vmbus/hv_connection.c

302 ↗

(On Diff #12400)

I'd prefer to have a NULL check before free.

357 ↗

(On Diff #12400)

Not too much related to this patch. But are we sure this is MPSAFE? In which context do we destroy the channels? swi_msg? I think the context of this lookup is in the swi_event.

[honzhan_microsoft.com]

sys/dev/hyperv/vmbus/hv_channel_mgmt.c

283 ↗

(On Diff #12408)

Yes, you are right. I will remove it.

528–536 ↗

(On Diff #12408)

This is a general good style. But for this case you can see line 529 has already done "if" test, so the value should not be NULL.

sys/dev/hyperv/vmbus/hv_connection.c

298 ↗

(On Diff #12408)

I have checked style(9), and free accepts "NULL". For code simplicity purpose, I removed the NULL check.

[sepherosa_gmail.com]

Looks fine to me. Though I have some questions about the channel accessing w/o lock/refcnt.

sys/dev/hyperv/vmbus/hv_channel_mgmt.c

528–536 ↗

(On Diff #12410)

That if test is not useful, if there are two threads run this function, will this happen?

This also relates to my question about the lookup-then-use w/o any reference counting in the swi_event.

sys/dev/hyperv/vmbus/hv_connection.c

298 ↗

(On Diff #12410)

Isn't NULL check more consistent w/ other resource cleanup in the same function? Well, after all, its your call.

[honzhan_microsoft.com]

sys/dev/hyperv/vmbus/hv_channel_mgmt.c

528–536 ↗

(On Diff #12410)

Repeately setting "NULL" does not cause issues here.

sys/dev/hyperv/vmbus/hv_connection.c

298 ↗

(On Diff #12410)

I agree with you that we should align the same rule in a small scope, file level or function level. I prefer to do a code clean in the future.

[decui_microsoft.com]

In D4802#105424, @sepherosa_gmail.com wrote:

Looks fine to me. Though I have some questions about the channel accessing w/o lock/refcnt.

Yeah, there is an issue when (a device is being hot-removed, or the driver is being unloaded) AND there is a pending event in the channel.

But I think we can address it in a new patch. :-)