Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1529556 > unrolled thread

[PATCH] z3fold: use %z modifier for format string

Back to article view | Back to linux.kernel

Contents

  [PATCH] z3fold: use %z modifier for format string Arnd Bergmann <arnd@arndb.de> - 2016-11-24 17:40 +0100
    Re: [PATCH] z3fold: use %z modifier for format string Joe Perches <joe@perches.com> - 2016-11-24 18:10 +0100
      Re: [PATCH] z3fold: use %z modifier for format string Vitaly Wool <vitalywool@gmail.com> - 2016-11-25 08:40 +0100
        Re: [PATCH] z3fold: use %z modifier for format string Arnd Bergmann <arnd@arndb.de> - 2016-11-25 09:50 +0100
          Re: [PATCH] z3fold: use %z modifier for format string Vitaly Wool <vitalywool@gmail.com> - 2016-11-25 17:10 +0100
            Re: [PATCH] z3fold: use %z modifier for format string Arnd Bergmann <arnd@arndb.de> - 2016-11-25 17:20 +0100
            Re: [PATCH] z3fold: use %z modifier for format string Dan Streetman <ddstreet@ieee.org> - 2016-11-25 19:40 +0100
    Re: [PATCH] z3fold: use %z modifier for format string Vitaly Wool <vitalywool@gmail.com> - 2016-11-25 08:40 +0100

#1529556 — [PATCH] z3fold: use %z modifier for format string

Printing a size_t requires the %zd format rather than %d:

mm/z3fold.c: In function ‘init_z3fold’:
include/linux/kern_levels.h:4:18: error: format ‘%d’ expects argument of type ‘int’, but argument 2 has type ‘long unsigned int’ [-Werror=format=]

