On 11/13/19 3:15 AM, Jan Kara wrote:
On Tue 12-11-19 20:26:49, John Hubbard wrote:
There are four locations in gup.c that have a fair amount of code
duplication. This means that changing one requires making the same
changes in four places, not to mention reading the same code four
times, and wondering if there are subtle differences.
Factor out the common code into static functions, thus reducing the
overall line count and the code's complexity.
Also, take the opportunity to slightly improve the efficiency of the
error cases, by doing a mass subtraction of the refcount, surrounded
by get_page()/put_page().
Also, further simplify (slightly), by waiting until the the successful
end of each routine, to increment *nr.
Reviewed-by: Jérôme Glisse <jglisse@xxxxxxxxxx>
Cc: Ira Weiny <ira.weiny@xxxxxxxxx>
Cc: Christoph Hellwig <hch@xxxxxx>
Cc: Aneesh Kumar K.V <aneesh.kumar@xxxxxxxxxxxxx>
Signed-off-by: John Hubbard <jhubbard@xxxxxxxxxx>
diff --git a/mm/gup.c b/mm/gup.c
index 85caf76b3012..199da99e8ffc 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1969,6 +1969,34 @@ static int __gup_device_huge_pud(pud_t pud, pud_t *pudp, unsigned long addr,
}
#endif
+static int __record_subpages(struct page *page, unsigned long addr,
+ unsigned long end, struct page **pages, int nr)
+{
+ int nr_recorded_pages = 0;
+
+ do {
+ pages[nr] = page;
+ nr++;
+ page++;
+ nr_recorded_pages++;
+ } while (addr += PAGE_SIZE, addr != end);
+ return nr_recorded_pages;
+}
Why don't you pass in already pages + nr?
Aha, that does save a function argument. Will do.
...
+static void __huge_pt_done(struct page *head, int nr_recorded_pages, int *nr)
+{
+ *nr += nr_recorded_pages;
+ SetPageReferenced(head);
+}
I don't find this last helper very useful. It seems to muddy water more
than necessary...
Yes, I suspect it's rather unloved, and the fact that it was hard to accurately
name should have been a big hint to not do it. I'll remove the helper and
put the lines back in directly.
thanks,
--
John Hubbard
NVIDIA
Other than that the cleanup looks nice to me.
Honza
