Date: Tue, 11 Oct 2016 08:22:18 +0000 (UTC) From: Sepherosa Ziehau <sephe@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-10@freebsd.org Subject: svn commit: r307028 - in stable/10/sys/dev/hyperv: include vmbus Message-ID: <201610110822.u9B8MIH8030431@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: sephe Date: Tue Oct 11 08:22:17 2016 New Revision: 307028 URL: https://svnweb.freebsd.org/changeset/base/307028 Log: MFC 302816-302818 302816 hyperv/vmbus: Release vmbus channel lock before detach devices Device detach method may sleep. While I'm here, rename the function, fix indentation and function comment. Sponsored by: Microsoft OSTC Differential Revision: https://reviews.freebsd.org/D7110 302817 hyperv/vmbus: Field renaming to reflect reality Sponsored by: Microsoft OSTC Differential Revision: https://reviews.freebsd.org/D7111 302818 hyperv/vmbus: Fix the racy channel close. It is not safe to iterate the sub-channel list w/o lock on the close path, while it's even more difficult to hold the lock and iterate the sub-channel list. We leverage the vmbua_{get,rel}_subchan() functions to solve this dilemma. Sponsored by: Microsoft OSTC Differential Revision: https://reviews.freebsd.org/D7112 Modified: stable/10/sys/dev/hyperv/include/hyperv.h stable/10/sys/dev/hyperv/vmbus/hv_channel.c stable/10/sys/dev/hyperv/vmbus/hv_channel_mgmt.c stable/10/sys/dev/hyperv/vmbus/hv_vmbus_priv.h stable/10/sys/dev/hyperv/vmbus/vmbus.c stable/10/sys/dev/hyperv/vmbus/vmbus_var.h Directory Properties: stable/10/ (props changed) Modified: stable/10/sys/dev/hyperv/include/hyperv.h ============================================================================== --- stable/10/sys/dev/hyperv/include/hyperv.h Tue Oct 11 08:14:11 2016 (r307027) +++ stable/10/sys/dev/hyperv/include/hyperv.h Tue Oct 11 08:22:17 2016 (r307028) @@ -326,7 +326,7 @@ typedef struct hv_vmbus_channel { void *hv_chan_priv3; struct task ch_detach_task; - TAILQ_ENTRY(hv_vmbus_channel) ch_link; + TAILQ_ENTRY(hv_vmbus_channel) ch_prilink; /* primary chan link */ uint32_t ch_subidx; /* subchan index */ volatile uint32_t ch_stflags; /* atomic-op */ /* VMBUS_CHAN_ST_ */ Modified: stable/10/sys/dev/hyperv/vmbus/hv_channel.c ============================================================================== --- stable/10/sys/dev/hyperv/vmbus/hv_channel.c Tue Oct 11 08:14:11 2016 (r307027) +++ stable/10/sys/dev/hyperv/vmbus/hv_channel.c Tue Oct 11 08:22:17 2016 (r307028) @@ -542,35 +542,40 @@ hv_vmbus_channel_close_internal(hv_vmbus M_DEVBUF); } -/** - * @brief Close the specified channel +/* + * Caller should make sure that all sub-channels have + * been added to 'chan' and all to-be-closed channels + * are not being opened. */ void -hv_vmbus_channel_close(hv_vmbus_channel *channel) +hv_vmbus_channel_close(struct hv_vmbus_channel *chan) { - hv_vmbus_channel* sub_channel; + int subchan_cnt; - if (channel->primary_channel != NULL) { + if (!VMBUS_CHAN_ISPRIMARY(chan)) { /* - * We only close multi-channels when the primary is - * closed. + * Sub-channel is closed when its primary channel + * is closed; done. */ return; } /* - * Close all multi-channels first. + * Close all sub-channels, if any. */ - TAILQ_FOREACH(sub_channel, &channel->sc_list_anchor, - sc_list_entry) { - if ((sub_channel->ch_stflags & VMBUS_CHAN_ST_OPENED) == 0) - continue; - hv_vmbus_channel_close_internal(sub_channel); + subchan_cnt = chan->subchan_cnt; + if (subchan_cnt > 0) { + struct hv_vmbus_channel **subchan; + int i; + + subchan = vmbus_get_subchan(chan, subchan_cnt); + for (i = 0; i < subchan_cnt; ++i) + hv_vmbus_channel_close_internal(subchan[i]); + vmbus_rel_subchan(subchan, subchan_cnt); } - /* - * Then close the primary channel. - */ - hv_vmbus_channel_close_internal(channel); + + /* Then close the primary channel. */ + hv_vmbus_channel_close_internal(chan); } /** Modified: stable/10/sys/dev/hyperv/vmbus/hv_channel_mgmt.c ============================================================================== --- stable/10/sys/dev/hyperv/vmbus/hv_channel_mgmt.c Tue Oct 11 08:14:11 2016 (r307027) +++ stable/10/sys/dev/hyperv/vmbus/hv_channel_mgmt.c Tue Oct 11 08:22:17 2016 (r307028) @@ -137,8 +137,8 @@ vmbus_chan_add(struct hv_vmbus_channel * newchan->ch_id, newchan->ch_subidx); } - mtx_lock(&sc->vmbus_chlist_lock); - TAILQ_FOREACH(prichan, &sc->vmbus_chlist, ch_link) { + mtx_lock(&sc->vmbus_prichan_lock); + TAILQ_FOREACH(prichan, &sc->vmbus_prichans, ch_prilink) { if (memcmp(&prichan->ch_guid_type, &newchan->ch_guid_type, sizeof(struct hyperv_guid)) == 0 && memcmp(&prichan->ch_guid_inst, &newchan->ch_guid_inst, @@ -148,18 +148,19 @@ vmbus_chan_add(struct hv_vmbus_channel * if (VMBUS_CHAN_ISPRIMARY(newchan)) { if (prichan == NULL) { /* Install the new primary channel */ - TAILQ_INSERT_TAIL(&sc->vmbus_chlist, newchan, ch_link); - mtx_unlock(&sc->vmbus_chlist_lock); + TAILQ_INSERT_TAIL(&sc->vmbus_prichans, newchan, + ch_prilink); + mtx_unlock(&sc->vmbus_prichan_lock); return 0; } else { - mtx_unlock(&sc->vmbus_chlist_lock); + mtx_unlock(&sc->vmbus_prichan_lock); device_printf(sc->vmbus_dev, "duplicated primary " "chan%u\n", newchan->ch_id); return EINVAL; } } else { /* Sub-channel */ if (prichan == NULL) { - mtx_unlock(&sc->vmbus_chlist_lock); + mtx_unlock(&sc->vmbus_prichan_lock); device_printf(sc->vmbus_dev, "no primary chan for " "chan%u\n", newchan->ch_id); return EINVAL; @@ -171,7 +172,7 @@ vmbus_chan_add(struct hv_vmbus_channel * * XXX refcnt prichan */ } - mtx_unlock(&sc->vmbus_chlist_lock); + mtx_unlock(&sc->vmbus_prichan_lock); /* * This is a sub-channel; link it with the primary channel. @@ -401,28 +402,28 @@ vmbus_channel_on_offers_delivered(struct vmbus_scan_done(sc); } -/** - * @brief Release channels that are unattached/unconnected (i.e., no drivers associated) +/* + * Detach all devices and destroy the corresponding primary channels. */ void -hv_vmbus_release_unattached_channels(struct vmbus_softc *sc) +vmbus_chan_destroy_all(struct vmbus_softc *sc) { - hv_vmbus_channel *channel; + struct hv_vmbus_channel *chan; - mtx_lock(&sc->vmbus_chlist_lock); + mtx_lock(&sc->vmbus_prichan_lock); + while ((chan = TAILQ_FIRST(&sc->vmbus_prichans)) != NULL) { + KASSERT(VMBUS_CHAN_ISPRIMARY(chan), ("not primary channel")); + TAILQ_REMOVE(&sc->vmbus_prichans, chan, ch_prilink); + mtx_unlock(&sc->vmbus_prichan_lock); - while (!TAILQ_EMPTY(&sc->vmbus_chlist)) { - channel = TAILQ_FIRST(&sc->vmbus_chlist); - KASSERT(VMBUS_CHAN_ISPRIMARY(channel), ("not primary channel")); - TAILQ_REMOVE(&sc->vmbus_chlist, channel, ch_link); + hv_vmbus_child_device_unregister(chan); + vmbus_chan_free(chan); - hv_vmbus_child_device_unregister(channel); - vmbus_chan_free(channel); + mtx_lock(&sc->vmbus_prichan_lock); } bzero(sc->vmbus_chmap, sizeof(struct hv_vmbus_channel *) * VMBUS_CHAN_MAX); - - mtx_unlock(&sc->vmbus_chlist_lock); + mtx_unlock(&sc->vmbus_prichan_lock); } /** Modified: stable/10/sys/dev/hyperv/vmbus/hv_vmbus_priv.h ============================================================================== --- stable/10/sys/dev/hyperv/vmbus/hv_vmbus_priv.h Tue Oct 11 08:14:11 2016 (r307027) +++ stable/10/sys/dev/hyperv/vmbus/hv_vmbus_priv.h Tue Oct 11 08:22:17 2016 (r307028) @@ -146,9 +146,6 @@ void hv_ring_buffer_read_begin( uint32_t hv_ring_buffer_read_end( hv_vmbus_ring_buffer_info *ring_info); -void hv_vmbus_release_unattached_channels( - struct vmbus_softc *); - int hv_vmbus_child_device_register( struct hv_vmbus_channel *chan); int hv_vmbus_child_device_unregister( Modified: stable/10/sys/dev/hyperv/vmbus/vmbus.c ============================================================================== --- stable/10/sys/dev/hyperv/vmbus/vmbus.c Tue Oct 11 08:14:11 2016 (r307027) +++ stable/10/sys/dev/hyperv/vmbus/vmbus.c Tue Oct 11 08:22:17 2016 (r307028) @@ -1184,8 +1184,8 @@ vmbus_doattach(struct vmbus_softc *sc) mtx_init(&sc->vmbus_scan_lock, "vmbus scan", NULL, MTX_DEF); sc->vmbus_gpadl = VMBUS_GPADL_START; - mtx_init(&sc->vmbus_chlist_lock, "vmbus chlist", NULL, MTX_DEF); - TAILQ_INIT(&sc->vmbus_chlist); + mtx_init(&sc->vmbus_prichan_lock, "vmbus prichan", NULL, MTX_DEF); + TAILQ_INIT(&sc->vmbus_prichans); sc->vmbus_chmap = malloc( sizeof(struct hv_vmbus_channel *) * VMBUS_CHAN_MAX, M_DEVBUF, M_WAITOK | M_ZERO); @@ -1256,7 +1256,7 @@ cleanup: } free(sc->vmbus_chmap, M_DEVBUF); mtx_destroy(&sc->vmbus_scan_lock); - mtx_destroy(&sc->vmbus_chlist_lock); + mtx_destroy(&sc->vmbus_prichan_lock); return (ret); } @@ -1314,7 +1314,7 @@ vmbus_detach(device_t dev) { struct vmbus_softc *sc = device_get_softc(dev); - hv_vmbus_release_unattached_channels(sc); + vmbus_chan_destroy_all(sc); vmbus_disconnect(sc); @@ -1333,7 +1333,7 @@ vmbus_detach(device_t dev) free(sc->vmbus_chmap, M_DEVBUF); mtx_destroy(&sc->vmbus_scan_lock); - mtx_destroy(&sc->vmbus_chlist_lock); + mtx_destroy(&sc->vmbus_prichan_lock); return (0); } Modified: stable/10/sys/dev/hyperv/vmbus/vmbus_var.h ============================================================================== --- stable/10/sys/dev/hyperv/vmbus/vmbus_var.h Tue Oct 11 08:14:11 2016 (r307027) +++ stable/10/sys/dev/hyperv/vmbus/vmbus_var.h Tue Oct 11 08:22:17 2016 (r307028) @@ -101,8 +101,9 @@ struct vmbus_softc { #define VMBUS_SCAN_CHCNT_DONE 0x80000000 uint32_t vmbus_scan_devcnt; - struct mtx vmbus_chlist_lock; - TAILQ_HEAD(, hv_vmbus_channel) vmbus_chlist; + /* Primary channels */ + struct mtx vmbus_prichan_lock; + TAILQ_HEAD(, hv_vmbus_channel) vmbus_prichans; }; #define VMBUS_FLAG_ATTACHED 0x0001 /* vmbus was attached */ @@ -137,6 +138,7 @@ void vmbus_handle_intr(struct trapframe void vmbus_et_intr(struct trapframe *); void vmbus_chan_msgproc(struct vmbus_softc *, const struct vmbus_message *); +void vmbus_chan_destroy_all(struct vmbus_softc *); struct vmbus_msghc *vmbus_msghc_get(struct vmbus_softc *, size_t); void vmbus_msghc_put(struct vmbus_softc *, struct vmbus_msghc *);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201610110822.u9B8MIH8030431>