LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Michael Kelley <mikelley@microsoft.com>
To: Dexuan Cui <decui@microsoft.com>,
	"linux-hyperv@vger.kernel.org" <linux-hyperv@vger.kernel.org>,
	"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
	Stephen Hemminger <sthemmin@microsoft.com>,
	Sasha Levin <Alexander.Levin@microsoft.com>,
	Haiyang Zhang <haiyangz@microsoft.com>,
	KY Srinivasan <kys@microsoft.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Tianyu Lan <Tianyu.Lan@microsoft.com>
Cc: "olaf@aepfle.de" <olaf@aepfle.de>,
	"apw@canonical.com" <apw@canonical.com>,
	"jasowang@redhat.com" <jasowang@redhat.com>,
	vkuznets <vkuznets@redhat.com>,
	"marcelo.cerri@canonical.com" <marcelo.cerri@canonical.com>
Subject: RE: [PATCH 2/2] hv_balloon: Reorganize the probe function
Date: Fri, 14 Jun 2019 21:56:24 +0000
Message-ID: <BL0PR2101MB13487B8D2A157AA7FCFD159DD7EE0@BL0PR2101MB1348.namprd21.prod.outlook.com> (raw)
In-Reply-To: <1560537692-37400-2-git-send-email-decui@microsoft.com>

