Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1233717 > unrolled thread
| Started by | Alexandru Moise <00moses.alexander00@gmail.com> |
|---|---|
| First post | 2015-09-27 20:10 +0200 |
| Last post | 2015-09-29 12:40 +0200 |
| Articles | 3 — 2 participants |
Back to article view | Back to linux.kernel
[PATCH 1/2] mm: change free_cma and free_pages declarations to unsigned Alexandru Moise <00moses.alexander00@gmail.com> - 2015-09-27 20:10 +0200
Re: [PATCH 1/2] mm: change free_cma and free_pages declarations to unsigned Mel Gorman <mgorman@suse.de> - 2015-09-29 12:20 +0200
Re: [PATCH 1/2] mm: change free_cma and free_pages declarations to unsigned Alexandru Moise <00moses.alexander00@gmail.com> - 2015-09-29 12:40 +0200
| From | Alexandru Moise <00moses.alexander00@gmail.com> |
|---|---|
| Date | 2015-09-27 20:10 +0200 |
| Subject | [PATCH 1/2] mm: change free_cma and free_pages declarations to unsigned |
| Message-ID | <qdoyC-6jq-7@gated-at.bofh.it> |
Their stored values come from zone_page_state() which returns
an unsigned long. To improve code correctness we should avoid
mixing signed and unsigned integers.
Signed-off-by: Alexandru Moise <00moses.alexander00@gmail.com>
---
mm/page_alloc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 48aaf7b..f55e3a2 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2242,7 +2242,7 @@ static bool __zone_watermark_ok(struct zone *z, unsigned int order,
/* free_pages may go negative - that's OK */
long min = mark;
int o;
- long free_cma = 0;
+ unsigned long free_cma = 0;
free_pages -= (1 << order) - 1;
if (alloc_flags & ALLOC_HIGH)
@@ -2280,7 +2280,7 @@ bool zone_watermark_ok(struct zone *z, unsigned int order, unsigned long mark,
bool zone_watermark_ok_safe(struct zone *z, unsigned int order,
unsigned long mark, int classzone_idx, int alloc_flags)
{
- long free_pages = zone_page_state(z, NR_FREE_PAGES);
+ unsigned long free_pages = zone_page_state(z, NR_FREE_PAGES);
if (z->percpu_drift_mark && free_pages < z->percpu_drift_mark)
free_pages = zone_page_state_snapshot(z, NR_FREE_PAGES);
--
2.5.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Mel Gorman <mgorman@suse.de> |
|---|---|
| Date | 2015-09-29 12:20 +0200 |
| Message-ID | <qe0aR-3wa-9@gated-at.bofh.it> |
| In reply to | #1233717 |
On Sun, Sep 27, 2015 at 09:04:16PM +0000, Alexandru Moise wrote: > Their stored values come from zone_page_state() which returns > an unsigned long. To improve code correctness we should avoid > mixing signed and unsigned integers. > > Signed-off-by: Alexandru Moise <00moses.alexander00@gmail.com> > --- > mm/page_alloc.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 48aaf7b..f55e3a2 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -2242,7 +2242,7 @@ static bool __zone_watermark_ok(struct zone *z, unsigned int order, > /* free_pages may go negative - that's OK */ > long min = mark; > int o; > - long free_cma = 0; > + unsigned long free_cma = 0; > NAK. free_cma is used with free_pages which is explicitly commented as saying it can go negative. With your patch, there is a signed/unsigned operation where the unsigned type cannot fit into the signed type which casts them both to unsigned which is then broken for the comparison. This patch looks broken for very subtle reasons. Please do not do any similar style patches to this because they can introduce subtle breakage if issues are not caught at review. -- Mel Gorman SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Alexandru Moise <00moses.alexander00@gmail.com> |
|---|---|
| Date | 2015-09-29 12:40 +0200 |
| Message-ID | <qe0ud-3SL-13@gated-at.bofh.it> |
| In reply to | #1234939 |
On Tue, Sep 29, 2015 at 11:13:27AM +0100, Mel Gorman wrote: > On Sun, Sep 27, 2015 at 09:04:16PM +0000, Alexandru Moise wrote: > > Their stored values come from zone_page_state() which returns > > an unsigned long. To improve code correctness we should avoid > > mixing signed and unsigned integers. > > > > Signed-off-by: Alexandru Moise <00moses.alexander00@gmail.com> > > --- > > mm/page_alloc.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > > index 48aaf7b..f55e3a2 100644 > > --- a/mm/page_alloc.c > > +++ b/mm/page_alloc.c > > @@ -2242,7 +2242,7 @@ static bool __zone_watermark_ok(struct zone *z, unsigned int order, > > /* free_pages may go negative - that's OK */ > > long min = mark; > > int o; > > - long free_cma = 0; > > + unsigned long free_cma = 0; > > > > NAK. > > free_cma is used with free_pages which is explicitly commented as saying > it can go negative. With your patch, there is a signed/unsigned operation > where the unsigned type cannot fit into the signed type which casts them > both to unsigned which is then broken for the comparison. This patch > looks broken for very subtle reasons. Please do not do any similar style > patches to this because they can introduce subtle breakage if issues are > not caught at review. > Understood, I thought the comment only applied to the "min" variable. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web