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

Groups > linux.kernel > #1230104 > unrolled thread

Re: crisv32 runtime failure in -next due to 'page-flags: define behavior SL*B-related flags on compound pages'

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: crisv32 runtime failure in -next due to 'page-flags: define  behavior SL*B-related flags on compound pages' "Kirill A. Shutemov" <kirill@shutemov.name> - 2015-09-22 14:10 +0200
    Re: crisv32 runtime failure in -next due to 'page-flags: define  behavior SL*B-related flags on compound pages' Mikael Starvik <mikael.starvik@axis.com> - 2015-09-22 14:20 +0200
      Re: crisv32 runtime failure in -next due to 'page-flags: define  behavior SL*B-related flags on compound pages' Hans-Peter Nilsson <hans-peter.nilsson@axis.com> - 2015-09-22 15:00 +0200
        Re: crisv32 runtime failure in -next due to 'page-flags: define  behavior SL*B-related flags on compound pages' "Kirill A. Shutemov" <kirill@shutemov.name> - 2015-09-22 15:30 +0200
          Re: crisv32 runtime failure in -next due to 'page-flags: define  behavior SL*B-related flags on compound pages' Hans-Peter Nilsson <hans-peter.nilsson@axis.com> - 2015-09-22 16:00 +0200
            Re: crisv32 runtime failure in -next due to 'page-flags: define  behavior SL*B-related flags on compound pages' "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-09-22 17:20 +0200
              Re: crisv32 runtime failure in -next due to 'page-flags: define  behavior SL*B-related flags on compound pages' "Kirill A. Shutemov" <kirill@shutemov.name> - 2015-09-22 17:40 +0200
                Re: crisv32 runtime failure in -next due to 'page-flags: define  behavior SL*B-related flags on compound pages' "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-09-22 17:50 +0200
                  Re: crisv32 runtime failure in -next due to 'page-flags: define  behavior SL*B-related flags on compound pages' "Kirill A. Shutemov" <kirill@shutemov.name> - 2015-09-23 13:00 +0200
                    Re: crisv32 runtime failure in -next due to 'page-flags: define  behavior SL*B-related flags on compound pages' Guenter Roeck <linux@roeck-us.net> - 2015-09-23 17:10 +0200
                      Re: crisv32 runtime failure in -next due to 'page-flags: define  behavior SL*B-related flags on compound pages' "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-09-24 07:00 +0200
                Re: crisv32 runtime failure in -next due to 'page-flags: define  behavior SL*B-related flags on compound pages' Hans-Peter Nilsson <hans-peter.nilsson@axis.com> - 2015-09-22 18:20 +0200
                  Re: crisv32 runtime failure in -next due to 'page-flags: define  behavior SL*B-related flags on compound pages' "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-09-22 18:40 +0200

#1230104 — Re: crisv32 runtime failure in -next due to 'page-flags: define behavior SL*B-related flags on compound pages'