From: Dexuan Cui <decui@microsoft.com>  Sent: Friday, June 14, 2019 11:43 AM
> 
> Move the code that negotiates with the host to a new function
> balloon_connect_vsp() and improve the error handling.
> 
> This makes the code more readable and paves the way for the
> support of hibernation in future.
> 
> Makes no real logic change here.
> 
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
>  drivers/hv/hv_balloon.c | 124 +++++++++++++++++++++-------------------
>  1 file changed, 66 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
> index 13381ea3e3e7..111ea3599659 100644
> --- a/drivers/hv/hv_balloon.c
> +++ b/drivers/hv/hv_balloon.c
> @@ -1574,50 +1574,18 @@ static void balloon_onchannelcallback(void *context)
> 
>  }
> 
> -static int balloon_probe(struct hv_device *dev,
> -			const struct hv_vmbus_device_id *dev_id)
> +static int balloon_connect_vsp(struct hv_device *dev)
>  {
> -	int ret;
> -	unsigned long t;
>  	struct dm_version_request version_req;
>  	struct dm_capabilities cap_msg;
> -
> -#ifdef CONFIG_MEMORY_HOTPLUG
> -	do_hot_add = hot_add;
> -#else
> -	do_hot_add = false;
> -#endif
> +	unsigned long t;
> +	int ret;
> 
>  	ret = vmbus_open(dev->channel, dm_ring_size, dm_ring_size, NULL, 0,
> -			balloon_onchannelcallback, dev);
> -
> +			 balloon_onchannelcallback, dev);
>  	if (ret)
>  		return ret;
> 
> -	dm_device.dev = dev;
> -	dm_device.state = DM_INITIALIZING;
> -	dm_device.next_version = DYNMEM_PROTOCOL_VERSION_WIN8;
> -	init_completion(&dm_device.host_event);
> -	init_completion(&dm_device.config_event);
> -	INIT_LIST_HEAD(&dm_device.ha_region_list);
> -	spin_lock_init(&dm_device.ha_lock);
> -	INIT_WORK(&dm_device.balloon_wrk.wrk, balloon_up);
> -	INIT_WORK(&dm_device.ha_wrk.wrk, hot_add_req);
> -	dm_device.host_specified_ha_region = false;
> -
> -	dm_device.thread =
> -		 kthread_run(dm_thread_func, &dm_device, "hv_balloon");
> -	if (IS_ERR(dm_device.thread)) {
> -		ret = PTR_ERR(dm_device.thread);
> -		goto probe_error1;
> -	}
> -
> -#ifdef CONFIG_MEMORY_HOTPLUG
> -	set_online_page_callback(&hv_online_page);
> -	register_memory_notifier(&hv_memory_nb);
> -#endif
> -
> -	hv_set_drvdata(dev, &dm_device);
>  	/*
>  	 * Initiate the hand shake with the host and negotiate
>  	 * a version that the host can support. We start with the
> @@ -1633,16 +1601,15 @@ static int balloon_probe(struct hv_device *dev,
>  	dm_device.version = version_req.version.version;
> 
>  	ret = vmbus_sendpacket(dev->channel, &version_req,
> -				sizeof(struct dm_version_request),
> -				(unsigned long)NULL,
> -				VM_PKT_DATA_INBAND, 0);
> +			       sizeof(struct dm_version_request),
> +			       (unsigned long)NULL, VM_PKT_DATA_INBAND, 0);
>  	if (ret)
> -		goto probe_error2;
> +		goto out;
> 
>  	t = wait_for_completion_timeout(&dm_device.host_event, 5*HZ);
>  	if (t == 0) {
>  		ret = -ETIMEDOUT;
> -		goto probe_error2;
> +		goto out;
>  	}
> 
>  	/*
> @@ -1650,8 +1617,8 @@ static int balloon_probe(struct hv_device *dev,
>  	 * fail the probe function.
>  	 */
>  	if (dm_device.state == DM_INIT_ERROR) {
> -		ret = -ETIMEDOUT;
> -		goto probe_error2;
> +		ret = -EPROTO;
> +		goto out;
>  	}
> 
>  	pr_info("Using Dynamic Memory protocol version %u.%u\n",
> @@ -1684,16 +1651,15 @@ static int balloon_probe(struct hv_device *dev,
>  	cap_msg.max_page_number = -1;
> 
>  	ret = vmbus_sendpacket(dev->channel, &cap_msg,
> -				sizeof(struct dm_capabilities),
> -				(unsigned long)NULL,
> -				VM_PKT_DATA_INBAND, 0);
> +			       sizeof(struct dm_capabilities),
> +			       (unsigned long)NULL, VM_PKT_DATA_INBAND, 0);
>  	if (ret)
> -		goto probe_error2;
> +		goto out;
> 
>  	t = wait_for_completion_timeout(&dm_device.host_event, 5*HZ);
>  	if (t == 0) {
>  		ret = -ETIMEDOUT;
> -		goto probe_error2;
> +		goto out;
>  	}
> 
>  	/*
> @@ -1701,23 +1667,65 @@ static int balloon_probe(struct hv_device *dev,
>  	 * fail the probe function.
>  	 */
>  	if (dm_device.state == DM_INIT_ERROR) {
> -		ret = -ETIMEDOUT;
> -		goto probe_error2;
> +		ret = -EPROTO;
> +		goto out;
>  	}
> 
> +	return 0;
> +out:
> +	vmbus_close(dev->channel);
> +	return ret;
> +}
> +
> +static int balloon_probe(struct hv_device *dev,
> +			 const struct hv_vmbus_device_id *dev_id)
> +{
> +	int ret;
> +
> +#ifdef CONFIG_MEMORY_HOTPLUG
> +	do_hot_add = hot_add;
> +#else
> +	do_hot_add = false;
> +#endif
> +	dm_device.dev = dev;
> +	dm_device.state = DM_INITIALIZING;
> +	dm_device.next_version = DYNMEM_PROTOCOL_VERSION_WIN8;
> +	init_completion(&dm_device.host_event);
> +	init_completion(&dm_device.config_event);
> +	INIT_LIST_HEAD(&dm_device.ha_region_list);
> +	spin_lock_init(&dm_device.ha_lock);
> +	INIT_WORK(&dm_device.balloon_wrk.wrk, balloon_up);
> +	INIT_WORK(&dm_device.ha_wrk.wrk, hot_add_req);
> +	dm_device.host_specified_ha_region = false;
> +
> +#ifdef CONFIG_MEMORY_HOTPLUG
> +	set_online_page_callback(&hv_online_page);
> +	register_memory_notifier(&hv_memory_nb);
> +#endif
> +
> +	hv_set_drvdata(dev, &dm_device);
> +
> +	ret = balloon_connect_vsp(dev);
> +	if (ret != 0)
> +		return ret;
> +
>  	dm_device.state = DM_INITIALIZED;
> -	last_post_time = jiffies;

I was curious about the above deletion.  But I guess the line
is not needed as the time_after() check in post_status() should
handle an initial value of 0 for last_post_time just fine.

> +
> +	dm_device.thread =
> +		 kthread_run(dm_thread_func, &dm_device, "hv_balloon");
> +	if (IS_ERR(dm_device.thread)) {
> +		ret = PTR_ERR(dm_device.thread);
> +		goto probe_error;
> +	}

Just an observation:  this thread creation now happens at the end of the
probing process.  But that's good, because in the old code, the thread
was started and could run before the protocol version had been
negotiated.  So I'll assume your change here is intentional.

> 
>  	return 0;
> 
> -probe_error2:
> +probe_error:
> +	vmbus_close(dev->channel);
>  #ifdef CONFIG_MEMORY_HOTPLUG
> +	unregister_memory_notifier(&hv_memory_nb);

Hmmm. Evidently the above cleanup was missing in the
old code.

>  	restore_online_page_callback(&hv_online_page);
>  #endif
> -	kthread_stop(dm_device.thread);
> -
> -probe_error1:
> -	vmbus_close(dev->channel);
>  	return ret;
>  }
> 
> @@ -1734,11 +1742,11 @@ static int balloon_remove(struct hv_device *dev)
>  	cancel_work_sync(&dm->balloon_wrk.wrk);
>  	cancel_work_sync(&dm->ha_wrk.wrk);
> 
> -	vmbus_close(dev->channel);
>  	kthread_stop(dm->thread);
> +	vmbus_close(dev->channel);

