The host can rescind an offer any time after the offer has been made
to the guest. This patch-set cleans up how we handle rescind messages
from the host.
K. Y. Srinivasan (6):
Drivers: hv: vmbus: Properly handle child device remove
Drivers: hv: vmbus: Introduce a function to remove a rescinded offer
Drivers: hv: vmbus: Handle both rescind and offer messages in the
same context
Drivers: hv: vmbus: Remove the channel from the channel list(s) on
failure
Drivers: hv: util: On device remove, close the channel after
de-initializing the service
Drivers: hv: vmbus: Get rid of some unnecessary messages
drivers/hv/channel.c | 9 ++++
drivers/hv/channel_mgmt.c | 95 ++++++++++++++++++++------------------------
drivers/hv/connection.c | 7 +---
drivers/hv/hv_util.c | 2 +-
drivers/hv/vmbus_drv.c | 26 +++++++++---
include/linux/hyperv.h | 1 +
6 files changed, 74 insertions(+), 66 deletions(-)
--
1.7.4.1
Handle the case when the device may be removed when the device has no driver
attached to it.
Signed-off-by: K. Y. Srinivasan <[email protected]>
---
drivers/hv/vmbus_drv.c | 15 +++++++++------
1 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index da4333b..04bdc0f 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -508,14 +508,17 @@ static int vmbus_probe(struct device *child_device)
*/
static int vmbus_remove(struct device *child_device)
{
- struct hv_driver *drv = drv_to_hv_drv(child_device->driver);
+ struct hv_driver *drv;
struct hv_device *dev = device_to_hv_device(child_device);
- if (drv->remove)
- drv->remove(dev);
- else
- pr_err("remove not set for driver %s\n",
- dev_name(child_device));
+ if (child_device->driver) {
+ drv = drv_to_hv_drv(child_device->driver);
+ if (drv->remove)
+ drv->remove(dev);
+ else
+ pr_err("remove not set for driver %s\n",
+ dev_name(child_device));
+ }
return 0;
}
--
1.7.4.1
In response to a rescind message, we need to remove the channel and the
corresponding device. Cleanup this code path by factoring out the code
to remove a channel.
Signed-off-by: K. Y. Srinivasan <[email protected]>
---
drivers/hv/channel.c | 9 ++++++++
drivers/hv/channel_mgmt.c | 49 +++++++++++++++++++++++++++-----------------
drivers/hv/vmbus_drv.c | 11 +++++++++-
include/linux/hyperv.h | 1 +
4 files changed, 50 insertions(+), 20 deletions(-)
diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index bf0cf8f..9b79aca 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -501,6 +501,15 @@ static int vmbus_close_internal(struct vmbus_channel *channel)
put_cpu();
}
+ /*
+ * If the channel has been rescinded; process device removal.
+ */
+ if (channel->rescind) {
+ hv_process_channel_removal(channel,
+ channel->offermsg.child_relid);
+ return 0;
+ }
+
/* Send a closing message */
msg = &channel->close_msg.msg;
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 0ba6b5c..b933891 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -207,33 +207,21 @@ static void percpu_channel_deq(void *arg)
list_del(&channel->percpu_list);
}
-/*
- * vmbus_process_rescind_offer -
- * Rescind the offer by initiating a device removal
- */
-static void vmbus_process_rescind_offer(struct work_struct *work)
+
+void hv_process_channel_removal(struct vmbus_channel *channel, u32 relid)
{
- struct vmbus_channel *channel = container_of(work,
- struct vmbus_channel,
- work);
+ struct vmbus_channel_relid_released msg;
unsigned long flags;
struct vmbus_channel *primary_channel;
- struct vmbus_channel_relid_released msg;
- struct device *dev;
-
- if (channel->device_obj) {
- dev = get_device(&channel->device_obj->device);
- if (dev) {
- vmbus_device_unregister(channel->device_obj);
- put_device(dev);
- }
- }
memset(&msg, 0, sizeof(struct vmbus_channel_relid_released));
- msg.child_relid = channel->offermsg.child_relid;
+ msg.child_relid = relid;
msg.header.msgtype = CHANNELMSG_RELID_RELEASED;
vmbus_post_msg(&msg, sizeof(struct vmbus_channel_relid_released));
+ if (channel == NULL)
+ return;
+
if (channel->target_cpu != get_cpu()) {
put_cpu();
smp_call_function_single(channel->target_cpu,
@@ -256,6 +244,29 @@ static void vmbus_process_rescind_offer(struct work_struct *work)
free_channel(channel);
}
+/*
+ * vmbus_process_rescind_offer -
+ * Rescind the offer by initiating a device removal
+ */
+static void vmbus_process_rescind_offer(struct work_struct *work)
+{
+ struct vmbus_channel *channel = container_of(work,
+ struct vmbus_channel,
+ work);
+ struct device *dev;
+
+ if (channel->device_obj) {
+ dev = get_device(&channel->device_obj->device);
+ if (dev) {
+ vmbus_device_unregister(channel->device_obj);
+ put_device(dev);
+ }
+ } else {
+ hv_process_channel_removal(channel,
+ channel->offermsg.child_relid);
+ }
+}
+
void vmbus_free_channels(void)
{
struct vmbus_channel *channel;
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 04bdc0f..f84ce5e 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -510,14 +510,23 @@ static int vmbus_remove(struct device *child_device)
{
struct hv_driver *drv;
struct hv_device *dev = device_to_hv_device(child_device);
+ u32 relid = dev->channel->offermsg.child_relid;
if (child_device->driver) {
drv = drv_to_hv_drv(child_device->driver);
if (drv->remove)
drv->remove(dev);
- else
+ else {
+ hv_process_channel_removal(dev->channel, relid);
pr_err("remove not set for driver %s\n",
dev_name(child_device));
+ }
+ } else {
+ /*
+ * We don't have a driver for this device; deal with the
+ * rescind message by removing the channel.
+ */
+ hv_process_channel_removal(dev->channel, relid);
}
return 0;
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 7d976ac..dd92a85 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -1226,6 +1226,7 @@ void hv_kvp_onchannelcallback(void *);
int hv_vss_init(struct hv_util_service *);
void hv_vss_deinit(void);
void hv_vss_onchannelcallback(void *);
+void hv_process_channel_removal(struct vmbus_channel *channel, u32 relid);
extern struct resource hyperv_mmio;
--
1.7.4.1
Execute both ressind and offer messages in the same work context. This serializes these
operations and naturally addresses the various corner cases.
Signed-off-by: K. Y. Srinivasan <[email protected]>
---
drivers/hv/channel_mgmt.c | 71 ++++++++++++--------------------------------
1 files changed, 20 insertions(+), 51 deletions(-)
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index b933891..af53168 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -244,29 +244,6 @@ void hv_process_channel_removal(struct vmbus_channel *channel, u32 relid)
free_channel(channel);
}
-/*
- * vmbus_process_rescind_offer -
- * Rescind the offer by initiating a device removal
- */
-static void vmbus_process_rescind_offer(struct work_struct *work)
-{
- struct vmbus_channel *channel = container_of(work,
- struct vmbus_channel,
- work);
- struct device *dev;
-
- if (channel->device_obj) {
- dev = get_device(&channel->device_obj->device);
- if (dev) {
- vmbus_device_unregister(channel->device_obj);
- put_device(dev);
- }
- } else {
- hv_process_channel_removal(channel,
- channel->offermsg.child_relid);
- }
-}
-
void vmbus_free_channels(void)
{
struct vmbus_channel *channel;
@@ -281,11 +258,8 @@ void vmbus_free_channels(void)
* vmbus_process_offer - Process the offer by creating a channel/device
* associated with this offer
*/
-static void vmbus_process_offer(struct work_struct *work)
+static void vmbus_process_offer(struct vmbus_channel *newchannel)
{
- struct vmbus_channel *newchannel = container_of(work,
- struct vmbus_channel,
- work);
struct vmbus_channel *channel;
bool fnew = true;
bool enq = false;
@@ -351,7 +325,7 @@ static void vmbus_process_offer(struct work_struct *work)
if (channel->sc_creation_callback != NULL)
channel->sc_creation_callback(newchannel);
- goto done_init_rescind;
+ return;
}
goto err_free_chan;
@@ -392,15 +366,9 @@ static void vmbus_process_offer(struct work_struct *work)
kfree(newchannel->device_obj);
goto err_free_chan;
}
-done_init_rescind:
- spin_lock_irqsave(&newchannel->lock, flags);
- /* The next possible work is rescind handling */
- INIT_WORK(&newchannel->work, vmbus_process_rescind_offer);
- /* Check if rescind offer was already received */
- if (newchannel->rescind)
- queue_work(newchannel->controlwq, &newchannel->work);
- spin_unlock_irqrestore(&newchannel->lock, flags);
+
return;
+
err_free_chan:
free_channel(newchannel);
}
@@ -526,8 +494,7 @@ static void vmbus_onoffer(struct vmbus_channel_message_header *hdr)
newchannel->monitor_grp = (u8)offer->monitorid / 32;
newchannel->monitor_bit = (u8)offer->monitorid % 32;
- INIT_WORK(&newchannel->work, vmbus_process_offer);
- queue_work(newchannel->controlwq, &newchannel->work);
+ vmbus_process_offer(newchannel);
}
/*
@@ -540,28 +507,30 @@ static void vmbus_onoffer_rescind(struct vmbus_channel_message_header *hdr)
struct vmbus_channel_rescind_offer *rescind;
struct vmbus_channel *channel;
unsigned long flags;
+ struct device *dev;
rescind = (struct vmbus_channel_rescind_offer *)hdr;
channel = relid2channel(rescind->child_relid);
- if (channel == NULL)
- /* Just return here, no channel found */
+ if (channel == NULL) {
+ hv_process_channel_removal(NULL, rescind->child_relid);
return;
+ }
spin_lock_irqsave(&channel->lock, flags);
channel->rescind = true;
- /*
- * channel->work.func != vmbus_process_rescind_offer means we are still
- * processing offer request and the rescind offer processing should be
- * postponed. It will be done at the very end of vmbus_process_offer()
- * as rescind flag is being checked there.
- */
- if (channel->work.func == vmbus_process_rescind_offer)
- /* work is initialized for vmbus_process_rescind_offer() from
- * vmbus_process_offer() where the channel got created */
- queue_work(channel->controlwq, &channel->work);
-
spin_unlock_irqrestore(&channel->lock, flags);
+
+ if (channel->device_obj) {
+ dev = get_device(&channel->device_obj->device);
+ if (dev) {
+ vmbus_device_unregister(channel->device_obj);
+ put_device(dev);
+ }
+ } else {
+ hv_process_channel_removal(channel,
+ channel->offermsg.child_relid);
+ }
}
/*
--
1.7.4.1
Properly rollback state in vmbus_pocess_offer() in the failure paths.
Signed-off-by: K. Y. Srinivasan <[email protected]>
---
drivers/hv/channel_mgmt.c | 21 ++++++++++++++++-----
1 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index af53168..bf9b80c 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -348,7 +348,7 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel)
&newchannel->offermsg.offer.if_instance,
newchannel);
if (!newchannel->device_obj)
- goto err_free_chan;
+ goto err_deq_chan;
/*
* Add the new device to the bus. This will kick off device-driver
@@ -360,15 +360,26 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel)
pr_err("unable to add child device object (relid %d)\n",
newchannel->offermsg.child_relid);
- spin_lock_irqsave(&vmbus_connection.channel_lock, flags);
- list_del(&newchannel->listentry);
- spin_unlock_irqrestore(&vmbus_connection.channel_lock, flags);
kfree(newchannel->device_obj);
- goto err_free_chan;
+ goto err_deq_chan;
}
return;
+err_deq_chan:
+ spin_lock_irqsave(&vmbus_connection.channel_lock, flags);
+ list_del(&newchannel->listentry);
+ spin_unlock_irqrestore(&vmbus_connection.channel_lock, flags);
+
+ if (newchannel->target_cpu != get_cpu()) {
+ put_cpu();
+ smp_call_function_single(newchannel->target_cpu,
+ percpu_channel_deq, newchannel, true);
+ } else {
+ percpu_channel_deq(newchannel);
+ put_cpu();
+ }
+
err_free_chan:
free_channel(newchannel);
}
--
1.7.4.1
When the offer is rescinded, vmbus_close() can free up the channel;
deinitialize the service before closing the channel.
Signed-off-by: K. Y. Srinivasan <[email protected]>
---
drivers/hv/hv_util.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
index c5be773..7994ec2 100644
--- a/drivers/hv/hv_util.c
+++ b/drivers/hv/hv_util.c
@@ -380,9 +380,9 @@ static int util_remove(struct hv_device *dev)
{
struct hv_util_service *srv = hv_get_drvdata(dev);
- vmbus_close(dev->channel);
if (srv->util_deinit)
srv->util_deinit();
+ vmbus_close(dev->channel);
kfree(srv->recv_buffer);
return 0;
--
1.7.4.1
Currently we log messages when either we are not able to map an ID to a
channel or when the channel does not have a callback associated
(in the channel interrupt handling path). These messages don't add
any value, get rid of them.
Signed-off-by: K. Y. Srinivasan <[email protected]>
---
drivers/hv/connection.c | 7 +------
1 files changed, 1 insertions(+), 6 deletions(-)
diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
index af2388f..583d7d4 100644
--- a/drivers/hv/connection.c
+++ b/drivers/hv/connection.c
@@ -318,10 +318,8 @@ static void process_chn_event(u32 relid)
*/
channel = pcpu_relid2channel(relid);
- if (!channel) {
- pr_err("channel not found for relid - %u\n", relid);
+ if (!channel)
return;
- }
/*
* A channel once created is persistent even when there
@@ -356,10 +354,7 @@ static void process_chn_event(u32 relid)
else
bytes_to_read = 0;
} while (read_state && (bytes_to_read != 0));
- } else {
- pr_err("no channel callback for relid - %u\n", relid);
}
-
}
/*
--
1.7.4.1
> -----Original Message-----
> From: devel [mailto:[email protected]] On Behalf
> Of K. Y. Srinivasan
> Sent: Monday, February 16, 2015 4:11 AM
> To: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]
> Subject: [PATCH 0/6] Drivers: hv: vmbus
>
> The host can rescind an offer any time after the offer has been made
> to the guest. This patch-set cleans up how we handle rescind messages
> from the host.
>
>
> K. Y. Srinivasan (6):
> Drivers: hv: vmbus: Properly handle child device remove
> Drivers: hv: vmbus: Introduce a function to remove a rescinded offer
> Drivers: hv: vmbus: Handle both rescind and offer messages in the
> same context
> Drivers: hv: vmbus: Remove the channel from the channel list(s) on
> failure
> Drivers: hv: util: On device remove, close the channel after
> de-initializing the service
> Drivers: hv: vmbus: Get rid of some unnecessary messages
>
> drivers/hv/channel.c | 9 ++++
> drivers/hv/channel_mgmt.c | 95 ++++++++++++++++++++------------------------
> drivers/hv/connection.c | 7 +---
> drivers/hv/hv_util.c | 2 +-
> drivers/hv/vmbus_drv.c | 26 +++++++++---
> include/linux/hyperv.h | 1 +
> 6 files changed, 74 insertions(+), 66 deletions(-)
>
> --
The patchset seems good to me.
Reviewed-by: Dexuan Cui <[email protected]>
BTW, IMO we need one more patch to remove the queue_work() in
free_channel() -- just make it an ordinary synchronous function call:
vmbus_process_offer() can invoke free_channel() on error path,
and vmbus_process_rescind() can invoke free_channel() too.
We should exclude the possible race.
And now the controlwq and work fields of struct vmbus_channel
are useless now.
Thanks,
-- Dexuan
> -----Original Message-----
> From: Dexuan Cui
> Sent: Sunday, February 15, 2015 7:19 PM
> To: KY Srinivasan; [email protected]; linux-
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: RE: [PATCH 0/6] Drivers: hv: vmbus
>
> > -----Original Message-----
> > From: devel [mailto:[email protected]] On
> > Behalf Of K. Y. Srinivasan
> > Sent: Monday, February 16, 2015 4:11 AM
> > To: [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]
> > Subject: [PATCH 0/6] Drivers: hv: vmbus
> >
> > The host can rescind an offer any time after the offer has been made
> > to the guest. This patch-set cleans up how we handle rescind messages
> > from the host.
> >
> >
> > K. Y. Srinivasan (6):
> > Drivers: hv: vmbus: Properly handle child device remove
> > Drivers: hv: vmbus: Introduce a function to remove a rescinded offer
> > Drivers: hv: vmbus: Handle both rescind and offer messages in the
> > same context
> > Drivers: hv: vmbus: Remove the channel from the channel list(s) on
> > failure
> > Drivers: hv: util: On device remove, close the channel after
> > de-initializing the service
> > Drivers: hv: vmbus: Get rid of some unnecessary messages
> >
> > drivers/hv/channel.c | 9 ++++
> > drivers/hv/channel_mgmt.c | 95 ++++++++++++++++++++-----------------
> -------
> > drivers/hv/connection.c | 7 +---
> > drivers/hv/hv_util.c | 2 +-
> > drivers/hv/vmbus_drv.c | 26 +++++++++---
> > include/linux/hyperv.h | 1 +
> > 6 files changed, 74 insertions(+), 66 deletions(-)
> >
> > --
>
> The patchset seems good to me.
> Reviewed-by: Dexuan Cui <[email protected]>
Dexuan,
Thank you for the review.
>
> BTW, IMO we need one more patch to remove the queue_work() in
> free_channel() -- just make it an ordinary synchronous function call:
>
> vmbus_process_offer() can invoke free_channel() on error path, and
> vmbus_process_rescind() can invoke free_channel() too.
> We should exclude the possible race.
I don't see the race; free_channel is only called after ensuring the channel cannot be discovered
by any other context. Note that we are now executing both rescind and the offer message in the
same work context. With this patch-set, there are only 3 call sites for free_channel:
1. hv_process_channel_removal()
2. vmbus_free_channels()
3. vmbus_process_offer()
If vmbus_process_offer() calls free_channel, the channel cannot be discovered via its ID as we remove
The chanel from the global as well as the per-cpu lists. In this case, the channel is destroyed and nobody
can get a reference to it.
>
> And now the controlwq and work fields of struct vmbus_channel are useless
> now.
Yes; we can get rid of this now. I will have that in a separate patch.
Regards,
K. Y
> -----Original Message-----
> From: KY Srinivasan
> Sent: Monday, February 16, 2015 13:28 PM
> To: Dexuan Cui; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]
> Subject: RE: [PATCH 0/6] Drivers: hv: vmbus
> > -----Original Message-----
> > From: Dexuan Cui
> > Sent: Sunday, February 15, 2015 7:19 PM
> > To: KY Srinivasan; [email protected]; linux-
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]
> > Subject: RE: [PATCH 0/6] Drivers: hv: vmbus
> >
> > > -----Original Message-----
> > > From: devel [mailto:[email protected]] On
> > > Behalf Of K. Y. Srinivasan
> > > Sent: Monday, February 16, 2015 4:11 AM
> > > To: [email protected]; [email protected];
> > > [email protected]; [email protected]; [email protected];
> > > [email protected]
> > > Subject: [PATCH 0/6] Drivers: hv: vmbus
> > >
> > > The host can rescind an offer any time after the offer has been made
> > > to the guest. This patch-set cleans up how we handle rescind messages
> > > from the host.
> > >
> > >
> > > K. Y. Srinivasan (6):
> > > Drivers: hv: vmbus: Properly handle child device remove
> > > Drivers: hv: vmbus: Introduce a function to remove a rescinded offer
> > > Drivers: hv: vmbus: Handle both rescind and offer messages in the
> > > same context
> > > Drivers: hv: vmbus: Remove the channel from the channel list(s) on
> > > failure
> > > Drivers: hv: util: On device remove, close the channel after
> > > de-initializing the service
> > > Drivers: hv: vmbus: Get rid of some unnecessary messages
> > >
> > > drivers/hv/channel.c | 9 ++++
> > > drivers/hv/channel_mgmt.c | 95 ++++++++++++++++++++-----------------
> > -------
> > > drivers/hv/connection.c | 7 +---
> > > drivers/hv/hv_util.c | 2 +-
> > > drivers/hv/vmbus_drv.c | 26 +++++++++---
> > > include/linux/hyperv.h | 1 +
> > > 6 files changed, 74 insertions(+), 66 deletions(-)
> > >
> > > --
> >
> > The patchset seems good to me.
> > Reviewed-by: Dexuan Cui <[email protected]>
>
> Dexuan,
>
> Thank you for the review.
> >
> > BTW, IMO we need one more patch to remove the queue_work() in
> > free_channel() -- just make it an ordinary synchronous function call:
> >
> > vmbus_process_offer() can invoke free_channel() on error path, and
> > vmbus_process_rescind() can invoke free_channel() too.
> > We should exclude the possible race.
>
>
> I don't see the race; free_channel is only called after ensuring the channel
KY, You're correct.
Sorry for my misreading.
> cannot be discovered
> by any other context. Note that we are now executing both rescind and the
> offer message in the
> same work context. With this patch-set, there are only 3 call sites for
> free_channel:
> 1. hv_process_channel_removal()
> 2. vmbus_free_channels()
> 3. vmbus_process_offer()
>
> If vmbus_process_offer() calls free_channel, the channel cannot be discovered
> via its ID as we remove
> The chanel from the global as well as the per-cpu lists. In this case, the channel
> is destroyed and nobody can get a reference to it.
Yeah, I got this now.
-- Dexuan
> >
> > And now the controlwq and work fields of struct vmbus_channel are useless
> > now.
>
> Yes; we can get rid of this now. I will have that in a separate patch.
>
> Regards,
>
> K. Y