On Mon, Sep 21, 2015 at 06:17:34PM -0700, Guenter Roeck wrote:
> On 09/21/2015 08:34 AM, Kirill A. Shutemov wrote:
> >Guenter Roeck wrote:
> >>On 09/18/2015 07:53 AM, Jesper Nilsson wrote:
> >>>On Fri, Sep 18, 2015 at 05:25:07PM +0300, Kirill A. Shutemov wrote:
> >>>>On Thu, Sep 17, 2015 at 09:29:27AM -0700, Guenter Roeck wrote:
> >>>>>Hi,
> >>>>>
> >>>>>my crisv32 qemu test fails with next-20150917 as follows.
> >>>>>
> >>>>>NET: Registered protocol family 16
> >>>>>kernel BUG at mm/slab.c:1648!
> >>>>>Linux 4.3.0-rc1-next-20150917 #1 Wed Sep 16 23:56:59 PDT 2015
> >>>>>Oops: 0000
> >>>>>
> >>>>>[ register dump follows ]
> >>>>>
> >>>>>See http://server.roeck-us.net:8010/builders/qemu-crisv32-next/builds/83/steps/qemubuildcommand/logs/stdio
> >>>>>for a complete log.
> >>>>
> >>>>Is there a chance to get proper backtrace?
> >>>
> >>>Yes, it should be possible with CONFIG_KALLSYMS=y in the kconfig.
> >>>
> >>
> >>Good to know. I added it to my configuration.
> >>
> >>Here it is:
> >>
> >>kernel BUG at mm/slab.c:1648!
> >
> >I still don't understand what's going on :(
> >Could you try with this instrumentation:
> >
> >diff --git a/mm/slab.c b/mm/slab.c
> >index ce9c6531e6f7..10035d1a06d3 100644
> >--- a/mm/slab.c
> >+++ b/mm/slab.c
> >@@ -1645,7 +1645,11 @@ static void kmem_freepages(struct kmem_cache *cachep, struct page *page)
> >                 sub_zone_page_state(page_zone(page),
> >                                 NR_SLAB_UNRECLAIMABLE, nr_freed);
> >
> >-       BUG_ON(!PageSlab(page));
> >+       if (!PageSlab(page)) {
> >+               dump_page(page, "page");
> >+               dump_page(compound_head(page), "compound_head(page)");
> >+               BUG();
> >+       }
> >         __ClearPageSlabPfmemalloc(page);
> >         __ClearPageSlab(page);
> >         page_mapcount_reset(page);
> >
> 
> page:c04a5340 count:1 mapcount:1 mapping:c1f34080 index:0xc1f34060
> flags: 0x80(slab)
> page dumped because: page
> page:c1f17a04 count:0 mapcount:1 mapping:00d13600 index:0xc0
> flags: 0x0()
> page dumped because: compound_head(page)
> 
> Does that help ?

Kinda. It's false positive PageTail() due low bit set in
page->rcu_head.next.

It happens (at least) due broken alignment of 'rcu' field within
task_struct -- offsetof(struct task_struct, rcu): 773.

That's looks veery broken. I would guess compiler does something horribly
wrong. I hope it's not an ABI issue. :-/

Mikael? Jesper?

-- 
 Kirill A. Shutemov
--
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]

#1230107

For cris it is completely valid to do that. It has been an issue before. If you for some reason really require dword alignment there should be an align in the struct.

CC:ing the compiler guy for further comments.

Best regard
/Mikael



> 22 sep 2015 kl. 14:03 skrev Kirill A. Shutemov <kirill@shutemov.name>:
> 
>> On Mon, Sep 21, 2015 at 06:17:34PM -0700, Guenter Roeck wrote:
>>> On 09/21/2015 08:34 AM, Kirill A. Shutemov wrote:
>>> Guenter Roeck wrote:
>>>>> On 09/18/2015 07:53 AM, Jesper Nilsson wrote:
>>>>>> On Fri, Sep 18, 2015 at 05:25:07PM +0300, Kirill A. Shutemov wrote:
>>>>>>> On Thu, Sep 17, 2015 at 09:29:27AM -0700, Guenter Roeck wrote:
>>>>>>> Hi,
>>>>>>> 
>>>>>>> my crisv32 qemu test fails with next-20150917 as follows.
>>>>>>> 
>>>>>>> NET: Registered protocol family 16
>>>>>>> kernel BUG at mm/slab.c:1648!
>>>>>>> Linux 4.3.0-rc1-next-20150917 #1 Wed Sep 16 23:56:59 PDT 2015
>>>>>>> Oops: 0000
>>>>>>> 
>>>>>>> [ register dump follows ]
>>>>>>> 
>>>>>>> See http://server.roeck-us.net:8010/builders/qemu-crisv32-next/builds/83/steps/qemubuildcommand/logs/stdio
>>>>>>> for a complete log.
>>>>>> 
>>>>>> Is there a chance to get proper backtrace?
>>>>> 
>>>>> Yes, it should be possible with CONFIG_KALLSYMS=y in the kconfig.
>>>> 
>>>> Good to know. I added it to my configuration.
>>>> 
>>>> Here it is:
>>>> 
>>>> kernel BUG at mm/slab.c:1648!
>>> 
>>> I still don't understand what's going on :(
>>> Could you try with this instrumentation:
>>> 
>>> diff --git a/mm/slab.c b/mm/slab.c
>>> index ce9c6531e6f7..10035d1a06d3 100644
>>> --- a/mm/slab.c
>>> +++ b/mm/slab.c
>>> @@ -1645,7 +1645,11 @@ static void kmem_freepages(struct kmem_cache *cachep, struct page *page)
>>>                sub_zone_page_state(page_zone(page),
>>>                                NR_SLAB_UNRECLAIMABLE, nr_freed);
>>> 
>>> -       BUG_ON(!PageSlab(page));
>>> +       if (!PageSlab(page)) {
>>> +               dump_page(page, "page");
>>> +               dump_page(compound_head(page), "compound_head(page)");
>>> +               BUG();
>>> +       }
>>>        __ClearPageSlabPfmemalloc(page);
>>>        __ClearPageSlab(page);
>>>        page_mapcount_reset(page);
>> 
>> page:c04a5340 count:1 mapcount:1 mapping:c1f34080 index:0xc1f34060
>> flags: 0x80(slab)
>> page dumped because: page
>> page:c1f17a04 count:0 mapcount:1 mapping:00d13600 index:0xc0
>> flags: 0x0()
>> page dumped because: compound_head(page)
>> 
>> Does that help ?
> 
> Kinda. It's false positive PageTail() due low bit set in
> page->rcu_head.next.
> 
> It happens (at least) due broken alignment of 'rcu' field within
> task_struct -- offsetof(struct task_struct, rcu): 773.
> 
> That's looks veery broken. I would guess compiler does something horribly
> wrong. I hope it's not an ABI issue. :-/
> 
> Mikael? Jesper?
> 
> -- 
> Kirill A. Shutemov
--
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]