Presumably this is an intentional ordering change as well.
The kthread should be stopped before closing the channel.

>  #ifdef CONFIG_MEMORY_HOTPLUG
> -	restore_online_page_callback(&hv_online_page);
>  	unregister_memory_notifier(&hv_memory_nb);
> +	restore_online_page_callback(&hv_online_page);

And you've changed the ordering of these steps so they are
the inverse of when they are set up.  Also a good cleanup ....

>  #endif
>  	spin_lock_irqsave(&dm_device.ha_lock, flags);
>  	list_for_each_entry_safe(has, tmp, &dm->ha_region_list, list) {
> --
> 2.19.1

Reviewed-by: Michael Kelley <mikelley@microsoft.com>

  reply index

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-14 18:42 [PATCH 1/2] hv_balloon: Use a static page for the balloon_up send buffer Dexuan Cui
2019-06-14 18:42 ` [PATCH 2/2] hv_balloon: Reorganize the probe function Dexuan Cui
2019-06-14 21:56   ` Michael Kelley [this message]
2019-06-14 23:08     ` Dexuan Cui
2019-06-14 20:56 ` [PATCH 1/2] hv_balloon: Use a static page for the balloon_up send buffer Michael Kelley

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=BL0PR2101MB13487B8D2A157AA7FCFD159DD7EE0@BL0PR2101MB1348.namprd21.prod.outlook.com \
    --to=mikelley@microsoft.com \
    --cc=Alexander.Levin@microsoft.com \
    --cc=Tianyu.Lan@microsoft.com \
    --cc=apw@canonical.com \
    --cc=decui@microsoft.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=haiyangz@microsoft.com \
    --cc=jasowang@redhat.com \
    --cc=kys@microsoft.com \
    --cc=linux-hyperv@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=marcelo.cerri@canonical.com \
    --cc=olaf@aepfle.de \
    --cc=sthemmin@microsoft.com \
    --cc=vkuznets@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox