Author: delphij Date: Wed Jan 13 08:22:53 2016 New Revision: 293820 URL: https://svnweb.freebsd.org/changeset/base/293820
Log: MFC r292861: hyperv: vmbus: run non-blocking message handlers in vmbus_msg_swintr() We'll remove the per-channel control_work_queue because it can't properly do serialization of message handling, e.g., when there are 2 NIC devices, vmbus_channel_on_offer() -> hv_queue_work_item() has a race condition: for an SMP VM, vmbus_channel_process_offer() can run concurrently on different CPUs and if the second NIC's vmbus_channel_process_offer() -> hv_vmbus_child_device_register() runs first, the second NIC's name will be hn0 and the first NIC's name will be hn1! We can fix the race condition by removing the per-channel control_work_queue and run all the message handlers in the global hv_vmbus_g_connection.work_queue -- we'll do this in the next patch. With the coming next patch, we have to run the non-blocking handlers directly in the kernel thread vmbus_msg_swintr(), because the special handling of sub-channel: when a sub-channel (e.g., of the storvsc driver) is received and being handled in vmbus_channel_on_offer() running on the global hv_vmbus_g_connection.work_queue, vmbus_channel_process_offer() invokes channel->sc_creation_callback, i.e., storvsc_handle_sc_creation, and the callback will invoke hv_vmbus_channel_open() -> hv_vmbus_post_message and expect a further reply from the host, but the handling of the further messag can't be done because the current message's handling hasn't finished yet; as result, hv_vmbus_channel_open() -> sema_timedwait() will time out and th device can't work. Also renamed the handler type from hv_pfn_channel_msg_handler to vmbus_msg_handler: the 'pfn' and 'channel' in the old name make no sense. Submitted by: Dexuan Cui <decui microsoft com> Reviewed by: royger Differential Revision: https://reviews.freebsd.org/D4596 MFC r292859: hyperv: vmbus: remove the per-channel control_work_queue Now vmbus_channel_on_offer() -> vmbus_channel_process_offer() can safely run on the global hv_vmbus_g_connection.work_queue now. We remove the per-channel control_work_queue to achieve the proper serialization of the message handling. I removed the bogus TODO in vmbus_channel_on_offer(): a vmbus offer can only come from the parent partition, i.e., the host. PR: kern/205156 Submitted by: Dexuan Cui <decui microsoft com> Reviewed by: Howard Su <howard0su gmail com>, delphij Differential Revision: https://reviews.freebsd.org/D4597 Modified: stable/10/sys/dev/hyperv/include/hyperv.h stable/10/sys/dev/hyperv/vmbus/hv_channel_mgmt.c stable/10/sys/dev/hyperv/vmbus/hv_vmbus_drv_freebsd.c stable/10/sys/dev/hyperv/vmbus/hv_vmbus_priv.h Directory Properties: stable/10/ (props changed) Modified: stable/10/sys/dev/hyperv/include/hyperv.h ============================================================================== --- stable/10/sys/dev/hyperv/include/hyperv.h Wed Jan 13 08:09:28 2016 (r293819) +++ stable/10/sys/dev/hyperv/include/hyperv.h Wed Jan 13 08:22:53 2016 (r293820) @@ -759,7 +759,6 @@ typedef struct hv_vmbus_channel { hv_vmbus_ring_buffer_info inbound; struct mtx inbound_lock; - hv_vmbus_handle control_work_queue; hv_vmbus_pfn_channel_callback on_channel_callback; void* channel_callback_context; Modified: stable/10/sys/dev/hyperv/vmbus/hv_channel_mgmt.c ============================================================================== --- stable/10/sys/dev/hyperv/vmbus/hv_channel_mgmt.c Wed Jan 13 08:09:28 2016 (r293819) +++ stable/10/sys/dev/hyperv/vmbus/hv_channel_mgmt.c Wed Jan 13 08:22:53 2016 (r293820) @@ -34,13 +34,6 @@ __FBSDID("$FreeBSD$"); #include "hv_vmbus_priv.h" -typedef void (*hv_pfn_channel_msg_handler)(hv_vmbus_channel_msg_header* msg); - -typedef struct hv_vmbus_channel_msg_table_entry { - hv_vmbus_channel_msg_type messageType; - hv_pfn_channel_msg_handler messageHandler; -} hv_vmbus_channel_msg_table_entry; - /* * Internal functions */ @@ -52,36 +45,46 @@ static void vmbus_channel_on_gpadl_creat static void vmbus_channel_on_gpadl_torndown(hv_vmbus_channel_msg_header* hdr); static void vmbus_channel_on_offers_delivered(hv_vmbus_channel_msg_header* hdr); static void vmbus_channel_on_version_response(hv_vmbus_channel_msg_header* hdr); -static void vmbus_channel_process_offer(void *context); /** * Channel message dispatch table */ hv_vmbus_channel_msg_table_entry g_channel_message_table[HV_CHANNEL_MESSAGE_COUNT] = { - { HV_CHANNEL_MESSAGE_INVALID, NULL }, - { HV_CHANNEL_MESSAGE_OFFER_CHANNEL, vmbus_channel_on_offer }, + { HV_CHANNEL_MESSAGE_INVALID, + 0, NULL }, + { HV_CHANNEL_MESSAGE_OFFER_CHANNEL, + 0, vmbus_channel_on_offer }, { HV_CHANNEL_MESSAGE_RESCIND_CHANNEL_OFFER, - vmbus_channel_on_offer_rescind }, - { HV_CHANNEL_MESSAGE_REQUEST_OFFERS, NULL }, + 0, vmbus_channel_on_offer_rescind }, + { HV_CHANNEL_MESSAGE_REQUEST_OFFERS, + 0, NULL }, { HV_CHANNEL_MESSAGE_ALL_OFFERS_DELIVERED, - vmbus_channel_on_offers_delivered }, - { HV_CHANNEL_MESSAGE_OPEN_CHANNEL, NULL }, + 1, vmbus_channel_on_offers_delivered }, + { HV_CHANNEL_MESSAGE_OPEN_CHANNEL, + 0, NULL }, { HV_CHANNEL_MESSAGE_OPEN_CHANNEL_RESULT, - vmbus_channel_on_open_result }, - { HV_CHANNEL_MESSAGE_CLOSE_CHANNEL, NULL }, - { HV_CHANNEL_MESSAGEL_GPADL_HEADER, NULL }, - { HV_CHANNEL_MESSAGE_GPADL_BODY, NULL }, + 1, vmbus_channel_on_open_result }, + { HV_CHANNEL_MESSAGE_CLOSE_CHANNEL, + 0, NULL }, + { HV_CHANNEL_MESSAGEL_GPADL_HEADER, + 0, NULL }, + { HV_CHANNEL_MESSAGE_GPADL_BODY, + 0, NULL }, { HV_CHANNEL_MESSAGE_GPADL_CREATED, - vmbus_channel_on_gpadl_created }, - { HV_CHANNEL_MESSAGE_GPADL_TEARDOWN, NULL }, + 1, vmbus_channel_on_gpadl_created }, + { HV_CHANNEL_MESSAGE_GPADL_TEARDOWN, + 0, NULL }, { HV_CHANNEL_MESSAGE_GPADL_TORNDOWN, - vmbus_channel_on_gpadl_torndown }, - { HV_CHANNEL_MESSAGE_REL_ID_RELEASED, NULL }, - { HV_CHANNEL_MESSAGE_INITIATED_CONTACT, NULL }, + 1, vmbus_channel_on_gpadl_torndown }, + { HV_CHANNEL_MESSAGE_REL_ID_RELEASED, + 0, NULL }, + { HV_CHANNEL_MESSAGE_INITIATED_CONTACT, + 0, NULL }, { HV_CHANNEL_MESSAGE_VERSION_RESPONSE, - vmbus_channel_on_version_response }, - { HV_CHANNEL_MESSAGE_UNLOAD, NULL } + 1, vmbus_channel_on_version_response }, + { HV_CHANNEL_MESSAGE_UNLOAD, + 0, NULL } }; @@ -209,15 +212,6 @@ hv_queue_work_item( return (taskqueue_enqueue(wq->queue, &w->work)); } -/** - * @brief Rescind the offer by initiating a device removal - */ -static void -vmbus_channel_process_rescind_offer(void *context) -{ - hv_vmbus_channel* channel = (hv_vmbus_channel*) context; - hv_vmbus_child_device_unregister(channel->device); -} /** * @brief Allocate and initialize a vmbus channel object @@ -240,14 +234,6 @@ hv_vmbus_allocate_channel(void) TAILQ_INIT(&channel->sc_list_anchor); - channel->control_work_queue = hv_work_queue_create("control"); - - if (channel->control_work_queue == NULL) { - mtx_destroy(&channel->inbound_lock); - free(channel, M_DEVBUF); - return (NULL); - } - return (channel); } @@ -258,7 +244,6 @@ static inline void ReleaseVmbusChannel(void *context) { hv_vmbus_channel* channel = (hv_vmbus_channel*) context; - hv_work_queue_close(channel->control_work_queue); free(channel, M_DEVBUF); } @@ -284,14 +269,12 @@ hv_vmbus_free_vmbus_channel(hv_vmbus_cha * associated with this offer */ static void -vmbus_channel_process_offer(void *context) +vmbus_channel_process_offer(hv_vmbus_channel *new_channel) { - hv_vmbus_channel* new_channel; boolean_t f_new; hv_vmbus_channel* channel; int ret; - new_channel = (hv_vmbus_channel*) context; f_new = TRUE; channel = NULL; @@ -524,11 +507,7 @@ vmbus_channel_on_offer(hv_vmbus_channel_ new_channel->monitor_group = (uint8_t) offer->monitor_id / 32; new_channel->monitor_bit = (uint8_t) offer->monitor_id % 32; - /* TODO: Make sure the offer comes from our parent partition */ - hv_queue_work_item( - new_channel->control_work_queue, - vmbus_channel_process_offer, - new_channel); + vmbus_channel_process_offer(new_channel); } /** @@ -549,8 +528,7 @@ vmbus_channel_on_offer_rescind(hv_vmbus_ if (channel == NULL) return; - hv_queue_work_item(channel->control_work_queue, - vmbus_channel_process_rescind_offer, channel); + hv_vmbus_child_device_unregister(channel->device); } /** Modified: stable/10/sys/dev/hyperv/vmbus/hv_vmbus_drv_freebsd.c ============================================================================== --- stable/10/sys/dev/hyperv/vmbus/hv_vmbus_drv_freebsd.c Wed Jan 13 08:09:28 2016 (r293819) +++ stable/10/sys/dev/hyperv/vmbus/hv_vmbus_drv_freebsd.c Wed Jan 13 08:22:53 2016 (r293820) @@ -76,8 +76,12 @@ vmbus_msg_swintr(void *arg) { int cpu; void* page_addr; + hv_vmbus_channel_msg_header *hdr; + hv_vmbus_channel_msg_table_entry *entry; + hv_vmbus_channel_msg_type msg_type; hv_vmbus_message* msg; hv_vmbus_message* copied; + static bool warned = false; cpu = (int)(long)arg; KASSERT(cpu <= mp_maxid, ("VMBUS: vmbus_msg_swintr: " @@ -87,9 +91,24 @@ vmbus_msg_swintr(void *arg) msg = (hv_vmbus_message*) page_addr + HV_VMBUS_MESSAGE_SINT; for (;;) { - if (msg->header.message_type == HV_MESSAGE_TYPE_NONE) { + if (msg->header.message_type == HV_MESSAGE_TYPE_NONE) break; /* no message */ - } else { + + hdr = (hv_vmbus_channel_msg_header *)msg->u.payload; + msg_type = hdr->message_type; + + if (msg_type >= HV_CHANNEL_MESSAGE_COUNT && !warned) { + warned = true; + printf("VMBUS: unknown message type = %d\n", msg_type); + goto handled; + } + + entry = &g_channel_message_table[msg_type]; + + if (entry->handler_no_sleep) + entry->messageHandler(hdr); + else { + copied = malloc(sizeof(hv_vmbus_message), M_DEVBUF, M_NOWAIT); KASSERT(copied != NULL, @@ -97,11 +116,13 @@ vmbus_msg_swintr(void *arg) " hv_vmbus_message!")); if (copied == NULL) continue; + memcpy(copied, msg, sizeof(hv_vmbus_message)); hv_queue_work_item(hv_vmbus_g_connection.work_queue, - hv_vmbus_on_channel_message, copied); - } - + hv_vmbus_on_channel_message, + copied); + } +handled: msg->header.message_type = HV_MESSAGE_TYPE_NONE; /* Modified: stable/10/sys/dev/hyperv/vmbus/hv_vmbus_priv.h ============================================================================== --- stable/10/sys/dev/hyperv/vmbus/hv_vmbus_priv.h Wed Jan 13 08:09:28 2016 (r293819) +++ stable/10/sys/dev/hyperv/vmbus/hv_vmbus_priv.h Wed Jan 13 08:22:53 2016 (r293820) @@ -586,6 +586,16 @@ typedef enum { extern hv_vmbus_context hv_vmbus_g_context; extern hv_vmbus_connection hv_vmbus_g_connection; +typedef void (*vmbus_msg_handler)(hv_vmbus_channel_msg_header *msg); + +typedef struct hv_vmbus_channel_msg_table_entry { + hv_vmbus_channel_msg_type messageType; + + bool handler_no_sleep; /* true: the handler doesn't sleep */ + vmbus_msg_handler messageHandler; +} hv_vmbus_channel_msg_table_entry; + +extern hv_vmbus_channel_msg_table_entry g_channel_message_table[]; /* * Private, VM Bus functions _______________________________________________ svn-src-stable-10@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-stable-10 To unsubscribe, send any mail to "svn-src-stable-10-unsubscr...@freebsd.org"