#1230151

> From: Mikael Starvik <mikael.starvik@axis.com>
> Date: Tue, 22 Sep 2015 14:19:38 +0200

> For cris it is completely valid to do that.

Correct, just as it's completely valid for any system to specify
an ABI that says that structures are laid out "packed" by
default.

> It has been an
> issue before. If you for some reason really require dword
> alignment there should be an align in the struct.

Yep.

> CC:ing the compiler guy for further comments.

I have no new information.

> > 22 sep 2015 kl. 14:03 skrev Kirill A. Shutemov <kirill@shutemov.name>:

> > Kinda. It's false positive PageTail() due low bit set in
> > page->rcu_head.next.
> > 
> > It happens (at least) due broken alignment of 'rcu' field within
> > task_struct -- offsetof(struct task_struct, rcu): 773.
> > 
> > That's looks veery broken. I would guess compiler does something horribly
> > wrong. I hope it's not an ABI issue. :-/

It is an ABI issue, but I'm sure you can cope.  If you need to
imply something you have to provide something.  If not, I'd say
the term "horrible" would fit hackish assumptions of the failing
code (and related code that works by happenstance).

That element (the struct) needs *explicit* padding or alignment
to the required multiplicity of bytes for anyone to portably be
able to imply something other than "byte alignment" for the
layout of it, as elements of an array, across systems.  Use
dummy elements or a compiler construct like __attribute__
((__aligned__ (...))) per kernel policy or taste.  I'd recommend
specifying the alignment, so TRT will happen for it when it in
turn is an element of an otherwise unpadded struct.

(I assume all applicable allocators provide "natural" alignment
of, say, sizeof (long)) or that'll be a separate issue.)

brgds, H-P
--
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]

#1230190

On Tue, Sep 22, 2015 at 02:50:19PM +0200, Hans-Peter Nilsson wrote:
> > From: Mikael Starvik <mikael.starvik@axis.com>
> > Date: Tue, 22 Sep 2015 14:19:38 +0200
> 
> > For cris it is completely valid to do that.
> 
> Correct, just as it's completely valid for any system to specify
> an ABI that says that structures are laid out "packed" by
> default.
> 
> > It has been an
> > issue before. If you for some reason really require dword
> > alignment there should be an align in the struct.
> 
> Yep.
> 
> > CC:ing the compiler guy for further comments.
> 
> I have no new information.
> 
> > > 22 sep 2015 kl. 14:03 skrev Kirill A. Shutemov <kirill@shutemov.name>:
> 
> > > Kinda. It's false positive PageTail() due low bit set in
> > > page->rcu_head.next.
> > > 
> > > It happens (at least) due broken alignment of 'rcu' field within
> > > task_struct -- offsetof(struct task_struct, rcu): 773.
> > > 
> > > That's looks veery broken. I would guess compiler does something horribly
> > > wrong. I hope it's not an ABI issue. :-/
> 
> It is an ABI issue, but I'm sure you can cope.  If you need to
> imply something you have to provide something.  If not, I'd say
> the term "horrible" would fit hackish assumptions of the failing
> code (and related code that works by happenstance).
> 
> That element (the struct) needs *explicit* padding or alignment
> to the required multiplicity of bytes for anyone to portably be
> able to imply something other than "byte alignment" for the
> layout of it, as elements of an array, across systems.  Use
> dummy elements or a compiler construct like __attribute__
> ((__aligned__ (...))) per kernel policy or taste.  I'd recommend
> specifying the alignment, so TRT will happen for it when it in
> turn is an element of an otherwise unpadded struct.
> 
> (I assume all applicable allocators provide "natural" alignment
> of, say, sizeof (long)) or that'll be a separate issue.)

