[PATCH] zsmalloc: use U suffix for negative literals being shifted

[PATCH] zsmalloc: use U suffix for negative literals being shifted

[Nick Desaulniers]

Fixes warnings about shifting unsigned literals being undefined
behavior.

Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
---
 mm/zsmalloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index 685049a..5d31458 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -1056,7 +1056,7 @@ static void init_zspage(struct size_class *class, struct zspage *zspage)
 			 * Reset OBJ_TAG_BITS bit to last link to tell
 			 * whether it's allocated object or not.
 			 */
-			link->next = -1 << OBJ_TAG_BITS;
+			link->next = -1U << OBJ_TAG_BITS;
 		}
 		kunmap_atomic(vaddr);
 		page = next_page;
-- 
2.7.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] zsmalloc: use U suffix for negative literals being shifted

[Matthew Wilcox]

On Sat, Dec 23, 2017 at 09:33:40PM -0500, Nick Desaulniers wrote:
> Fixes warnings about shifting unsigned literals being undefined
> behavior.

Do you mean signed literals?

>  			 */
> -			link->next = -1 << OBJ_TAG_BITS;
> +			link->next = -1U << OBJ_TAG_BITS;
>  		}

I don't understand what -1U means.  Seems like a contradiction in terms,
a negative unsigned number.  Is this supposed to be ~0U?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] zsmalloc: use U suffix for negative literals being shifted

[Nick Desaulniers]

On Sat, Dec 23, 2017 at 10:24 PM, Matthew Wilcox <willy@infradead.org> wrote:
> On Sat, Dec 23, 2017 at 09:33:40PM -0500, Nick Desaulniers wrote:
>> Fixes warnings about shifting unsigned literals being undefined
>> behavior.
>
> Do you mean signed literals?


A sorry, s/unsigned/negative signed/g.  The warning is:

mm/zsmalloc.c:1059:20: warning: shifting a negative signed value is undefined
      [-Wshift-negative-value]
                        link->next = -1 << OBJ_TAG_BITS;
                                     ~~ ^

>
>>                        */
>> -                     link->next = -1 << OBJ_TAG_BITS;
>> +                     link->next = -1U << OBJ_TAG_BITS;
>>               }
>
> I don't understand what -1U means.  Seems like a contradiction in terms,
> a negative unsigned number.  Is this supposed to be ~0U?

$ ag \\-1U[^L]

The code base is full of that literal.  I think of it as:

(unsigned) -1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] zsmalloc: use U suffix for negative literals being shifted

[Nick Desaulniers]

Hello,
What are the next steps for this patch?

Note: the MAINTAINERS file does not contain a T: (tree) entry for
ZSMALLOC, so I can't check to see if this has already been merged or
not.  Unless you work out of the mm tree?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] zsmalloc: use U suffix for negative literals being shifted

[Minchan Kim]

Hello,

Sorry for the delay. I have missed this until now. ;-(

On Sun, Dec 24, 2017 at 11:33 AM, Nick Desaulniers
<nick.desaulniers@gmail.com> wrote:
> Fixes warnings about shifting unsigned literals being undefined
> behavior.
>
> Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
> ---
>  mm/zsmalloc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
> index 685049a..5d31458 100644
> --- a/mm/zsmalloc.c
> +++ b/mm/zsmalloc.c
> @@ -1056,7 +1056,7 @@ static void init_zspage(struct size_class *class, struct zspage *zspage)
>                          * Reset OBJ_TAG_BITS bit to last link to tell
>                          * whether it's allocated object or not.
>                          */
> -                       link->next = -1 << OBJ_TAG_BITS;
> +                       link->next = -1U << OBJ_TAG_BITS;

-1UL?

Please, resend it with including Andrew Morton
<akpm@linux-foundation.org> who merges zsmalloc patch into his tree.

Thanks.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] zsmalloc: use U suffix for negative literals being shifted

[Andy Shevchenko]

On Sun, Jan 7, 2018 at 5:04 PM, Minchan Kim <minchan@kernel.org> wrote:

>> -                       link->next = -1 << OBJ_TAG_BITS;
>> +                       link->next = -1U << OBJ_TAG_BITS;
>
> -1UL?

