| Message ID | 20240408194232.118537-4-willy@infradead.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Some cleanups for memory-failure | expand |
On 4/8/2024 12:42 PM, Matthew Wilcox (Oracle) wrote: > The only user of this function calls page_address_in_vma() immediately > after page_mapped_in_vma() calculates it and uses it to return true/false. > Return the address instead, allowing memory-failure to skip the call > to page_address_in_vma(). > > Acked-by: Miaohe Lin <linmiaohe@huawei.com> > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > --- > mm/page_vma_mapped.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/mm/page_vma_mapped.c b/mm/page_vma_mapped.c > index 53b8868ede61..48bfc17934cd 100644 > --- a/mm/page_vma_mapped.c > +++ b/mm/page_vma_mapped.c > @@ -319,9 +319,10 @@ bool page_vma_mapped_walk(struct page_vma_mapped_walk *pvmw) > * @page: the page to test > * @vma: the VMA to test > * > - * Returns 1 if the page is mapped into the page tables of the VMA, 0 > - * if the page is not mapped into the page tables of this VMA. Only > - * valid for normal file or anonymous VMAs. > + * Return: The address the page is mapped at if the page is in the range > + * covered by the VMA and present in the page table. If the page is > + * outside the VMA or not present, returns -EFAULT. > + * Only valid for normal file or anonymous VMAs. > */ > int page_mapped_in_vma(struct page *page, struct vm_area_struct *vma) > { > @@ -336,9 +337,10 @@ int page_mapped_in_vma(struct page *page, struct vm_area_struct *vma) > > pvmw.address = vma_address(vma, pgoff, 1); > if (pvmw.address == -EFAULT) > - return 0; > + goto out; > if (!page_vma_mapped_walk(&pvmw)) > - return 0; > + return -EFAULT; > page_vma_mapped_walk_done(&pvmw); > - return 1; > +out: > + return pvmw.address; > } Looks good. Reviewed-by: Jane Chu <jane.chu@oracle.com> -jane
On Mon, Apr 08, 2024 at 08:42:21PM +0100, Matthew Wilcox (Oracle) wrote: > The only user of this function calls page_address_in_vma() immediately > after page_mapped_in_vma() calculates it and uses it to return true/false. > Return the address instead, allowing memory-failure to skip the call > to page_address_in_vma(). > > Acked-by: Miaohe Lin <linmiaohe@huawei.com> > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > --- > mm/page_vma_mapped.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/mm/page_vma_mapped.c b/mm/page_vma_mapped.c > index 53b8868ede61..48bfc17934cd 100644 > --- a/mm/page_vma_mapped.c > +++ b/mm/page_vma_mapped.c > @@ -319,9 +319,10 @@ bool page_vma_mapped_walk(struct page_vma_mapped_walk *pvmw) > * @page: the page to test > * @vma: the VMA to test > * > - * Returns 1 if the page is mapped into the page tables of the VMA, 0 > - * if the page is not mapped into the page tables of this VMA. Only > - * valid for normal file or anonymous VMAs. > + * Return: The address the page is mapped at if the page is in the range > + * covered by the VMA and present in the page table. If the page is > + * outside the VMA or not present, returns -EFAULT. > + * Only valid for normal file or anonymous VMAs. I am probably missing something here but I am confused. Now we either return -EFAULT or the address. But page_vma_mapped_walk() gets called from collect_procs_anon() like this: if (!page_mapped_in_vma(page, vma)) so that is not gonna work the way we want? Should not that be converted to if (page_mapped_in_vma(page, vma) == -EFAULT) ?
On 2024/4/10 17:38, Oscar Salvador wrote: > On Mon, Apr 08, 2024 at 08:42:21PM +0100, Matthew Wilcox (Oracle) wrote: >> The only user of this function calls page_address_in_vma() immediately >> after page_mapped_in_vma() calculates it and uses it to return true/false. >> Return the address instead, allowing memory-failure to skip the call >> to page_address_in_vma(). >> >> Acked-by: Miaohe Lin <linmiaohe@huawei.com> >> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> >> --- >> mm/page_vma_mapped.c | 14 ++++++++------ >> 1 file changed, 8 insertions(+), 6 deletions(-) >> >> diff --git a/mm/page_vma_mapped.c b/mm/page_vma_mapped.c >> index 53b8868ede61..48bfc17934cd 100644 >> --- a/mm/page_vma_mapped.c >> +++ b/mm/page_vma_mapped.c >> @@ -319,9 +319,10 @@ bool page_vma_mapped_walk(struct page_vma_mapped_walk *pvmw) >> * @page: the page to test >> * @vma: the VMA to test >> * >> - * Returns 1 if the page is mapped into the page tables of the VMA, 0 >> - * if the page is not mapped into the page tables of this VMA. Only >> - * valid for normal file or anonymous VMAs. >> + * Return: The address the page is mapped at if the page is in the range >> + * covered by the VMA and present in the page table. If the page is >> + * outside the VMA or not present, returns -EFAULT. >> + * Only valid for normal file or anonymous VMAs. > > I am probably missing something here but I am confused. > Now we either return -EFAULT or the address. > > But page_vma_mapped_walk() gets called from collect_procs_anon() like > this: > > if (!page_mapped_in_vma(page, vma)) > > so that is not gonna work the way we want? > Should not that be converted to > > if (page_mapped_in_vma(page, vma) == -EFAULT) > > ? It seems this patch is incomplete. It forgets to change the caller parts as it once did at [1]. Or am I miss something too? [1] https://www.spinics.net/lists/linux-mm/msg375668.html Thanks. . >
On Wed, Apr 10, 2024 at 11:38:36AM +0200, Oscar Salvador wrote: > On Mon, Apr 08, 2024 at 08:42:21PM +0100, Matthew Wilcox (Oracle) wrote: > > The only user of this function calls page_address_in_vma() immediately > > after page_mapped_in_vma() calculates it and uses it to return true/false. > > Return the address instead, allowing memory-failure to skip the call > > to page_address_in_vma(). > > > > Acked-by: Miaohe Lin <linmiaohe@huawei.com> > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > > --- > > mm/page_vma_mapped.c | 14 ++++++++------ > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > diff --git a/mm/page_vma_mapped.c b/mm/page_vma_mapped.c > > index 53b8868ede61..48bfc17934cd 100644 > > --- a/mm/page_vma_mapped.c > > +++ b/mm/page_vma_mapped.c > > @@ -319,9 +319,10 @@ bool page_vma_mapped_walk(struct page_vma_mapped_walk *pvmw) > > * @page: the page to test > > * @vma: the VMA to test > > * > > - * Returns 1 if the page is mapped into the page tables of the VMA, 0 > > - * if the page is not mapped into the page tables of this VMA. Only > > - * valid for normal file or anonymous VMAs. > > + * Return: The address the page is mapped at if the page is in the range > > + * covered by the VMA and present in the page table. If the page is > > + * outside the VMA or not present, returns -EFAULT. > > + * Only valid for normal file or anonymous VMAs. > > I am probably missing something here but I am confused. > Now we either return -EFAULT or the address. I don't know where the other hunks from this patch went. Going back through git reflog, they're missing since the very first version applied to the tree they're currently part of (using git-am, well actually b4, I think). I'll resurrect them from the original posting and resend the whole patchset.
diff --git a/mm/page_vma_mapped.c b/mm/page_vma_mapped.c index 53b8868ede61..48bfc17934cd 100644 --- a/mm/page_vma_mapped.c +++ b/mm/page_vma_mapped.c @@ -319,9 +319,10 @@ bool page_vma_mapped_walk(struct page_vma_mapped_walk *pvmw) * @page: the page to test * @vma: the VMA to test * - * Returns 1 if the page is mapped into the page tables of the VMA, 0 - * if the page is not mapped into the page tables of this VMA. Only - * valid for normal file or anonymous VMAs. + * Return: The address the page is mapped at if the page is in the range + * covered by the VMA and present in the page table. If the page is + * outside the VMA or not present, returns -EFAULT. + * Only valid for normal file or anonymous VMAs. */ int page_mapped_in_vma(struct page *page, struct vm_area_struct *vma) { @@ -336,9 +337,10 @@ int page_mapped_in_vma(struct page *page, struct vm_area_struct *vma) pvmw.address = vma_address(vma, pgoff, 1); if (pvmw.address == -EFAULT) - return 0; + goto out; if (!page_vma_mapped_walk(&pvmw)) - return 0; + return -EFAULT; page_vma_mapped_walk_done(&pvmw); - return 1; +out: + return pvmw.address; }