Linux-HyperV Archive on lore.kernel.org
 help / color / Atom feed
From: Sunil Muthuswamy <sunilmut@microsoft.com>
To: Wei Liu <wei.liu@kernel.org>,
	Alexander Duyck <alexander.h.duyck@linux.intel.com>
Cc: KY Srinivasan <kys@microsoft.com>,
	Haiyang Zhang <haiyangz@microsoft.com>,
	Stephen Hemminger <sthemmin@microsoft.com>,
	Wei Liu <liuwe@microsoft.com>,
	Michael Kelley <mikelley@microsoft.com>,
	Tianyu Lan <Tianyu.Lan@microsoft.com>,
	"linux-hyperv@vger.kernel.org" <linux-hyperv@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [EXTERNAL] Re: [PATCH] x86/Hyper-V: Support for free page reporting
Date: Fri, 22 May 2020 16:40:15 +0000
Message-ID: <SN4PR2101MB08804F99992085C64982C821C0B40@SN4PR2101MB0880.namprd21.prod.outlook.com> (raw)
In-Reply-To: <20200520090158.4x4lkbssm7ncirn7@liuwe-devbox-debian-v2.j3c5onc20sse1dnehy4noqpfcg.zx.internal.cloudapp.net>

> > +	if (hv_do_hypercall(HV_EXT_CALL_QUERY_CAPABILITIES, NULL, cap) ==
> > +	    HV_STATUS_SUCCESS)
> 
> You're using the input page as the output parameter. Ideally we should
> introduce hyperv_pcpu_output_arg page, but that would waste one page per
> cpu just for this one call.
> 
> So for now I think this setup is fine, but I would like to add the
> following comment.
> 
>     /*
>      * Repurpose the input_arg page to accept output from Hyper-V for
>      * now because this is the only call that needs output from the
>      * hypervisor. It should be fixed properly by introducing an
>      * output_arg page once we have more places that require output.
>      */

Sounds good. Will add it in v2.

> > +#ifdef CONFIG_PAGE_REPORTING
> > +static int hv_free_page_report(struct page_reporting_dev_info *pr_dev_info,
> > +		    struct scatterlist *sgl, unsigned int nents)
> > +{
> > +	unsigned long flags;
> > +	struct hv_memory_hint *hint;
> > +	int i;
> > +	u64 status;
> > +	struct scatterlist *sg;
> > +
> > +	WARN_ON(nents > HV_MAX_GPA_PAGE_RANGES);
> 
> Should we return -ENOSPC here?

This is more of an assert because PAGE_REPORTING_CAPACITY is set to 32 and has
already been checked that it is < HV_MAX_GPA_PAGE_RANGES in enable_page_reporting.

> > +	hint->type = HV_EXT_MEMORY_HEAT_HINT_TYPE_COLD_DISCARD;
> > +	hint->reserved = 0;
> > +	for (i = 0, sg = sgl; sg; sg = sg_next(sg), i++) {
> > +		int order;
> > +		union hv_gpa_page_range *range;
> > +
> 
> Unfortunately I can't find the semantics of this hypercall in TLFS 6, so
> I have a few questions here.

This structure is not specific to this hypercall.

> 
> > +		order = get_order(sg->length);
> > +		range = &hint->ranges[i];
> > +		range->address_space = 0;
> 
> I guess this means all address spaces?

'address_space' is being used here just as a zero initializer for the union. I think the use
of 'address_space' in the definition of hv_gpa_page_range is not appropriate. This struct is
defined in the TLFS 6.0 with the same name, if you want to look it up.

> 
> > +		range->page.largepage = 1;
> 
> What effect does this have? What if the page is a 4k page?

Page reporting infrastructure doesn't report 4k pages today, but only huge pages (see 
PAGE_REPORTING_MIN_ORDER in page_reporting.h). Additionally, the Hyper-V hypervisor
only supports reporting of 2M pages and above. The current code assumes that the minimum
order will be 9 i.e 2M pages and above.
If we feel that this could change in the future, or an implementation detail that should be
protected against, I can add some checks in hv_balloon.c. But, in that case, the ballon driver
should be able to query the page reporting min order somehow, which it is currently, since it is
private.
Alexander, do you have any suggestions or feedback here?

> 
> > +		range->page.additional_pages = (1ull << (order - 9)) - 1;
> 
> What is 9 here? Is there a macro name *ORDER that you can use?

It is to determine the count of 2M pages. I will define a macro.

> 
> Wei.

  reply index

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-19 18:37 Sunil Muthuswamy
2020-05-20  8:59 ` Vitaly Kuznetsov
2020-05-20  9:05   ` Wei Liu
2020-05-22 17:02   ` [EXTERNAL] " Sunil Muthuswamy
2020-05-20  9:01 ` Wei Liu
2020-05-22 16:40   ` Sunil Muthuswamy [this message]
2020-05-22 20:11     ` [EXTERNAL] " Alexander Duyck
2020-05-23  0:40     ` Michael Kelley

Reply instructions:

You may reply publicly 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=SN4PR2101MB08804F99992085C64982C821C0B40@SN4PR2101MB0880.namprd21.prod.outlook.com \
    --to=sunilmut@microsoft.com \
    --cc=Tianyu.Lan@microsoft.com \
    --cc=alexander.h.duyck@linux.intel.com \
    --cc=haiyangz@microsoft.com \
    --cc=kys@microsoft.com \
    --cc=linux-hyperv@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=liuwe@microsoft.com \
    --cc=mikelley@microsoft.com \
    --cc=sthemmin@microsoft.com \
    --cc=wei.liu@kernel.org \
    /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

Linux-HyperV Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-hyperv/0 linux-hyperv/git/0.git

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

Example config snippet for mirrors

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


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