I see. I would say it's very risky ABI choice, but okay.

What was the reason behind? I don't understand it.
Is it free to make misaligned memory access on CRIS?
What about atomicity? How it works for misaligned accesses?

The patch below fixes issue for me.

I'm not sure if we want to ask for alignment to sizeof(long).
aligned(2) works too.

Paul, any thoughts?

diff --git a/include/linux/types.h b/include/linux/types.h
index c314989d9158..ead18bdcaf70 100644
--- a/include/linux/types.h
+++ b/include/linux/types.h
@@ -209,7 +209,7 @@ struct ustat {
 struct callback_head {
        struct callback_head *next;
        void (*func)(struct callback_head *head);
-};
+}  __attribute__((aligned(sizeof(long))));
 #define rcu_head callback_head
 
 typedef void (*rcu_callback_t)(struct rcu_head *head);
-- 
 Kirill A. Shutemov
--
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]

#1230234

> From: "Kirill A. Shutemov" <kirill@shutemov.name>
> Date: Tue, 22 Sep 2015 15:27:51 +0200

> On Tue, Sep 22, 2015 at 02:50:19PM +0200, Hans-Peter Nilsson wrote:
> > That element (the struct) needs *explicit* padding or alignment
> > to the required multiplicity of bytes for anyone to portably be
> > able to imply something other than "byte alignment" for the
> > layout of it, as elements of an array, across systems.  Use
> > dummy elements or a compiler construct like __attribute__
> > ((__aligned__ (...))) per kernel policy or taste.  I'd recommend
> > specifying the alignment, so TRT will happen for it when it in
> > turn is an element of an otherwise unpadded struct.

> I see. I would say it's very risky ABI choice, but okay.
> 
> What was the reason behind? I don't understand it.

It was made some 20+ years ago, some of the reason being (here's
the irony) compatibility with a toolchain for another
architecture, popular at the time, now forgotten.
Another reason (IIRC) was that it saves space. :)

> Is it free to make misaligned memory access on CRIS?

Within a cache-line for CRIS v32, it's free.

> What about atomicity? How it works for misaligned accesses?

Good spotting.  No system with page layouts fixed at size
multiples (all are, it'd be crazy to split pages as low as byte
boundaries) can support naturally-misaligned atomic accesses.

Therefore elements with access expecting atomicity, have be
decorated with alignment-inducing attributes, for portability,
e.g. to work for CRIS.  In userspace, I can at times get away
with calling a special function with a process-wide lock, in
those cases where the upstream project is unlikely to timely
understand e.g. that a naked "int" is not naturally aligned.

> The patch below fixes issue for me.

Thanks.

> I'm not sure if we want to ask for alignment to sizeof(long).
> aligned(2) works too.

I guess you hit the right spot, but I'd think people would be
more comfortable with aligning to sizeof (void *).

brgds, H-P
--
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]

#1230304

