On Tue, Dec 05, 2017 at 09:37:13AM -0800, Matthew Wilcox wrote:
> 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?
Yep, I personally like these better, and putting them in a global header seems
like the right way to go.
> > 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
I would also like to get rid of them if possible, but quick grep makes me
think that unfortunately they may not be entirely equivalent to other defines
we have?
i.e:
arch/metag/include/asm/page.h:# define HPAGE_SHIFT 13
arch/metag/include/asm/page.h:# define HPAGE_SHIFT 14
arch/metag/include/asm/page.h:# define HPAGE_SHIFT 15
arch/metag/include/asm/page.h:# define HPAGE_SHIFT 16
arch/metag/include/asm/page.h:# define HPAGE_SHIFT 17
arch/metag/include/asm/page.h:# define HPAGE_SHIFT 18
arch/metag/include/asm/page.h:# define HPAGE_SHIFT 19
arch/metag/include/asm/page.h:# define HPAGE_SHIFT 20
arch/metag/include/asm/page.h:# define HPAGE_SHIFT 21
arch/metag/include/asm/page.h:# define HPAGE_SHIFT 22
this arch has no PMD_SHIFT definition...
I'm not really familiar with the HPAGE defines, though, so maybe it's not as
complex as it seems.