Fixes: 50a50d2676c4 ("z3fold: don't fail kernel build if z3fold_header is too big")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 mm/z3fold.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/z3fold.c b/mm/z3fold.c
index e282ba073e77..66ac7a7dc934 100644
--- a/mm/z3fold.c
+++ b/mm/z3fold.c
@@ -884,7 +884,7 @@ static int __init init_z3fold(void)
 {
 	/* Fail the initialization if z3fold header won't fit in one chunk */
 	if (sizeof(struct z3fold_header) > ZHDR_SIZE_ALIGNED) {
-		pr_err("z3fold: z3fold_header size (%d) is bigger than "
+		pr_err("z3fold: z3fold_header size (%zd) is bigger than "
 			"the chunk size (%d), can't proceed\n",
 			sizeof(struct z3fold_header) , ZHDR_SIZE_ALIGNED);
 		return -E2BIG;
-- 
2.9.0

[toc] | [next] | [standalone]

#1529579

On Thu, 2016-11-24 at 17:31 +0100, Arnd Bergmann wrote:
> Printing a size_t requires the %zd format rather than %d:
> 
> mm/z3fold.c: In function ‘init_z3fold’:
> include/linux/kern_levels.h:4:18: error: format ‘%d’ expects argument of type ‘int’, but argument 2 has type ‘long unsigned int’ [-Werror=format=]
> 
> Fixes: 50a50d2676c4 ("z3fold: don't fail kernel build if z3fold_header is too big")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  mm/z3fold.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/z3fold.c b/mm/z3fold.c
> index e282ba073e77..66ac7a7dc934 100644
> --- a/mm/z3fold.c
> +++ b/mm/z3fold.c
> @@ -884,7 +884,7 @@ static int __init init_z3fold(void)
>  {
>  	/* Fail the initialization if z3fold header won't fit in one chunk */
>  	if (sizeof(struct z3fold_header) > ZHDR_SIZE_ALIGNED) {
> -		pr_err("z3fold: z3fold_header size (%d) is bigger than "
> +		pr_err("z3fold: z3fold_header size (%zd) is bigger than "
>  			"the chunk size (%d), can't proceed\n",
>  			sizeof(struct z3fold_header) , ZHDR_SIZE_ALIGNED);
>  		return -E2BIG;

The embedded "z3fold: " prefix here should be removed
as there's a pr_fmt that also adds it.

The test looks like it should be a BUILD_BUG_ON rather
than any runtime test too.

[toc] | [prev] | [next] | [standalone]

#1529847

Hi Joe,

On Thu, Nov 24, 2016 at 6:08 PM, Joe Perches <joe@perches.com> wrote:
> On Thu, 2016-11-24 at 17:31 +0100, Arnd Bergmann wrote:
>> Printing a size_t requires the %zd format rather than %d:
>>
>> mm/z3fold.c: In function ‘init_z3fold’:
>> include/linux/kern_levels.h:4:18: error: format ‘%d’ expects argument of type ‘int’, but argument 2 has type ‘long unsigned int’ [-Werror=format=]
>>
>> Fixes: 50a50d2676c4 ("z3fold: don't fail kernel build if z3fold_header is too big")
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> ---
>>  mm/z3fold.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/mm/z3fold.c b/mm/z3fold.c
>> index e282ba073e77..66ac7a7dc934 100644
>> --- a/mm/z3fold.c
>> +++ b/mm/z3fold.c
>> @@ -884,7 +884,7 @@ static int __init init_z3fold(void)
>>  {
>>       /* Fail the initialization if z3fold header won't fit in one chunk */
>>       if (sizeof(struct z3fold_header) > ZHDR_SIZE_ALIGNED) {
>> -             pr_err("z3fold: z3fold_header size (%d) is bigger than "
>> +             pr_err("z3fold: z3fold_header size (%zd) is bigger than "
>>                       "the chunk size (%d), can't proceed\n",
>>                       sizeof(struct z3fold_header) , ZHDR_SIZE_ALIGNED);
>>               return -E2BIG;
>
> The embedded "z3fold: " prefix here should be removed
> as there's a pr_fmt that also adds it.
>
> The test looks like it should be a BUILD_BUG_ON rather
> than any runtime test too.

It used to be BUILD_BUG_ON but we deliberately changed that because
sizeof(spinlock_t) gets bloated in debug builds, so it just won't
build with default CHUNK_SIZE.

~vitaly

[toc] | [prev] | [next] | [standalone]

#1529954

On Friday, November 25, 2016 8:38:25 AM CET Vitaly Wool wrote:
> >> diff --git a/mm/z3fold.c b/mm/z3fold.c
> >> index e282ba073e77..66ac7a7dc934 100644
> >> --- a/mm/z3fold.c
> >> +++ b/mm/z3fold.c
> >> @@ -884,7 +884,7 @@ static int __init init_z3fold(void)
> >>  {
> >>       /* Fail the initialization if z3fold header won't fit in one chunk */
> >>       if (sizeof(struct z3fold_header) > ZHDR_SIZE_ALIGNED) {
> >> -             pr_err("z3fold: z3fold_header size (%d) is bigger than "
> >> +             pr_err("z3fold: z3fold_header size (%zd) is bigger than "
> >>                       "the chunk size (%d), can't proceed\n",
> >>                       sizeof(struct z3fold_header) , ZHDR_SIZE_ALIGNED);
> >>               return -E2BIG;
> >
> > The embedded "z3fold: " prefix here should be removed
> > as there's a pr_fmt that also adds it.
> >
> > The test looks like it should be a BUILD_BUG_ON rather
> > than any runtime test too.
> 
> It used to be BUILD_BUG_ON but we deliberately changed that because
> sizeof(spinlock_t) gets bloated in debug builds, so it just won't
> build with default CHUNK_SIZE.

Could this be improved by making the CHUNK_SIZE bigger depending on
the debug options?

Alternatively, how about using a bit_spin_lock instead of raw_spin_lock?
That would guarantee a fixed size for the lock and make z3fold_header
always 24 bytes (on 32-bit architectures) or 40 bytes
(on 64-bit architectures). You could even play some tricks with the
first_num field to make it fit in the same word as the lock and make the
structure fit into 32 bytes if you care about that.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1530391

On Fri, Nov 25, 2016 at 9:41 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday, November 25, 2016 8:38:25 AM CET Vitaly Wool wrote:
>> >> diff --git a/mm/z3fold.c b/mm/z3fold.c
>> >> index e282ba073e77..66ac7a7dc934 100644
>> >> --- a/mm/z3fold.c
>> >> +++ b/mm/z3fold.c
>> >> @@ -884,7 +884,7 @@ static int __init init_z3fold(void)
>> >>  {
>> >>       /* Fail the initialization if z3fold header won't fit in one chunk */
>> >>       if (sizeof(struct z3fold_header) > ZHDR_SIZE_ALIGNED) {
>> >> -             pr_err("z3fold: z3fold_header size (%d) is bigger than "
>> >> +             pr_err("z3fold: z3fold_header size (%zd) is bigger than "
>> >>                       "the chunk size (%d), can't proceed\n",
>> >>                       sizeof(struct z3fold_header) , ZHDR_SIZE_ALIGNED);
>> >>               return -E2BIG;
>> >
>> > The embedded "z3fold: " prefix here should be removed
>> > as there's a pr_fmt that also adds it.
>> >
>> > The test looks like it should be a BUILD_BUG_ON rather
>> > than any runtime test too.
>>
>> It used to be BUILD_BUG_ON but we deliberately changed that because
>> sizeof(spinlock_t) gets bloated in debug builds, so it just won't
>> build with default CHUNK_SIZE.
>
> Could this be improved by making the CHUNK_SIZE bigger depending on
> the debug options?

I don't see how silently enforcing a suboptimal configuration is
better than failing the initialization (so that you can adjust
CHUNK_SIZE yourself). I can add something descriptive to
Documentation/vm/z3fold.txt for that matter.

> Alternatively, how about using a bit_spin_lock instead of raw_spin_lock?
> That would guarantee a fixed size for the lock and make z3fold_header
> always 24 bytes (on 32-bit architectures) or 40 bytes
> (on 64-bit architectures). You could even play some tricks with the
> first_num field to make it fit in the same word as the lock and make the
> structure fit into 32 bytes if you care about that.

That is interesting. Actually I can have that bit in page->private and
then I don't need to handle headless pages in a special way, that
sounds appealing. However, there is a warning about bit_spin_lock
performance penalty. Do you know how big it is?

Best regards,
   Vitaly

[toc] | [prev] | [next] | [standalone]

#1530400

On Friday, November 25, 2016 4:51:03 PM CET Vitaly Wool wrote:
> On Fri, Nov 25, 2016 at 9:41 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Friday, November 25, 2016 8:38:25 AM CET Vitaly Wool wrote:
> >> >> diff --git a/mm/z3fold.c b/mm/z3fold.c
> >> >> index e282ba073e77..66ac7a7dc934 100644
> >> >> --- a/mm/z3fold.c
> >> >> +++ b/mm/z3fold.c
> >> >> @@ -884,7 +884,7 @@ static int __init init_z3fold(void)
> >> >>  {
> >> >>       /* Fail the initialization if z3fold header won't fit in one chunk */
> >> >>       if (sizeof(struct z3fold_header) > ZHDR_SIZE_ALIGNED) {
> >> >> -             pr_err("z3fold: z3fold_header size (%d) is bigger than "
> >> >> +             pr_err("z3fold: z3fold_header size (%zd) is bigger than "
> >> >>                       "the chunk size (%d), can't proceed\n",
> >> >>                       sizeof(struct z3fold_header) , ZHDR_SIZE_ALIGNED);
> >> >>               return -E2BIG;
> >> >
> >> > The embedded "z3fold: " prefix here should be removed
> >> > as there's a pr_fmt that also adds it.
> >> >
> >> > The test looks like it should be a BUILD_BUG_ON rather
> >> > than any runtime test too.
> >>
> >> It used to be BUILD_BUG_ON but we deliberately changed that because
> >> sizeof(spinlock_t) gets bloated in debug builds, so it just won't
> >> build with default CHUNK_SIZE.
> >
> > Could this be improved by making the CHUNK_SIZE bigger depending on
> > the debug options?
> 
> I don't see how silently enforcing a suboptimal configuration is
> better than failing the initialization (so that you can adjust
> CHUNK_SIZE yourself). I can add something descriptive to
> Documentation/vm/z3fold.txt for that matter.

Failing at runtime when you know it's broken at compile-time
seems wrong, too. If you can't use z3fold with spinlock debugging,
you may as well hide the option in Kconfig based on the other ones.

Printing a runtime warning for the suboptimal configuration but
making it work anyway is probably better than just failing.

> > Alternatively, how about using a bit_spin_lock instead of raw_spin_lock?
> > That would guarantee a fixed size for the lock and make z3fold_header
> > always 24 bytes (on 32-bit architectures) or 40 bytes
> > (on 64-bit architectures). You could even play some tricks with the
> > first_num field to make it fit in the same word as the lock and make the
> > structure fit into 32 bytes if you care about that.
> 
> That is interesting. Actually I can have that bit in page->private and
> then I don't need to handle headless pages in a special way, that
> sounds appealing. However, there is a warning about bit_spin_lock
> performance penalty. Do you know how big it is?

No idea, sorry. On x86, test_and_set_bit() seems to be only
one instruction to test/set the bit, followed by a conditional
branch, compared to a cmpxchg() for the raw_spin_lock(), so the
fast path seems pretty much the same.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1530488

On Fri, Nov 25, 2016 at 10:51 AM, Vitaly Wool <vitalywool@gmail.com> wrote:
> On Fri, Nov 25, 2016 at 9:41 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Friday, November 25, 2016 8:38:25 AM CET Vitaly Wool wrote:
>>> >> diff --git a/mm/z3fold.c b/mm/z3fold.c
>>> >> index e282ba073e77..66ac7a7dc934 100644
>>> >> --- a/mm/z3fold.c
>>> >> +++ b/mm/z3fold.c
>>> >> @@ -884,7 +884,7 @@ static int __init init_z3fold(void)
>>> >>  {
>>> >>       /* Fail the initialization if z3fold header won't fit in one chunk */
>>> >>       if (sizeof(struct z3fold_header) > ZHDR_SIZE_ALIGNED) {
>>> >> -             pr_err("z3fold: z3fold_header size (%d) is bigger than "
>>> >> +             pr_err("z3fold: z3fold_header size (%zd) is bigger than "
>>> >>                       "the chunk size (%d), can't proceed\n",
>>> >>                       sizeof(struct z3fold_header) , ZHDR_SIZE_ALIGNED);
>>> >>               return -E2BIG;
>>> >
>>> > The embedded "z3fold: " prefix here should be removed
>>> > as there's a pr_fmt that also adds it.
>>> >
>>> > The test looks like it should be a BUILD_BUG_ON rather
>>> > than any runtime test too.
>>>
>>> It used to be BUILD_BUG_ON but we deliberately changed that because
>>> sizeof(spinlock_t) gets bloated in debug builds, so it just won't
>>> build with default CHUNK_SIZE.
>>
>> Could this be improved by making the CHUNK_SIZE bigger depending on
>> the debug options?
>
> I don't see how silently enforcing a suboptimal configuration is
> better than failing the initialization (so that you can adjust
> CHUNK_SIZE yourself). I can add something descriptive to
> Documentation/vm/z3fold.txt for that matter.
>
>> Alternatively, how about using a bit_spin_lock instead of raw_spin_lock?
>> That would guarantee a fixed size for the lock and make z3fold_header
>> always 24 bytes (on 32-bit architectures) or 40 bytes
>> (on 64-bit architectures). You could even play some tricks with the
>> first_num field to make it fit in the same word as the lock and make the
>> structure fit into 32 bytes if you care about that.
>
> That is interesting. Actually I can have that bit in page->private and
> then I don't need to handle headless pages in a special way, that
> sounds appealing. However, there is a warning about bit_spin_lock
> performance penalty. Do you know how big it is?

all these patches you're sending are to improve performance...why
would we then use bit spinlocks that degrade performance?  let's just
calculate the zhdr size correctly instead of assuming it's always <
chunk size.


>
> Best regards,
>    Vitaly

[toc] | [prev] | [next] | [standalone]

#1529846

Hi Arnd,

On Thu, Nov 24, 2016 at 5:31 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> Printing a size_t requires the %zd format rather than %d:
>
> mm/z3fold.c: In function ‘init_z3fold’:
> include/linux/kern_levels.h:4:18: error: format ‘%d’ expects argument of type ‘int’, but argument 2 has type ‘long unsigned int’ [-Werror=format=]
>
> Fixes: 50a50d2676c4 ("z3fold: don't fail kernel build if z3fold_header is too big")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Acked-by: Vitaly Wool <vitalywool@gmail.com>

And thanks :)

~vitaly

> ---
>  mm/z3fold.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/z3fold.c b/mm/z3fold.c
> index e282ba073e77..66ac7a7dc934 100644
> --- a/mm/z3fold.c
> +++ b/mm/z3fold.c
> @@ -884,7 +884,7 @@ static int __init init_z3fold(void)
>  {
>         /* Fail the initialization if z3fold header won't fit in one chunk */
>         if (sizeof(struct z3fold_header) > ZHDR_SIZE_ALIGNED) {
> -               pr_err("z3fold: z3fold_header size (%d) is bigger than "
> +               pr_err("z3fold: z3fold_header size (%zd) is bigger than "
>                         "the chunk size (%d), can't proceed\n",
>                         sizeof(struct z3fold_header) , ZHDR_SIZE_ALIGNED);
>                 return -E2BIG;
> --
> 2.9.0
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web