On Tue, Sep 22, 2015 at 03:57:06PM +0200, Hans-Peter Nilsson wrote:
> > From: "Kirill A. Shutemov" <kirill@shutemov.name>
> > Date: Tue, 22 Sep 2015 15:27:51 +0200
> 
> > On Tue, Sep 22, 2015 at 02:50:19PM +0200, Hans-Peter Nilsson wrote:
> > > That element (the struct) needs *explicit* padding or alignment
> > > to the required multiplicity of bytes for anyone to portably be
> > > able to imply something other than "byte alignment" for the
> > > layout of it, as elements of an array, across systems.  Use
> > > dummy elements or a compiler construct like __attribute__
> > > ((__aligned__ (...))) per kernel policy or taste.  I'd recommend
> > > specifying the alignment, so TRT will happen for it when it in
> > > turn is an element of an otherwise unpadded struct.
> 
> > I see. I would say it's very risky ABI choice, but okay.
> > 
> > What was the reason behind? I don't understand it.
> 
> It was made some 20+ years ago, some of the reason being (here's
> the irony) compatibility with a toolchain for another
> architecture, popular at the time, now forgotten.
> Another reason (IIRC) was that it saves space. :)
> 
> > Is it free to make misaligned memory access on CRIS?
> 
> Within a cache-line for CRIS v32, it's free.
> 
> > What about atomicity? How it works for misaligned accesses?
> 
> Good spotting.  No system with page layouts fixed at size
> multiples (all are, it'd be crazy to split pages as low as byte
> boundaries) can support naturally-misaligned atomic accesses.
> 
> Therefore elements with access expecting atomicity, have be
> decorated with alignment-inducing attributes, for portability,
> e.g. to work for CRIS.  In userspace, I can at times get away
> with calling a special function with a process-wide lock, in
> those cases where the upstream project is unlikely to timely
> understand e.g. that a naked "int" is not naturally aligned.
> 
> > The patch below fixes issue for me.
> 
> Thanks.
> 
> > I'm not sure if we want to ask for alignment to sizeof(long).
> > aligned(2) works too.
> 
> I guess you hit the right spot, but I'd think people would be
> more comfortable with aligning to sizeof (void *).

I would indeed prefer sizeof(void *).

						Thanx, Paul

--
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]

#1230343

On Tue, Sep 22, 2015 at 08:18:35AM -0700, Paul E. McKenney wrote:
> On Tue, Sep 22, 2015 at 03:57:06PM +0200, Hans-Peter Nilsson wrote:
> > I guess you hit the right spot, but I'd think people would be
> > more comfortable with aligning to sizeof (void *).
> 
> I would indeed prefer sizeof(void *).

Do you prefer to have the attribute set for whole structure or for ->next?
I think attribute on ->next is more appropriate from documentation POV.

-- 
 Kirill A. Shutemov
--
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]

#1230364

On Tue, Sep 22, 2015 at 06:31:04PM +0300, Kirill A. Shutemov wrote:
> On Tue, Sep 22, 2015 at 08:18:35AM -0700, Paul E. McKenney wrote:
> > On Tue, Sep 22, 2015 at 03:57:06PM +0200, Hans-Peter Nilsson wrote:
> > > I guess you hit the right spot, but I'd think people would be
> > > more comfortable with aligning to sizeof (void *).
> > 
> > I would indeed prefer sizeof(void *).
> 
> Do you prefer to have the attribute set for whole structure or for ->next?
> I think attribute on ->next is more appropriate from documentation POV.

Agreed, I do prefer ->next.

							Thanx, Paul

--
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]

#1231340

On Tue, Sep 22, 2015 at 08:40:14AM -0700, Paul E. McKenney wrote:
> On Tue, Sep 22, 2015 at 06:31:04PM +0300, Kirill A. Shutemov wrote:
> > On Tue, Sep 22, 2015 at 08:18:35AM -0700, Paul E. McKenney wrote:
> > > On Tue, Sep 22, 2015 at 03:57:06PM +0200, Hans-Peter Nilsson wrote:
> > > > I guess you hit the right spot, but I'd think people would be
> > > > more comfortable with aligning to sizeof (void *).
> > > 
> > > I would indeed prefer sizeof(void *).
> > 
> > Do you prefer to have the attribute set for whole structure or for ->next?
> > I think attribute on ->next is more appropriate from documentation POV.

I retract this claim: we have requirement about pointee alignment, not
pointer alignment.

From edbab9e89f5e4ad42e63d93ab05519e6a5f4d552 Mon Sep 17 00:00:00 2001
From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Date: Wed, 23 Sep 2015 13:39:28 +0300
Subject: [PATCH] rcu: force alignment on struct callback_head/rcu_head

