On 8/27/21 4:33 PM, Christoph Hellwig wrote:
On Fri, Aug 27, 2021 at 03:58:09PM +0100, Joao Martins wrote:
quoted
+ * @geometry: structural definition of how the vmemmap metadata is populated.
+ * A zero or 1 defaults to using base pages as the memmap metadata
+ * representation. A bigger value will set up compound struct pages
+ * representative of the requested geometry size.
* @ops: method table
* @owner: an opaque pointer identifying the entity that manages this
* instance. Used by various helpers to make sure that no
@@ -114,6 +118,7 @@ struct dev_pagemap {
struct completion done;
enum memory_type type;
unsigned int flags;
+ unsigned long geometry;
So why not make this a shift as I suggested somewhere deep in the
last thread?
So the previous you suggested had the check for pgmap_geometry() > PAGE_SIZE,
but because pgmap_geometry() return 1 by default, then the pfn_end() - pfn
computation wouldn't change for those that don't request a geometry.
So rather than this that you initially suggested:
refs = pfn_end(pgmap, range_id) - pfn_first(pgmap, range_id);
if (pgmap_geometry(pgmap) > 1)
refs /= pgmap_geometry(pgmap);
I would turn into this, because now for users which don't select geometry it's always a
division by 1:
refs = pfn_end(pgmap, range_id) - pfn_first(pgmap, range_id);
refs /= pgmap_geometry(pgmap);
So felt like doing it inline straight away inline when calling percpu_ref_get_many():
(pfn_end(pgmap, range_id) - pfn_first(pgmap, range_id)) / pgmap_geometry(pgmap);
I can switch to a shift if you prefer:
(pfn_end(pgmap, range_id) - pfn_first(pgmap, range_id))
<< pgmap_geometry_order(pgmap);
Also geometry sounds a bit strange, even if I can't really
offer anything better offhand.
We started with @align (like in device dax core), and then we switched
to @geometry because these are slightly different things (one relates
to vmemmap metadata structure (number of pages) and the other is how
the mmap is aligned to a page size. I couldn't suggest anything else,
besides a more verbose name like vmemmap_align maybe.
quoted
+static inline unsigned long pgmap_geometry(struct dev_pagemap *pgmap)
+{
+ if (pgmap && pgmap->geometry)
+ return pgmap->geometry;
Why does this need to support a NULL pgmap?
This was mainly a defensive choice, similar to put_dev_pagemap(). There's
only one caller, which is populate_section_memmap with an altmap but without
pgmap.
quoted
+static void __ref memmap_init_compound(struct page *head, unsigned long head_pfn,
Overly long line.
Fixed.
Interesting that checkpatch doesn't complain with one character past 80 lines.