Re: [PATCH 08/13] mm: remove the mask variable in hmm_vma_walk_hugetlb_entry

[Ralph Campbell]

On 7/29/19 10:51 PM, Christoph Hellwig wrote:

The pagewalk code already passes the value as the hmask parameter.

Signed-off-by: Christoph Hellwig 
---
  mm/hmm.c | 7 ++-
  1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/mm/hmm.c b/mm/hmm.c
index f26d6abc4ed2..88b77a4a6a1e 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -771,19 +771,16 @@ static int hmm_vma_walk_hugetlb_entry(pte_t *pte, 
unsigned long hmask,
  struct mm_walk *walk)
  {
  #ifdef CONFIG_HUGETLB_PAGE
-   unsigned long addr = start, i, pfn, mask;
+   unsigned long addr = start, i, pfn;
struct hmm_vma_walk *hmm_vma_walk = walk->private;
struct hmm_range *range = hmm_vma_walk->range;
struct vm_area_struct *vma = walk->vma;
-   struct hstate *h = hstate_vma(vma);
uint64_t orig_pfn, cpu_flags;
bool fault, write_fault;
spinlock_t *ptl;
pte_t entry;
int ret = 0;
  
-	mask = huge_page_size(h) - 1;

-
ptl = huge_pte_lock(hstate_vma(vma), walk->mm, pte);
entry = huge_ptep_get(pte);
  
@@ -799,7 +796,7 @@ static int hmm_vma_walk_hugetlb_entry(pte_t *pte, unsigned long hmask,

goto unlock;
}
  
-	pfn = pte_pfn(entry) + ((start & mask) >> PAGE_SHIFT);

+   pfn = pte_pfn(entry) + ((start & hmask) >> PAGE_SHIFT);


This needs to be "~hmask" so that the upper bits of the start address
are not added to the pfn. It's the middle bits of the address that
offset into the huge page that are needed.


for (; addr < end; addr += PAGE_SIZE, i++, pfn++)
range->pfns[i] = hmm_device_entry_from_pfn(range, pfn) |
 cpu_flags;

Re: [PATCH 08/13] mm: remove the mask variable in hmm_vma_walk_hugetlb_entry

[Jason Gunthorpe]

On Tue, Jul 30, 2019 at 08:51:58AM +0300, Christoph Hellwig wrote:
> The pagewalk code already passes the value as the hmask parameter.
> 
> Signed-off-by: Christoph Hellwig 
>  mm/hmm.c | 7 ++-
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/hmm.c b/mm/hmm.c
> index f26d6abc4ed2..88b77a4a6a1e 100644
> +++ b/mm/hmm.c
> @@ -771,19 +771,16 @@ static int hmm_vma_walk_hugetlb_entry(pte_t *pte, 
> unsigned long hmask,
> struct mm_walk *walk)
>  {
>  #ifdef CONFIG_HUGETLB_PAGE
> - unsigned long addr = start, i, pfn, mask;
> + unsigned long addr = start, i, pfn;
>   struct hmm_vma_walk *hmm_vma_walk = walk->private;
>   struct hmm_range *range = hmm_vma_walk->range;
>   struct vm_area_struct *vma = walk->vma;
> - struct hstate *h = hstate_vma(vma);
>   uint64_t orig_pfn, cpu_flags;
>   bool fault, write_fault;
>   spinlock_t *ptl;
>   pte_t entry;
>   int ret = 0;
>  
> - mask = huge_page_size(h) - 1;
> -
>   ptl = huge_pte_lock(hstate_vma(vma), walk->mm, pte);
>   entry = huge_ptep_get(pte);
>  
> @@ -799,7 +796,7 @@ static int hmm_vma_walk_hugetlb_entry(pte_t *pte, 
> unsigned long hmask,
>   goto unlock;
>   }
>  
> - pfn = pte_pfn(entry) + ((start & mask) >> PAGE_SHIFT);
> + pfn = pte_pfn(entry) + ((start & hmask) >> PAGE_SHIFT);

I don't know this hstate stuff, but this doesn't look the same?

static int walk_hugetlb_range(unsigned long addr, unsigned long end, {
struct hstate *h = hstate_vma(vma);
unsigned long hmask = huge_page_mask(h); // aka h->mask

err = walk->hugetlb_entry(pte, hmask, addr, next, walk);

And the first place I found setting h->mask is:

void __init hugetlb_add_hstate(unsigned int order) {
h->mask = ~((1ULL << (order + PAGE_SHIFT)) - 1);

Compared with
mask = huge_page_size(h) - 1;
 = ((unsigned long)PAGE_SIZE << h->order) - 1

Looks like hmask == ~mask

?

Jason