This patch makes struct callback_head aligned to size of pointer. On
most architectures it happens naturally due ABI requirements, but some
architectures (like CRIS) have weird ABI and we need to ask it
explicitly.

The alignment is required to guarantee that bits 0 and 1 of @next will
be clear under normal conditions -- as long as we use call_rcu(),
call_rcu_bh(), call_rcu_sched(), or call_srcu() to queue callback.

This guarantee is important for few reasons:
 - future call_rcu_lazy() will make use of lower bits in the pointer;
 - the structure shares storage spacer in struct page with @compound_head,
   which encode PageTail() in bit 0. The guarantee is needed to avoid
   false-positive PageTail().

False postive PageTail() caused crash on crisv32[1]. It happend due
misaligned task_struct->rcu, which was byte-aligned.

[1] http://lkml.kernel.org/r/55FAEA67.9000102@roeck-us.net

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Reported-by: Guenter Roeck <linux@roeck-us.net>
---
 include/linux/types.h | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/include/linux/types.h b/include/linux/types.h
index c314989d9158..70d8500bddf1 100644
--- a/include/linux/types.h
+++ b/include/linux/types.h
@@ -205,11 +205,25 @@ struct ustat {
  * struct callback_head - callback structure for use with RCU and task_work
  * @next: next update requests in a list
  * @func: actual update function to call after the grace period.
+ *
+ * The struct is aligned to size of pointer. On most architectures it happens
+ * naturally due ABI requirements, but some architectures (like CRIS) have
+ * weird ABI and we need to ask it explicitly.
+ *
+ * The alignment is required to guarantee that bits 0 and 1 of @next will be
+ * clear under normal conditions -- as long as we use call_rcu(),
+ * call_rcu_bh(), call_rcu_sched(), or call_srcu() to queue callback.
+ *
+ * This guarantee is important for few reasons:
+ *  - future call_rcu_lazy() will make use of lower bits in the pointer;
+ *  - the structure shares storage spacer in struct page with @compound_head,
+ *    which encode PageTail() in bit 0. The guarantee is needed to avoid
+ *    false-positive PageTail().
  */
 struct callback_head {
 	struct callback_head *next;
 	void (*func)(struct callback_head *head);
-};
+} __attribute__((aligned(sizeof(void *))));
 #define rcu_head callback_head
 
 typedef void (*rcu_callback_t)(struct rcu_head *head);
-- 
 Kirill A. Shutemov
--
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]

#1231497

On 09/23/2015 03:53 AM, Kirill A. Shutemov wrote:
> On Tue, Sep 22, 2015 at 08:40:14AM -0700, Paul E. McKenney wrote:
>> On Tue, Sep 22, 2015 at 06:31:04PM +0300, Kirill A. Shutemov wrote:
>>> On Tue, Sep 22, 2015 at 08:18:35AM -0700, Paul E. McKenney wrote:
>>>> On Tue, Sep 22, 2015 at 03:57:06PM +0200, Hans-Peter Nilsson wrote:
>>>>> I guess you hit the right spot, but I'd think people would be
>>>>> more comfortable with aligning to sizeof (void *).
>>>>
>>>> I would indeed prefer sizeof(void *).
>>>
>>> Do you prefer to have the attribute set for whole structure or for ->next?
>>> I think attribute on ->next is more appropriate from documentation POV.
>
> I retract this claim: we have requirement about pointee alignment, not
> pointer alignment.
>
>>From edbab9e89f5e4ad42e63d93ab05519e6a5f4d552 Mon Sep 17 00:00:00 2001
> From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> Date: Wed, 23 Sep 2015 13:39:28 +0300
> Subject: [PATCH] rcu: force alignment on struct callback_head/rcu_head
>
> This patch makes struct callback_head aligned to size of pointer. On
> most architectures it happens naturally due ABI requirements, but some
> architectures (like CRIS) have weird ABI and we need to ask it
> explicitly.
>
> The alignment is required to guarantee that bits 0 and 1 of @next will
> be clear under normal conditions -- as long as we use call_rcu(),
> call_rcu_bh(), call_rcu_sched(), or call_srcu() to queue callback.
>
> This guarantee is important for few reasons:
>   - future call_rcu_lazy() will make use of lower bits in the pointer;
>   - the structure shares storage spacer in struct page with @compound_head,
>     which encode PageTail() in bit 0. The guarantee is needed to avoid
>     false-positive PageTail().
>
> False postive PageTail() caused crash on crisv32[1]. It happend due
> misaligned task_struct->rcu, which was byte-aligned.
>
> [1] http://lkml.kernel.org/r/55FAEA67.9000102@roeck-us.net
>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Reported-by: Guenter Roeck <linux@roeck-us.net>

