* [PATCH] hugetlb: Fix an error code in hugetlb_reserve_pages()
@ 2020-12-11 10:13 ` Dan Carpenter
0 siblings, 0 replies; 11+ messages in thread
From: Dan Carpenter @ 2020-12-11 10:13 UTC (permalink / raw)
To: Mike Kravetz, Mina Almasry
Cc: Andrew Morton, David Rientjes, linux-mm, kernel-janitors
Preserve the error code from region_add() instead of returning success.
Fixes: 0db9d74ed884 ("hugetlb: disable region_add file_region coalescing")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
From static analysis. Untested.
mm/hugetlb.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 1f3bf1710b66..ac2e48b9f1d7 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -5113,6 +5113,7 @@ int hugetlb_reserve_pages(struct inode *inode,
if (unlikely(add < 0)) {
hugetlb_acct_memory(h, -gbl_reserve);
+ ret = add;
goto out_put_pages;
} else if (unlikely(chg > add)) {
/*
--
2.29.2
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH] hugetlb: Fix an error code in hugetlb_reserve_pages()
2020-12-11 10:13 ` Dan Carpenter
@ 2020-12-11 19:52 ` Souptick Joarder
-1 siblings, 0 replies; 11+ messages in thread
From: Souptick Joarder @ 2020-12-11 19:49 UTC (permalink / raw)
To: Dan Carpenter
Cc: Mike Kravetz, Mina Almasry, Andrew Morton, David Rientjes,
Linux-MM, kernel-janitors
Hi Dan,
On Fri, Dec 11, 2020 at 3:44 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> Preserve the error code from region_add() instead of returning success.
>
> Fixes: 0db9d74ed884 ("hugetlb: disable region_add file_region coalescing")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> From static analysis. Untested.
>
> mm/hugetlb.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 1f3bf1710b66..ac2e48b9f1d7 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -5113,6 +5113,7 @@ int hugetlb_reserve_pages(struct inode *inode,
>
> if (unlikely(add < 0)) {
> hugetlb_acct_memory(h, -gbl_reserve);
> + ret = add;
This function returns int but ret is long type.
Does it need correction ?
> goto out_put_pages;
> } else if (unlikely(chg > add)) {
> /*
> --
> 2.29.2
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] hugetlb: Fix an error code in hugetlb_reserve_pages()
@ 2020-12-11 19:52 ` Souptick Joarder
0 siblings, 0 replies; 11+ messages in thread
From: Souptick Joarder @ 2020-12-11 19:52 UTC (permalink / raw)
To: Dan Carpenter
Cc: Mike Kravetz, Mina Almasry, Andrew Morton, David Rientjes,
Linux-MM, kernel-janitors
Hi Dan,
On Fri, Dec 11, 2020 at 3:44 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> Preserve the error code from region_add() instead of returning success.
>
> Fixes: 0db9d74ed884 ("hugetlb: disable region_add file_region coalescing")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> From static analysis. Untested.
>
> mm/hugetlb.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 1f3bf1710b66..ac2e48b9f1d7 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -5113,6 +5113,7 @@ int hugetlb_reserve_pages(struct inode *inode,
>
> if (unlikely(add < 0)) {
> hugetlb_acct_memory(h, -gbl_reserve);
> + ret = add;
This function returns int but ret is long type.
Does it need correction ?
> goto out_put_pages;
> } else if (unlikely(chg > add)) {
> /*
> --
> 2.29.2
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] hugetlb: Fix an error code in hugetlb_reserve_pages()
2020-12-11 19:52 ` Souptick Joarder
(?)
@ 2020-12-11 21:45 ` Matthew Wilcox
-1 siblings, 0 replies; 11+ messages in thread
From: Matthew Wilcox @ 2020-12-11 21:45 UTC (permalink / raw)
To: Souptick Joarder
Cc: Dan Carpenter, Mike Kravetz, Mina Almasry, Andrew Morton,
David Rientjes, Linux-MM, kernel-janitors
On Sat, Dec 12, 2020 at 01:19:28AM +0530, Souptick Joarder wrote:
> > @@ -5113,6 +5113,7 @@ int hugetlb_reserve_pages(struct inode *inode,
> >
> > if (unlikely(add < 0)) {
> > hugetlb_acct_memory(h, -gbl_reserve);
> > + ret = add;
>
> This function returns int but ret is long type.
> Does it need correction ?
I wouold say "no", because 'ret' isn't returned _by_ this function (*),
its purpose is to capture the return value from other functions.
(*) OK, it is, if ret < 0. But ret < 0 really means "Is this an error
number", which can be perfectly well represented in an int, short or long.
char is too small ;-)
So the range of values which will be placed in 'ret' is (-4096-LONG_MAX]
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] hugetlb: Fix an error code in hugetlb_reserve_pages()
2020-12-11 19:52 ` Souptick Joarder
@ 2020-12-14 6:30 ` Dan Carpenter
-1 siblings, 0 replies; 11+ messages in thread
From: Dan Carpenter @ 2020-12-14 6:30 UTC (permalink / raw)
To: Souptick Joarder
Cc: Mike Kravetz, Mina Almasry, Andrew Morton, David Rientjes,
Linux-MM, kernel-janitors
On Sat, Dec 12, 2020 at 01:19:28AM +0530, Souptick Joarder wrote:
> Hi Dan,
>
> On Fri, Dec 11, 2020 at 3:44 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > Preserve the error code from region_add() instead of returning success.
> >
> > Fixes: 0db9d74ed884 ("hugetlb: disable region_add file_region coalescing")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> > From static analysis. Untested.
> >
> > mm/hugetlb.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> > index 1f3bf1710b66..ac2e48b9f1d7 100644
> > --- a/mm/hugetlb.c
> > +++ b/mm/hugetlb.c
> > @@ -5113,6 +5113,7 @@ int hugetlb_reserve_pages(struct inode *inode,
> >
> > if (unlikely(add < 0)) {
> > hugetlb_acct_memory(h, -gbl_reserve);
> > + ret = add;
>
> This function returns int but ret is long type.
> Does it need correction ?
>
It doesn't *need* correction. The code works fine as-is. Smatch parses
it correctly, also. Hard to say if making it "ret" makes the code
simpler or more complicated for a human reader.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] hugetlb: Fix an error code in hugetlb_reserve_pages()
@ 2020-12-14 6:30 ` Dan Carpenter
0 siblings, 0 replies; 11+ messages in thread
From: Dan Carpenter @ 2020-12-14 6:30 UTC (permalink / raw)
To: Souptick Joarder
Cc: Mike Kravetz, Mina Almasry, Andrew Morton, David Rientjes,
Linux-MM, kernel-janitors
On Sat, Dec 12, 2020 at 01:19:28AM +0530, Souptick Joarder wrote:
> Hi Dan,
>
> On Fri, Dec 11, 2020 at 3:44 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > Preserve the error code from region_add() instead of returning success.
> >
> > Fixes: 0db9d74ed884 ("hugetlb: disable region_add file_region coalescing")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> > From static analysis. Untested.
> >
> > mm/hugetlb.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> > index 1f3bf1710b66..ac2e48b9f1d7 100644
> > --- a/mm/hugetlb.c
> > +++ b/mm/hugetlb.c
> > @@ -5113,6 +5113,7 @@ int hugetlb_reserve_pages(struct inode *inode,
> >
> > if (unlikely(add < 0)) {
> > hugetlb_acct_memory(h, -gbl_reserve);
> > + ret = add;
>
> This function returns int but ret is long type.
> Does it need correction ?
>
It doesn't *need* correction. The code works fine as-is. Smatch parses
it correctly, also. Hard to say if making it "ret" makes the code
simpler or more complicated for a human reader.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] hugetlb: Fix an error code in hugetlb_reserve_pages()
2020-12-11 10:13 ` Dan Carpenter
@ 2020-12-14 9:37 ` David Hildenbrand
-1 siblings, 0 replies; 11+ messages in thread
From: David Hildenbrand @ 2020-12-14 9:37 UTC (permalink / raw)
To: Dan Carpenter, Mike Kravetz, Mina Almasry
Cc: Andrew Morton, David Rientjes, linux-mm, kernel-janitors
On 11.12.20 11:13, Dan Carpenter wrote:
> Preserve the error code from region_add() instead of returning success.
>
> Fixes: 0db9d74ed884 ("hugetlb: disable region_add file_region coalescing")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> From static analysis. Untested.
>
> mm/hugetlb.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 1f3bf1710b66..ac2e48b9f1d7 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -5113,6 +5113,7 @@ int hugetlb_reserve_pages(struct inode *inode,
>
> if (unlikely(add < 0)) {
> hugetlb_acct_memory(h, -gbl_reserve);
> + ret = add;
> goto out_put_pages;
> } else if (unlikely(chg > add)) {
> /*
>
Was wioing if something like
return ret ? ret : add;
would be cleaner, but then I spotted "chg" ...
Reviewed-by: David Hildenbrand <david@redhat.com>
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] hugetlb: Fix an error code in hugetlb_reserve_pages()
@ 2020-12-14 9:37 ` David Hildenbrand
0 siblings, 0 replies; 11+ messages in thread
From: David Hildenbrand @ 2020-12-14 9:37 UTC (permalink / raw)
To: Dan Carpenter, Mike Kravetz, Mina Almasry
Cc: Andrew Morton, David Rientjes, linux-mm, kernel-janitors
On 11.12.20 11:13, Dan Carpenter wrote:
> Preserve the error code from region_add() instead of returning success.
>
> Fixes: 0db9d74ed884 ("hugetlb: disable region_add file_region coalescing")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> From static analysis. Untested.
>
> mm/hugetlb.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 1f3bf1710b66..ac2e48b9f1d7 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -5113,6 +5113,7 @@ int hugetlb_reserve_pages(struct inode *inode,
>
> if (unlikely(add < 0)) {
> hugetlb_acct_memory(h, -gbl_reserve);
> + ret = add;
> goto out_put_pages;
> } else if (unlikely(chg > add)) {
> /*
>
Was wioing if something like
return ret ? ret : add;
would be cleaner, but then I spotted "chg" ...
Reviewed-by: David Hildenbrand <david@redhat.com>
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] hugetlb: Fix an error code in hugetlb_reserve_pages()
2020-12-11 10:13 ` Dan Carpenter
@ 2020-12-14 19:27 ` Mike Kravetz
-1 siblings, 0 replies; 11+ messages in thread
From: Mike Kravetz @ 2020-12-14 19:27 UTC (permalink / raw)
To: Dan Carpenter, Mina Almasry
Cc: Andrew Morton, David Rientjes, linux-mm, kernel-janitors
On 12/11/20 2:13 AM, Dan Carpenter wrote:
> Preserve the error code from region_add() instead of returning success.
>
> Fixes: 0db9d74ed884 ("hugetlb: disable region_add file_region coalescing")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> From static analysis. Untested.
Thanks Dan.
>
> mm/hugetlb.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 1f3bf1710b66..ac2e48b9f1d7 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -5113,6 +5113,7 @@ int hugetlb_reserve_pages(struct inode *inode,
>
> if (unlikely(add < 0)) {
> hugetlb_acct_memory(h, -gbl_reserve);
> + ret = add;
> goto out_put_pages;
> } else if (unlikely(chg > add)) {
> /*
>
That error path is VERY unlikely to be taken, but is indeed incorrect.
When looking at this, I noticed that callers of hugetlb_reserve_pages only
check for 0 or !0. This changed as the code evolved to add reservation
cgroup support. The routine type can be changed to a bool and simplified
some. I'll send that as a follow up patch not for stable.
Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>
--
Mike Kravetz
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] hugetlb: Fix an error code in hugetlb_reserve_pages()
@ 2020-12-14 19:27 ` Mike Kravetz
0 siblings, 0 replies; 11+ messages in thread
From: Mike Kravetz @ 2020-12-14 19:27 UTC (permalink / raw)
To: Dan Carpenter, Mina Almasry
Cc: Andrew Morton, David Rientjes, linux-mm, kernel-janitors
On 12/11/20 2:13 AM, Dan Carpenter wrote:
> Preserve the error code from region_add() instead of returning success.
>
> Fixes: 0db9d74ed884 ("hugetlb: disable region_add file_region coalescing")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> From static analysis. Untested.
Thanks Dan.
>
> mm/hugetlb.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 1f3bf1710b66..ac2e48b9f1d7 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -5113,6 +5113,7 @@ int hugetlb_reserve_pages(struct inode *inode,
>
> if (unlikely(add < 0)) {
> hugetlb_acct_memory(h, -gbl_reserve);
> + ret = add;
> goto out_put_pages;
> } else if (unlikely(chg > add)) {
> /*
>
That error path is VERY unlikely to be taken, but is indeed incorrect.
When looking at this, I noticed that callers of hugetlb_reserve_pages only
check for 0 or !0. This changed as the code evolved to add reservation
cgroup support. The routine type can be changed to a bool and simplified
some. I'll send that as a follow up patch not for stable.
Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>
--
Mike Kravetz
^ permalink raw reply [flat|nested] 11+ messages in thread