Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
To:     Zi Yan <ziy@nvidia.com>, linux-mm@kvack.org
Cc:     Matthew Wilcox <willy@infradead.org>,
        Vlastimil Babka <vbabka@suse.cz>,
        "Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>,
        Mike Kravetz <mike.kravetz@oracle.com>,
        Michal Hocko <mhocko@kernel.org>,
        John Hubbard <jhubbard@nvidia.com>,
        linux-kernel@vger.kernel.org,
        "Michael S. Tsirkin" <mst@redhat.com>,
        Jason Wang <jasowang@redhat.com>,
        virtualization@lists.linux-foundation.org
References: <20210805190253.2795604-1-zi.yan@sent.com>
 <20210805190253.2795604-10-zi.yan@sent.com>
From:   David Hildenbrand <david@redhat.com>
Organization: Red Hat
Subject: Re: [RFC PATCH 09/15] virtio: virtio_mem: use PAGES_PER_SECTION
 instead of MAX_ORDER_NR_PAGES
Message-ID: <3e2f1910-e7d9-ddf9-063b-d702793f1525@redhat.com>
Date:   Mon, 9 Aug 2021 09:35:12 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <20210805190253.2795604-10-zi.yan@sent.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 05.08.21 21:02, Zi Yan wrote:
> From: Zi Yan <ziy@nvidia.com>
> 
> It keeps the existing behavior when MAX_ORDER grows beyond a section
> size.
> 
> Signed-off-by: Zi Yan <ziy@nvidia.com>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: virtualization@lists.linux-foundation.org
> Cc: linux-mm@kvack.org
> Cc: linux-kernel@vger.kernel.org
> ---
>   drivers/virtio/virtio_mem.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
> index 19036922f7ef..bab5a81fa796 100644
> --- a/drivers/virtio/virtio_mem.c
> +++ b/drivers/virtio/virtio_mem.c
> @@ -1105,11 +1105,11 @@ static void virtio_mem_clear_fake_offline(unsigned long pfn,
>    */
>   static void virtio_mem_fake_online(unsigned long pfn, unsigned long nr_pages)
>   {
> -	const unsigned long max_nr_pages = MAX_ORDER_NR_PAGES;
> +	const unsigned long max_nr_pages = PAGES_PER_SECTION;
>   	unsigned long i;
>   
>   	/*
> -	 * We are always called at least with MAX_ORDER_NR_PAGES
> +	 * We are always called at least with PAGES_PER_SECTION
>   	 * granularity/alignment (e.g., the way subblocks work). All pages
>   	 * inside such a block are alike.
>   	 */
> @@ -1125,7 +1125,7 @@ static void virtio_mem_fake_online(unsigned long pfn, unsigned long nr_pages)
>   		if (PageDirty(page)) {
>   			virtio_mem_clear_fake_offline(pfn + i, max_nr_pages,
>   						      false);
> -			generic_online_page(page, MAX_ORDER - 1);
> +			generic_online_page(page, PAGES_PER_SECTION - 1);
>   		} else {
>   			virtio_mem_clear_fake_offline(pfn + i, max_nr_pages,
>   						      true);
> @@ -1228,7 +1228,7 @@ static void virtio_mem_online_page_cb(struct page *page, unsigned int order)
>   		if (vm->in_sbm) {
>   			/*
>   			 * We exploit here that subblocks have at least
> -			 * MAX_ORDER_NR_PAGES size/alignment - so we cannot
> +			 * PAGES_PER_SECTION size/alignment - so we cannot
>   			 * cross subblocks within one call.
>   			 */
>   			id = virtio_mem_phys_to_mb_id(addr);
> @@ -2438,14 +2438,14 @@ static int virtio_mem_init(struct virtio_mem *vm)
>   				      VIRTIO_MEM_DEFAULT_OFFLINE_THRESHOLD);
>   
>   	/*
> -	 * We want subblocks to span at least MAX_ORDER_NR_PAGES and
> +	 * We want subblocks to span at least PAGES_PER_SECTION and
>   	 * pageblock_nr_pages pages. This:
>   	 * - Simplifies our page onlining code (virtio_mem_online_page_cb)
>   	 *   and fake page onlining code (virtio_mem_fake_online).
>   	 * - Is required for now for alloc_contig_range() to work reliably -
>   	 *   it doesn't properly handle smaller granularity on ZONE_NORMAL.
>   	 */
> -	sb_size = max_t(uint64_t, MAX_ORDER_NR_PAGES,
> +	sb_size = max_t(uint64_t, PAGES_PER_SECTION,
>   			pageblock_nr_pages) * PAGE_SIZE;
>   	sb_size = max_t(uint64_t, vm->device_block_size, sb_size);
>   
> 

This is very much completely broken and destroys most of the purpose of 
virtio-mem. It even is broken once MAX_ORDER would exceed a single 
memory section I think.

Whatever you do, keep virtio-mem working *as is* unless someone 
explicitly sets MAX_ORDER on the command line to something bigger.


virtio-mem will require some minor adjustments once MAX_ORDER_NR_PAGES 
would exceed the memory section size -- the functionality will, however, 
be heavily degraded once you increase MAX_ORDER_NR_PAGES in any way 
(again, which is fine if it's explicitly done by an admin on the command 
line).

As mentioned somewhere else already, we'll have to tackle 
alloc_contig_range() to properly deal with pageblock_order granularity, 
then we can rework virtio-mem code to be based on that instead of 
MAX_ORDER - 1.

-- 
Thanks,

David / dhildenb