Tested-by: Guenter Roeck <linux@roeck-us.net>

Hope the patch won't get lost since it was attached to an e-mail.
Can it be added to the branch introducing the problem ?

Thanks,
Guenter

> ---
>   include/linux/types.h | 16 +++++++++++++++-
>   1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/types.h b/include/linux/types.h
> index c314989d9158..70d8500bddf1 100644
> --- a/include/linux/types.h
> +++ b/include/linux/types.h
> @@ -205,11 +205,25 @@ struct ustat {
>    * struct callback_head - callback structure for use with RCU and task_work
>    * @next: next update requests in a list
>    * @func: actual update function to call after the grace period.
> + *
> + * The struct is aligned to size of pointer. On most architectures it happens
> + * naturally due ABI requirements, but some architectures (like CRIS) have
> + * weird ABI and we need to ask it explicitly.
> + *
> + * The alignment is required to guarantee that bits 0 and 1 of @next will be
> + * clear under normal conditions -- as long as we use call_rcu(),
> + * call_rcu_bh(), call_rcu_sched(), or call_srcu() to queue callback.
> + *
> + * This guarantee is important for few reasons:
> + *  - future call_rcu_lazy() will make use of lower bits in the pointer;
> + *  - the structure shares storage spacer in struct page with @compound_head,
> + *    which encode PageTail() in bit 0. The guarantee is needed to avoid
> + *    false-positive PageTail().
>    */
>   struct callback_head {
>   	struct callback_head *next;
>   	void (*func)(struct callback_head *head);
> -};
> +} __attribute__((aligned(sizeof(void *))));
>   #define rcu_head callback_head
>
>   typedef void (*rcu_callback_t)(struct rcu_head *head);
>

--
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]

#1231856

On Wed, Sep 23, 2015 at 08:02:44AM -0700, Guenter Roeck wrote:
> On 09/23/2015 03:53 AM, Kirill A. Shutemov wrote:
> >On Tue, Sep 22, 2015 at 08:40:14AM -0700, Paul E. McKenney wrote:
> >>On Tue, Sep 22, 2015 at 06:31:04PM +0300, Kirill A. Shutemov wrote:
> >>>On Tue, Sep 22, 2015 at 08:18:35AM -0700, Paul E. McKenney wrote:
> >>>>On Tue, Sep 22, 2015 at 03:57:06PM +0200, Hans-Peter Nilsson wrote:
> >>>>>I guess you hit the right spot, but I'd think people would be
> >>>>>more comfortable with aligning to sizeof (void *).
> >>>>
> >>>>I would indeed prefer sizeof(void *).
> >>>
> >>>Do you prefer to have the attribute set for whole structure or for ->next?
> >>>I think attribute on ->next is more appropriate from documentation POV.
> >
> >I retract this claim: we have requirement about pointee alignment, not
> >pointer alignment.
> >
> >>From edbab9e89f5e4ad42e63d93ab05519e6a5f4d552 Mon Sep 17 00:00:00 2001
> >From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> >Date: Wed, 23 Sep 2015 13:39:28 +0300
> >Subject: [PATCH] rcu: force alignment on struct callback_head/rcu_head
> >
> >This patch makes struct callback_head aligned to size of pointer. On
> >most architectures it happens naturally due ABI requirements, but some
> >architectures (like CRIS) have weird ABI and we need to ask it
> >explicitly.
> >
> >The alignment is required to guarantee that bits 0 and 1 of @next will
> >be clear under normal conditions -- as long as we use call_rcu(),
> >call_rcu_bh(), call_rcu_sched(), or call_srcu() to queue callback.
> >
> >This guarantee is important for few reasons:
> >  - future call_rcu_lazy() will make use of lower bits in the pointer;
> >  - the structure shares storage spacer in struct page with @compound_head,
> >    which encode PageTail() in bit 0. The guarantee is needed to avoid
> >    false-positive PageTail().
> >
> >False postive PageTail() caused crash on crisv32[1]. It happend due
> >misaligned task_struct->rcu, which was byte-aligned.
> >
> >[1] http://lkml.kernel.org/r/55FAEA67.9000102@roeck-us.net
> >
> >Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> >Reported-by: Guenter Roeck <linux@roeck-us.net>
> 
> Tested-by: Guenter Roeck <linux@roeck-us.net>
> 
> Hope the patch won't get lost since it was attached to an e-mail.
> Can it be added to the branch introducing the problem ?

