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;
Subject: Re: [PATCH 1/2] xen: add helpers for caching grant mapping pages
To:     Juergen Gross <jgross@suse.com>, xen-devel@lists.xenproject.org,
        linux-block@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-scsi@vger.kernel.org
Cc:     Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
        =?UTF-8?Q?Roger_Pau_Monn=c3=a9?= <roger.pau@citrix.com>,
        Jens Axboe <axboe@kernel.dk>,
        Stefano Stabellini <sstabellini@kernel.org>
References: <20201207133024.16621-1-jgross@suse.com>
 <20201207133024.16621-2-jgross@suse.com>
From:   boris.ostrovsky@oracle.com
Organization: Oracle Corporation
Message-ID: <67875212-a5e1-5ea3-6ecd-2cf878f3cd5d@oracle.com>
Date:   Mon, 7 Dec 2020 18:52:24 -0500
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0)
 Gecko/20100101 Thunderbird/78.5.1
MIME-Version: 1.0
In-Reply-To: <20201207133024.16621-2-jgross@suse.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Precedence: bulk


On 12/7/20 8:30 AM, Juergen Gross wrote:
> Instead of having similar helpers in multiple backend drivers use
> common helpers for caching pages allocated via gnttab_alloc_pages().
>
> Make use of those helpers in blkback and scsiback.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>


Reviewed-by: Boris Ostrovsky <boris.ostrovksy@oracle.com>


> +
> +void gnttab_page_cache_shrink(struct gnttab_page_cache *cache, unsigned int num)
> +{
> +	struct page *page[10];
> +	unsigned int i = 0;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&cache->lock, flags);
> +
> +	while (cache->num_pages > num) {
> +		page[i] = list_first_entry(&cache->pages, struct page, lru);
> +		list_del(&page[i]->lru);
> +		cache->num_pages--;
> +		if (++i == ARRAY_SIZE(page)) {
> +			spin_unlock_irqrestore(&cache->lock, flags);
> +			gnttab_free_pages(i, page);
> +			i = 0;
> +			spin_lock_irqsave(&cache->lock, flags);
> +		}
> +	}
> +
> +	spin_unlock_irqrestore(&cache->lock, flags);
> +
> +	if (i != 0)
> +		gnttab_free_pages(i, page);
> +}


How about splitting cache->pages list into two lists (one @num long and the other holding the rest) and then batching gntab_free_pages() from that first list? Then you won't have to deal with locking.


In fact, I am not even sure batching gains us much, I'd consider going one-by-one.


-boris