On Thu, Oct 31, 2019 at 11:43:37AM -0700, John Hubbard wrote:

On 10/31/19 11:35 AM, Ira Weiny wrote:

quoted

On Wed, Oct 30, 2019 at 03:49:13PM -0700, John Hubbard wrote:

...

quoted

quoted

+
+static void __remove_refs_from_head(struct page *page, int refs)
+{
+	/* Do a get_page() first, in case refs == page->_refcount */
+	get_page(page);
+	page_ref_sub(page, refs);
+	put_page(page);
+}

I wonder if this is better implemented as "put_compound_head()"?  To match the
try_get_compound_head() call below?

Hi Ira,

Good idea, I'll rename it to that.

quoted

quoted

+
+static int __huge_pt_done(struct page *head, int nr_recorded_pages, int *nr)
+{
+	*nr += nr_recorded_pages;
+	SetPageReferenced(head);
+	return 1;

When will this return anything but 1?

Never, but it saves a line at all four call sites, by having it return like that.

I could see how maybe people would prefer to just have it be a void function,
and return 1 directly at the call sites. Since this was a lower line count I
thought maybe it would be slightly better, but it's hard to say really.

It is a NIT perhaps but I feel like the signature of a function should stand on
it's own.  What this does is mix the meaning of this function with those
calling it.  Which IMO is not good style.

We can see what others say.

Ira

thanks,

John Hubbard
NVIDIA