| Message ID | 20250330121718.175815-4-bhe@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/gup: Minor fix, cleanup and improvements | expand |
On Sun, Mar 30, 2025 at 08:17:13PM +0800, Baoquan He wrote: > Since commit f0818f472d8d ("mm: gup: add get_user_pages_locked > and get_user_pages_unlocked"), get_user_pages() doesn't need to have > mmap_lock held anymore. It calls __get_user_pages_locked() which > can acquire and drop the mmap_lock internaly. Yes, __get_user_pages_locked() can aquire and drop the lock, but AFAICS get_user_pages() always calls __get_user_pages_locked() with locked=1, which means that is holding the lock, right? > Hence remove the incorrect code comments now. > > Signed-off-by: Baoquan He <bhe@redhat.com> > --- > mm/gup.c | 16 +++------------- > 1 file changed, 3 insertions(+), 13 deletions(-) > > diff --git a/mm/gup.c b/mm/gup.c > index 8788105daee8..3345a065c2cb 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -2702,19 +2702,9 @@ long get_user_pages(unsigned long start, unsigned long nr_pages, > EXPORT_SYMBOL(get_user_pages); > > /* > - * get_user_pages_unlocked() is suitable to replace the form: > - * > - * mmap_read_lock(mm); > - * get_user_pages(mm, ..., pages, NULL); > - * mmap_read_unlock(mm); > - * > - * with: > - * > - * get_user_pages_unlocked(mm, ..., pages); > - * > - * It is functionally equivalent to get_user_pages_fast so > - * get_user_pages_fast should be used instead if specific gup_flags > - * (e.g. FOLL_FORCE) are not required. > + * get_user_pages_unlocked() is functionally equivalent to > + * get_user_pages_fast so get_user_pages_fast should be used instead > + * if specific gup_flags (e.g. FOLL_FORCE) are not required. > */ > long get_user_pages_unlocked(unsigned long start, unsigned long nr_pages, > struct page **pages, unsigned int gup_flags) > -- > 2.41.0 > >
On 03/31/25 at 03:58pm, Oscar Salvador wrote: > On Sun, Mar 30, 2025 at 08:17:13PM +0800, Baoquan He wrote: > > Since commit f0818f472d8d ("mm: gup: add get_user_pages_locked > > and get_user_pages_unlocked"), get_user_pages() doesn't need to have > > mmap_lock held anymore. It calls __get_user_pages_locked() which > > can acquire and drop the mmap_lock internaly. > > Yes, __get_user_pages_locked() can aquire and drop the lock, but AFAICS > get_user_pages() always calls __get_user_pages_locked() with locked=1, > which means that is holding the lock, right? Ah, You are right. Thanks for looking into this, Oscar. I incredibly missed the local virable definition "int locked = 1;" line. I will drop this patch, or wrap the code comment fix into other patch about the obsolete reference with "get_user_pages(mm, ..., pages, NULL)". > > > Hence remove the incorrect code comments now. > > > > Signed-off-by: Baoquan He <bhe@redhat.com> > > --- > > mm/gup.c | 16 +++------------- > > 1 file changed, 3 insertions(+), 13 deletions(-) > > > > diff --git a/mm/gup.c b/mm/gup.c > > index 8788105daee8..3345a065c2cb 100644 > > --- a/mm/gup.c > > +++ b/mm/gup.c > > @@ -2702,19 +2702,9 @@ long get_user_pages(unsigned long start, unsigned long nr_pages, > > EXPORT_SYMBOL(get_user_pages); > > > > /* > > - * get_user_pages_unlocked() is suitable to replace the form: > > - * > > - * mmap_read_lock(mm); > > - * get_user_pages(mm, ..., pages, NULL); > > - * mmap_read_unlock(mm); > > - * > > - * with: > > - * > > - * get_user_pages_unlocked(mm, ..., pages); > > - * > > - * It is functionally equivalent to get_user_pages_fast so > > - * get_user_pages_fast should be used instead if specific gup_flags > > - * (e.g. FOLL_FORCE) are not required. > > + * get_user_pages_unlocked() is functionally equivalent to > > + * get_user_pages_fast so get_user_pages_fast should be used instead > > + * if specific gup_flags (e.g. FOLL_FORCE) are not required. > > */ > > long get_user_pages_unlocked(unsigned long start, unsigned long nr_pages, > > struct page **pages, unsigned int gup_flags) > > -- > > 2.41.0 > > > > > > -- > Oscar Salvador > SUSE Labs >
diff --git a/mm/gup.c b/mm/gup.c index 8788105daee8..3345a065c2cb 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -2702,19 +2702,9 @@ long get_user_pages(unsigned long start, unsigned long nr_pages, EXPORT_SYMBOL(get_user_pages); /* - * get_user_pages_unlocked() is suitable to replace the form: - * - * mmap_read_lock(mm); - * get_user_pages(mm, ..., pages, NULL); - * mmap_read_unlock(mm); - * - * with: - * - * get_user_pages_unlocked(mm, ..., pages); - * - * It is functionally equivalent to get_user_pages_fast so - * get_user_pages_fast should be used instead if specific gup_flags - * (e.g. FOLL_FORCE) are not required. + * get_user_pages_unlocked() is functionally equivalent to + * get_user_pages_fast so get_user_pages_fast should be used instead + * if specific gup_flags (e.g. FOLL_FORCE) are not required. */ long get_user_pages_unlocked(unsigned long start, unsigned long nr_pages, struct page **pages, unsigned int gup_flags)
Since commit f0818f472d8d ("mm: gup: add get_user_pages_locked and get_user_pages_unlocked"), get_user_pages() doesn't need to have mmap_lock held anymore. It calls __get_user_pages_locked() which can acquire and drop the mmap_lock internaly. Hence remove the incorrect code comments now. Signed-off-by: Baoquan He <bhe@redhat.com> --- mm/gup.c | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-)