Andrew Morton picked it up.  No idea where the problem was introduced.

						Thanx, Paul

> Thanks,
> Guenter
> 
> >---
> >  include/linux/types.h | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> >
> >diff --git a/include/linux/types.h b/include/linux/types.h
> >index c314989d9158..70d8500bddf1 100644
> >--- a/include/linux/types.h
> >+++ b/include/linux/types.h
> >@@ -205,11 +205,25 @@ struct ustat {
> >   * struct callback_head - callback structure for use with RCU and task_work
> >   * @next: next update requests in a list
> >   * @func: actual update function to call after the grace period.
> >+ *
> >+ * The struct is aligned to size of pointer. On most architectures it happens
> >+ * naturally due ABI requirements, but some architectures (like CRIS) have
> >+ * weird ABI and we need to ask it explicitly.
> >+ *
> >+ * The alignment is required to guarantee that bits 0 and 1 of @next will be
> >+ * clear under normal conditions -- as long as we use call_rcu(),
> >+ * call_rcu_bh(), call_rcu_sched(), or call_srcu() to queue callback.
> >+ *
> >+ * This guarantee is important for few reasons:
> >+ *  - future call_rcu_lazy() will make use of lower bits in the pointer;
> >+ *  - the structure shares storage spacer in struct page with @compound_head,
> >+ *    which encode PageTail() in bit 0. The guarantee is needed to avoid
> >+ *    false-positive PageTail().
> >   */
> >  struct callback_head {
> >  	struct callback_head *next;
> >  	void (*func)(struct callback_head *head);
> >-};
> >+} __attribute__((aligned(sizeof(void *))));
> >  #define rcu_head callback_head
> >
> >  typedef void (*rcu_callback_t)(struct rcu_head *head);
> >
> 

--
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]

#1230430

> From: "Kirill A. Shutemov" <kirill@shutemov.name>
> Date: Tue, 22 Sep 2015 17:31:04 +0200

> On Tue, Sep 22, 2015 at 08:18:35AM -0700, Paul E. McKenney wrote:
> > On Tue, Sep 22, 2015 at 03:57:06PM +0200, Hans-Peter Nilsson wrote:
> > > I guess you hit the right spot, but I'd think people would be
> > > more comfortable with aligning to sizeof (void *).
> > 
> > I would indeed prefer sizeof(void *).
> 
> Do you prefer to have the attribute set for whole structure or for ->next?
> I think attribute on ->next is more appropriate from documentation POV.

Speaking of the documentation POV, I'd recommend adding an
explanatory comment.  Here's hoping this was obvious! ;)

brgds, H-P
--
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]

#1230448

On Tue, Sep 22, 2015 at 06:16:18PM +0200, Hans-Peter Nilsson wrote:
> > From: "Kirill A. Shutemov" <kirill@shutemov.name>
> > Date: Tue, 22 Sep 2015 17:31:04 +0200
> 
> > On Tue, Sep 22, 2015 at 08:18:35AM -0700, Paul E. McKenney wrote:
> > > On Tue, Sep 22, 2015 at 03:57:06PM +0200, Hans-Peter Nilsson wrote:
> > > > I guess you hit the right spot, but I'd think people would be
> > > > more comfortable with aligning to sizeof (void *).
> > > 
> > > I would indeed prefer sizeof(void *).
> > 
> > Do you prefer to have the attribute set for whole structure or for ->next?
> > I think attribute on ->next is more appropriate from documentation POV.
> 
> Speaking of the documentation POV, I'd recommend adding an
> explanatory comment.  Here's hoping this was obvious! ;)

What Hans-Peter said!  ;-)

							Thanx, Paul

--
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