On Tue, Dec 05, 2017 at 10:07:09AM -0700, Ross Zwisler wrote:
> > /* The 'colour' (ie low bits) within a PMD of a page offset. */
> > #define PG_PMD_COLOUR ((PMD_SIZE >> PAGE_SHIFT) - 1)
> > +#define PG_PMD_NR (PMD_SIZE >> PAGE_SHIFT)
>
> I wonder if it's confusing that PG_PMD_COLOUR is a mask, but PG_PMD_NR is a
> count? Would "PAGES_PER_PMD" be clearer, in the spirit of
> PTRS_PER_{PGD,PMD,PTE}?
Maybe. I don't think that 'NR' can ever be confused with a mask.
I went with PG_PMD_NR because I didn't want to use HPAGE_PMD_NR, but
in retrospect I just needed to go to sleep and leave thinking about
hard problems like naming things for the morning. I decided to call it
'colour' rather than 'mask' originally because I got really confused with
PMD_MASK masking off the low bits. If you ask 'What colour is this page
within the PMD', you know you're talking about the low bits.
I actually had cause to define PMD_ORDER in a separate unrelated patch
I was working on this morning. How does this set of definitions grab you?
#define PMD_ORDER (PMD_SHIFT - PAGE_SHIFT)
#define PMD_PAGES (1UL << PMD_ORDER)
#define PMD_PAGE_COLOUR (PMD_PAGES - 1)
and maybe put them in linux/mm.h so everybody can see them?
> Also, can we use the same define both in fs/dax.c and in mm/truncate.c,
> instead of the latter using HPAGE_PMD_NR?
I'm OK with the latter using HPAGE_PMD_NR because it's explicitly "is
this a huge page?" But I'd kind of like to get rid of a lot of the HPAGE_*
definitions, so
> > static wait_queue_head_t wait_table[DAX_WAIT_TABLE_ENTRIES];
> >
> > @@ -375,8 +376,8 @@ static void *grab_mapping_entry(struct address_space *mapping, pgoff_t index,
> > * unmapped.
> > */
> > if (pmd_downgrade && dax_is_zero_entry(entry))
> > - unmap_mapping_range(mapping,
> > - (index << PAGE_SHIFT) & PMD_MASK, PMD_SIZE, 0);
> > + unmap_mapping_pages(mapping, index & ~PG_PMD_COLOUR,
> > + PG_PMD_NR, 0);
> >
> > err = radix_tree_preload(
> > mapping_gfp_mask(mapping) & ~__GFP_HIGHMEM);
> > @@ -538,12 +539,10 @@ static void *dax_insert_mapping_entry(struct address_space *mapping,
> > if (dax_is_zero_entry(entry) && !(flags & RADIX_DAX_ZERO_PAGE)) {
> > /* we are replacing a zero page with block mapping */
> > if (dax_is_pmd_entry(entry))
> > - unmap_mapping_range(mapping,
> > - (vmf->pgoff << PAGE_SHIFT) & PMD_MASK,
> > - PMD_SIZE, 0);
> > + unmap_mapping_pages(mapping, vmf->pgoff & PG_PMD_COLOUR,
>
> I think you need: ~PG_PMD_COLOUR,
Heh, yeah, I fixed that in v2 ... which I forgot to cc you on. Sorry.
It's on linux-fsdevel & linux-mm.
