| Message ID | 1567649871-60594-1-git-send-email-zhongjiang@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] mm: Unsigned 'nr_pages' always larger than zero | expand |
On Thu, Sep 05, 2019 at 10:17:51AM +0800, zhong jiang wrote: > With the help of unsigned_lesser_than_zero.cocci. Unsigned 'nr_pages' > compare with zero. And __gup_longterm_locked pass an long local variant > 'rc' to check_and_migrate_cma_pages. Hence it is nicer to change the > parameter to long to fix the issue. I think this patch is right, but I have concerns about this cocci grep. The code says: if ((nr_pages > 0) && migrate_allow) { There's nothing wrong with this (... other than the fact that nr_pages might happen to be a negative errno). nr_pages might be 0, and this would be exactly the right test for that situation. I suppose some might argue that this should be != 0 instead of > 0, but it depends on the situation which one would read better. So please don't blindly make these changes; you're right this time.
On 2019/9/5 11:12, Matthew Wilcox wrote: > On Thu, Sep 05, 2019 at 10:17:51AM +0800, zhong jiang wrote: >> With the help of unsigned_lesser_than_zero.cocci. Unsigned 'nr_pages' >> compare with zero. And __gup_longterm_locked pass an long local variant >> 'rc' to check_and_migrate_cma_pages. Hence it is nicer to change the >> parameter to long to fix the issue. > I think this patch is right, but I have concerns about this cocci grep. > > The code says: > > if ((nr_pages > 0) && migrate_allow) { > > There's nothing wrong with this (... other than the fact that nr_pages might > happen to be a negative errno). nr_pages might be 0, and this would be > exactly the right test for that situation. I suppose some might argue > that this should be != 0 instead of > 0, but it depends on the situation > which one would read better. > > So please don't blindly make these changes; you're right this time. Thanks for your affirmation. but Andrew come up with anther fix, using an local long variant to store the nr_pages. which one do you prefer ? Thanks, zhong jiang
diff --git a/mm/gup.c b/mm/gup.c index 23a9f9c..ee0b71f 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -1433,13 +1433,13 @@ static struct page *new_non_cma_page(struct page *page, unsigned long private) static long check_and_migrate_cma_pages(struct task_struct *tsk, struct mm_struct *mm, unsigned long start, - unsigned long nr_pages, + long nr_pages, struct page **pages, struct vm_area_struct **vmas, unsigned int gup_flags) { - unsigned long i; - unsigned long step; + long i; + long step; bool drain_allow = true; bool migrate_allow = true; LIST_HEAD(cma_page_list); @@ -1520,7 +1520,7 @@ static long check_and_migrate_cma_pages(struct task_struct *tsk, static long check_and_migrate_cma_pages(struct task_struct *tsk, struct mm_struct *mm, unsigned long start, - unsigned long nr_pages, + long nr_pages, struct page **pages, struct vm_area_struct **vmas, unsigned int gup_flags)
With the help of unsigned_lesser_than_zero.cocci. Unsigned 'nr_pages' compare with zero. And __gup_longterm_locked pass an long local variant 'rc' to check_and_migrate_cma_pages. Hence it is nicer to change the parameter to long to fix the issue. Fixes: 932f4a630a69 ("mm/gup: replace get_user_pages_longterm() with FOLL_LONGTERM") Signed-off-by: zhong jiang <zhongjiang@huawei.com> --- mm/gup.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)