| Message ID | 20250418095600.721989-2-ye.liu@linux.dev (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: minor cleanups in rmap | expand |
On Fri, Apr 18, 2025 at 05:55:59PM +0800, Ye Liu wrote: > From: Ye Liu <liuye@kylinos.cn> > > Renamed local variable page__anon_vma in page_address_in_vma() to > page_anon_vma. The previous naming convention of using double underscores > (__) is unnecessary and inconsistent with typical kernel style, which uses > single underscores to denote local variables. Also updated comments to > reflect the new variable name. > > Functionality unchanged. > > Signed-off-by: Ye Liu <liuye@kylinos.cn> For future reference, if you don't change the patch substantially, it's best practice to propagate the tags you already received :) that way it should just get taken! Anyway for avoidance of doubt: Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > --- > mm/rmap.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/mm/rmap.c b/mm/rmap.c > index 67bb273dfb80..b509c226e50d 100644 > --- a/mm/rmap.c > +++ b/mm/rmap.c > @@ -789,13 +789,13 @@ unsigned long page_address_in_vma(const struct folio *folio, > const struct page *page, const struct vm_area_struct *vma) > { > if (folio_test_anon(folio)) { > - struct anon_vma *page__anon_vma = folio_anon_vma(folio); > + struct anon_vma *page_anon_vma = folio_anon_vma(folio); > /* > * Note: swapoff's unuse_vma() is more efficient with this > * check, and needs it to match anon_vma when KSM is active. > */ > - if (!vma->anon_vma || !page__anon_vma || > - vma->anon_vma->root != page__anon_vma->root) > + if (!vma->anon_vma || !page_anon_vma || > + vma->anon_vma->root != page_anon_vma->root) > return -EFAULT; > } else if (!vma->vm_file) { > return -EFAULT; > @@ -803,7 +803,7 @@ unsigned long page_address_in_vma(const struct folio *folio, > return -EFAULT; > } > > - /* KSM folios don't reach here because of the !page__anon_vma check */ > + /* KSM folios don't reach here because of the !page_anon_vma check */ > return vma_address(vma, page_pgoff(folio, page), 1); > } > > -- > 2.25.1 >
在 2025/4/18 18:00, Lorenzo Stoakes 写道: > On Fri, Apr 18, 2025 at 05:55:59PM +0800, Ye Liu wrote: >> From: Ye Liu <liuye@kylinos.cn> >> >> Renamed local variable page__anon_vma in page_address_in_vma() to >> page_anon_vma. The previous naming convention of using double underscores >> (__) is unnecessary and inconsistent with typical kernel style, which uses >> single underscores to denote local variables. Also updated comments to >> reflect the new variable name. >> >> Functionality unchanged. >> >> Signed-off-by: Ye Liu <liuye@kylinos.cn> > For future reference, if you don't change the patch substantially, it's best > practice to propagate the tags you already received :) that way it should just > get taken! Got it, thanks for the tip! I’ll make sure to retain the ACKs and reviewers if the patch doesn’t change much next time. Appreciate the guidance :) Thanks, Ye > Anyway for avoidance of doubt: > > Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > >> --- >> mm/rmap.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/mm/rmap.c b/mm/rmap.c >> index 67bb273dfb80..b509c226e50d 100644 >> --- a/mm/rmap.c >> +++ b/mm/rmap.c >> @@ -789,13 +789,13 @@ unsigned long page_address_in_vma(const struct folio *folio, >> const struct page *page, const struct vm_area_struct *vma) >> { >> if (folio_test_anon(folio)) { >> - struct anon_vma *page__anon_vma = folio_anon_vma(folio); >> + struct anon_vma *page_anon_vma = folio_anon_vma(folio); >> /* >> * Note: swapoff's unuse_vma() is more efficient with this >> * check, and needs it to match anon_vma when KSM is active. >> */ >> - if (!vma->anon_vma || !page__anon_vma || >> - vma->anon_vma->root != page__anon_vma->root) >> + if (!vma->anon_vma || !page_anon_vma || >> + vma->anon_vma->root != page_anon_vma->root) >> return -EFAULT; >> } else if (!vma->vm_file) { >> return -EFAULT; >> @@ -803,7 +803,7 @@ unsigned long page_address_in_vma(const struct folio *folio, >> return -EFAULT; >> } >> >> - /* KSM folios don't reach here because of the !page__anon_vma check */ >> + /* KSM folios don't reach here because of the !page_anon_vma check */ >> return vma_address(vma, page_pgoff(folio, page), 1); >> } >> >> -- >> 2.25.1 >>
On 18.04.25 11:55, Ye Liu wrote: > From: Ye Liu <liuye@kylinos.cn> > > Renamed local variable page__anon_vma in page_address_in_vma() to > page_anon_vma. The previous naming convention of using double underscores > (__) is unnecessary and inconsistent with typical kernel style, which uses > single underscores to denote local variables. Also updated comments to > reflect the new variable name. > > Functionality unchanged. > > Signed-off-by: Ye Liu <liuye@kylinos.cn> > --- > mm/rmap.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/mm/rmap.c b/mm/rmap.c > index 67bb273dfb80..b509c226e50d 100644 > --- a/mm/rmap.c > +++ b/mm/rmap.c > @@ -789,13 +789,13 @@ unsigned long page_address_in_vma(const struct folio *folio, > const struct page *page, const struct vm_area_struct *vma) > { > if (folio_test_anon(folio)) { > - struct anon_vma *page__anon_vma = folio_anon_vma(folio); > + struct anon_vma *page_anon_vma = folio_anon_vma(folio); I'm extremely confused why this should not simply be called "anon_vma". Why do we need the "page" in here *at all* ?
On Fri, Apr 18, 2025 at 12:40:10PM +0200, David Hildenbrand wrote: > On 18.04.25 11:55, Ye Liu wrote: > > From: Ye Liu <liuye@kylinos.cn> > > > > Renamed local variable page__anon_vma in page_address_in_vma() to > > page_anon_vma. The previous naming convention of using double underscores > > (__) is unnecessary and inconsistent with typical kernel style, which uses > > single underscores to denote local variables. Also updated comments to > > reflect the new variable name. > > > > Functionality unchanged. > > > > Signed-off-by: Ye Liu <liuye@kylinos.cn> > > --- > > mm/rmap.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/mm/rmap.c b/mm/rmap.c > > index 67bb273dfb80..b509c226e50d 100644 > > --- a/mm/rmap.c > > +++ b/mm/rmap.c > > @@ -789,13 +789,13 @@ unsigned long page_address_in_vma(const struct folio *folio, > > const struct page *page, const struct vm_area_struct *vma) > > { > > if (folio_test_anon(folio)) { > > - struct anon_vma *page__anon_vma = folio_anon_vma(folio); > > + struct anon_vma *page_anon_vma = folio_anon_vma(folio); > > I'm extremely confused why this should not simply be called "anon_vma". Why > do we need the "page" in here *at all* ? Presumably to differentiate from the VMA's anon_vma, but it seems redundant and silly to preface it as the page's (really, folio's) anon_vma. The original patch is strictly an improvement so I'm fine with that, but we could also rename this to anon_vma to make the function a little simpler to read. I guess the key bit where this becomes vaguely relevant is: vma->anon_vma->root != page_anon_vma->root This whole thing seems to be to deal with races with unuse_vma() as per the comment. Anyway, TL;DR: fine with that rename if we want it! > > -- > Cheers, > > David / dhildenb >
On 18.04.25 12:50, Lorenzo Stoakes wrote: > On Fri, Apr 18, 2025 at 12:40:10PM +0200, David Hildenbrand wrote: >> On 18.04.25 11:55, Ye Liu wrote: >>> From: Ye Liu <liuye@kylinos.cn> >>> >>> Renamed local variable page__anon_vma in page_address_in_vma() to >>> page_anon_vma. The previous naming convention of using double underscores >>> (__) is unnecessary and inconsistent with typical kernel style, which uses >>> single underscores to denote local variables. Also updated comments to >>> reflect the new variable name. >>> >>> Functionality unchanged. >>> >>> Signed-off-by: Ye Liu <liuye@kylinos.cn> >>> --- >>> mm/rmap.c | 8 ++++---- >>> 1 file changed, 4 insertions(+), 4 deletions(-) >>> >>> diff --git a/mm/rmap.c b/mm/rmap.c >>> index 67bb273dfb80..b509c226e50d 100644 >>> --- a/mm/rmap.c >>> +++ b/mm/rmap.c >>> @@ -789,13 +789,13 @@ unsigned long page_address_in_vma(const struct folio *folio, >>> const struct page *page, const struct vm_area_struct *vma) >>> { >>> if (folio_test_anon(folio)) { >>> - struct anon_vma *page__anon_vma = folio_anon_vma(folio); >>> + struct anon_vma *page_anon_vma = folio_anon_vma(folio); >> >> I'm extremely confused why this should not simply be called "anon_vma". Why >> do we need the "page" in here *at all* ? > > Presumably to differentiate from the VMA's anon_vma, but it seems redundant > and silly to preface it as the page's (really, folio's) anon_vma. Exactly, thus my confusion :) > > The original patch is strictly an improvement so I'm fine with that, but we > could also rename this to anon_vma to make the function a little simpler to > read. > > I guess the key bit where this becomes vaguely relevant is: > > vma->anon_vma->root != page_anon_vma->root > > This whole thing seems to be to deal with races with unuse_vma() as per the > comment. > > Anyway, TL;DR: fine with that rename if we want it! Okay, let's drop the "page_" part if possible.
diff --git a/mm/rmap.c b/mm/rmap.c index 67bb273dfb80..b509c226e50d 100644 --- a/mm/rmap.c +++ b/mm/rmap.c @@ -789,13 +789,13 @@ unsigned long page_address_in_vma(const struct folio *folio, const struct page *page, const struct vm_area_struct *vma) { if (folio_test_anon(folio)) { - struct anon_vma *page__anon_vma = folio_anon_vma(folio); + struct anon_vma *page_anon_vma = folio_anon_vma(folio); /* * Note: swapoff's unuse_vma() is more efficient with this * check, and needs it to match anon_vma when KSM is active. */ - if (!vma->anon_vma || !page__anon_vma || - vma->anon_vma->root != page__anon_vma->root) + if (!vma->anon_vma || !page_anon_vma || + vma->anon_vma->root != page_anon_vma->root) return -EFAULT; } else if (!vma->vm_file) { return -EFAULT; @@ -803,7 +803,7 @@ unsigned long page_address_in_vma(const struct folio *folio, return -EFAULT; } - /* KSM folios don't reach here because of the !page__anon_vma check */ + /* KSM folios don't reach here because of the !page_anon_vma check */ return vma_address(vma, page_pgoff(folio, page), 1); }