Oh, boy, shouldn't be rather GENMASK() / GENMASK_ULL() in a way how
it's done, for example, here:
https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git/commit/?h=for-next&id=d2b3c353595a855794f8b9df5b5bdbe8deb0c413

-- 
With Best Regards,
Andy Shevchenko

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] zsmalloc: use U suffix for negative literals being shifted

[Nick Desaulniers]

On Sun, Jan 7, 2018 at 7:04 AM, Minchan Kim <minchan@kernel.org> wrote:
> Sorry for the delay. I have missed this until now. ;-(

No worries, figured patches would need a post holiday bump for review.

>
> On Sun, Dec 24, 2017 at 11:33 AM, Nick Desaulniers
> <nick.desaulniers@gmail.com> wrote:
>> -                       link->next = -1 << OBJ_TAG_BITS;
>> +                       link->next = -1U << OBJ_TAG_BITS;
>
> -1UL?

Oops, good catch.

> Please, resend it with including Andrew Morton
> <akpm@linux-foundation.org> who merges zsmalloc patch into his tree.

Will do.

On Sun, Jan 7, 2018 at 3:02 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> Oh, boy, shouldn't be rather GENMASK() / GENMASK_ULL() in a way how

Thanks for the suggestion. `GENMASK(BITS_PER_LONG - 1, OBJ_TAG_BITS);`
is equivalent.  Whether that is more readable, I'll wait for Minchan
to decide.  If that's preferred, I'll make sure to credit you with the
Suggested-By tag in the commit message.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] zsmalloc: use U suffix for negative literals being shifted

[Minchan Kim]

Hi Nick,

On Mon, Jan 08, 2018 at 08:35:19PM -0800, Nick Desaulniers wrote:
> On Sun, Jan 7, 2018 at 7:04 AM, Minchan Kim <minchan@kernel.org> wrote:
> > Sorry for the delay. I have missed this until now. ;-(
> 
> No worries, figured patches would need a post holiday bump for review.
> 
> >
> > On Sun, Dec 24, 2017 at 11:33 AM, Nick Desaulniers
> > <nick.desaulniers@gmail.com> wrote:
> >> -                       link->next = -1 << OBJ_TAG_BITS;
> >> +                       link->next = -1U << OBJ_TAG_BITS;
> >
> > -1UL?
> 
> Oops, good catch.
> 
> > Please, resend it with including Andrew Morton
> > <akpm@linux-foundation.org> who merges zsmalloc patch into his tree.
> 
> Will do.
> 
> On Sun, Jan 7, 2018 at 3:02 PM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > Oh, boy, shouldn't be rather GENMASK() / GENMASK_ULL() in a way how
> 
> Thanks for the suggestion. `GENMASK(BITS_PER_LONG - 1, OBJ_TAG_BITS);`
> is equivalent.  Whether that is more readable, I'll wait for Minchan
> to decide.  If that's preferred, I'll make sure to credit you with the
> Suggested-By tag in the commit message.

I don't see any benefit with GENMASK in our usecase.
If it's not a good justfication, I'd like to use just -1UL which
would be more readable without effort to understand new API.

Thanks.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v2] zsmalloc: use U suffix for negative literals being shifted

[Nick Desaulniers]

Fixes warnings about shifting unsigned literals being undefined
behavior.

Suggested-by: Minchan Kim <minchan@kernel.org>
Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
---
Changes since v1:
* Use L suffix in addition to U, as suggested (link->next is unsigned long).

 mm/zsmalloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index 683c065..b9040bd 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -1057,7 +1057,7 @@ static void init_zspage(struct size_class *class, struct zspage *zspage)
 			 * Reset OBJ_TAG_BITS bit to last link to tell
 			 * whether it's allocated object or not.
 			 */
-			link->next = -1 << OBJ_TAG_BITS;
+			link->next = -1UL << OBJ_TAG_BITS;
 		}
 		kunmap_atomic(vaddr);
 		page = next_page;
-- 
2.7.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2] zsmalloc: use U suffix for negative literals being shifted

[Sergey Senozhatsky]

On (01/10/18 19:41), Nick Desaulniers wrote:
> Fixes warnings about shifting unsigned literals being undefined
> behavior.
> 
> Suggested-by: Minchan Kim <minchan@kernel.org>
> Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>

looks good to me.

Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

	-ss

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>