LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Dexuan Cui <decui@microsoft.com>
To: Michael Kelley <mikelley@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 23:08:39 +0000
Message-ID: <PU1P153MB01699145D8BDE54ABAFF7EF5BFEE0@PU1P153MB0169.APCP153.PROD.OUTLOOK.COM> (raw)
In-Reply-To: <BL0PR2101MB13487B8D2A157AA7FCFD159DD7EE0@BL0PR2101MB1348.namprd21.prod.outlook.com>

> From: Michael Kelley <mikelley@microsoft.com>
> Sent: Friday, June 14, 2019 2:56 PM
> > ...
> > +	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.

In a 32-bit kernel, sizeof(unsigned long) is 4, and the global 32-bit
varilable "jiffies" can overflow in 49.7 days if HZ is defined as 1000;
so in theory there is a tiny chance time_after() can not work as
expected here (i.e. we're loading hv_balloon driver when the
"jiffies" is just about to overflow, which is highly unlikely in practice);
even if that happens, we do not care, since the consequence is
just that the memory pressure reporting is delayed by 1 second. :-)

> > +
> > +	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.

Yes, this 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.

Yes.
 
> >  	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.

Yes. The old code is buggy: after the vmbus_close(), there is
a small window in which the old code can still try to send
messages to the host via a freed ringbuffer, causing panic.
 
> >  #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 ....

Yes. The change is not really necessary, but let's just do it
in a better manner.
 
> 
> Reviewed-by: Michael Kelley <mikelley@microsoft.com>

Thaks for the detailed comments!

Thanks,
-- Dexuan

  reply index

Thread overview: 6+ 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
2019-06-14 23:08     ` Dexuan Cui [this message]
2019-06-14 20:56 ` [PATCH 1/2] hv_balloon: Use a static page for the balloon_up send buffer Michael Kelley
2019-07-30 22:36 ` Sasha Levin

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=PU1P153MB01699145D8BDE54ABAFF7EF5BFEE0@PU1P153MB0169.APCP153.PROD.OUTLOOK.COM \
    --to=decui@microsoft.com \
    --cc=Alexander.Levin@microsoft.com \
    --cc=Tianyu.Lan@microsoft.com \
    --cc=apw@canonical.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=mikelley@microsoft.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