[0/2] locking/qspinlock: Break qspinlock_types.h header loop
mbox series

Message ID 20200729122807.GA7047@gondor.apana.org.au
Headers show
Series
  • locking/qspinlock: Break qspinlock_types.h header loop
Related show

Message

Herbert Xu July 29, 2020, 12:28 p.m. UTC
This miniseries breaks a header loop involving qspinlock_types.h.
The issue is that qspinlock_types.h includes atomic.h, which then
eventually includes kernel.h which could lead back to the original
file via spinlock_types.h.

The first patch moves ATOMIC_INIT into linux/types.h while the second
patch actuallys breaks the loop by no longer including atomic.h
in qspinlock_types.h.

Cheers,

Comments

Peter Zijlstra July 29, 2020, 12:47 p.m. UTC | #1
On Wed, Jul 29, 2020 at 10:28:07PM +1000, Herbert Xu wrote:
> This miniseries breaks a header loop involving qspinlock_types.h.
> The issue is that qspinlock_types.h includes atomic.h, which then
> eventually includes kernel.h which could lead back to the original
> file via spinlock_types.h.

How did you run into this, I haven't seen any build failures due to
this.

> The first patch moves ATOMIC_INIT into linux/types.h while the second
> patch actuallys breaks the loop by no longer including atomic.h
> in qspinlock_types.h.

Anyway, the patches look sane enough, I'll go stick them in
tip/locking/core or somesuch.
Herbert Xu July 29, 2020, 12:51 p.m. UTC | #2
On Wed, Jul 29, 2020 at 02:47:44PM +0200, peterz@infradead.org wrote:
>
> How did you run into this, I haven't seen any build failures due to
> this.

I didn't, Stephen ran into it after merging the printk tree together
with the powerpc tree:

https://lore.kernel.org/lkml/20200729210311.425d0e9b@canb.auug.org.au/

> Anyway, the patches look sane enough, I'll go stick them in
> tip/locking/core or somesuch.

Perhaps add them on top of the other two patches in locking/header?

Thanks,
Peter Zijlstra July 29, 2020, 1 p.m. UTC | #3
On Wed, Jul 29, 2020 at 10:51:44PM +1000, Herbert Xu wrote:
> On Wed, Jul 29, 2020 at 02:47:44PM +0200, peterz@infradead.org wrote:
> >
> > How did you run into this, I haven't seen any build failures due to
> > this.
> 
> I didn't, Stephen ran into it after merging the printk tree together
> with the powerpc tree:
> 
> https://lore.kernel.org/lkml/20200729210311.425d0e9b@canb.auug.org.au/
> 
> > Anyway, the patches look sane enough, I'll go stick them in
> > tip/locking/core or somesuch.
> 
> Perhaps add them on top of the other two patches in locking/header?

Can do,
Sergey Senozhatsky July 29, 2020, 1:28 p.m. UTC | #4
On (20/07/29 15:00), peterz@infradead.org wrote:
> On Wed, Jul 29, 2020 at 10:51:44PM +1000, Herbert Xu wrote:
> > On Wed, Jul 29, 2020 at 02:47:44PM +0200, peterz@infradead.org wrote:
> > >
[..]
> > > Anyway, the patches look sane enough, I'll go stick them in
> > > tip/locking/core or somesuch.
> > 
> > Perhaps add them on top of the other two patches in locking/header?
> 
> Can do,

locking/header would be better

	-ss
Waiman Long July 29, 2020, 1:35 p.m. UTC | #5
On 7/29/20 8:28 AM, Herbert Xu wrote:
> This miniseries breaks a header loop involving qspinlock_types.h.
> The issue is that qspinlock_types.h includes atomic.h, which then
> eventually includes kernel.h which could lead back to the original
> file via spinlock_types.h.
>
> The first patch moves ATOMIC_INIT into linux/types.h while the second
> patch actuallys breaks the loop by no longer including atomic.h
> in qspinlock_types.h.
>
> Cheers,

This patch series looks good to me. I just wonder if we should also move 
ATOMIC64_INIT() to types.h for symmetry purpose. Anyway,

Acked-by: Waiman Long <longman@redhat.com>
Peter Zijlstra July 29, 2020, 2:28 p.m. UTC | #6
On Wed, Jul 29, 2020 at 10:28:49PM +0900, Sergey Senozhatsky wrote:
> On (20/07/29 15:00), peterz@infradead.org wrote:
> > On Wed, Jul 29, 2020 at 10:51:44PM +1000, Herbert Xu wrote:
> > > On Wed, Jul 29, 2020 at 02:47:44PM +0200, peterz@infradead.org wrote:
> > > >
> [..]
> > > > Anyway, the patches look sane enough, I'll go stick them in
> > > > tip/locking/core or somesuch.
> > > 
> > > Perhaps add them on top of the other two patches in locking/header?
> > 
> > Can do,
> 
> locking/header would be better

Done.
Andy Shevchenko July 29, 2020, 3:04 p.m. UTC | #7
On Wed, Jul 29, 2020 at 4:35 PM Waiman Long <longman@redhat.com> wrote:
> On 7/29/20 8:28 AM, Herbert Xu wrote:

...

> This patch series looks good to me. I just wonder if we should also move
> ATOMIC64_INIT() to types.h for symmetry purpose. Anyway,

Same question here.
Sergey Senozhatsky July 29, 2020, 4:41 p.m. UTC | #8
On (20/07/29 16:28), peterz@infradead.org wrote:
> On Wed, Jul 29, 2020 at 10:28:49PM +0900, Sergey Senozhatsky wrote:
> > On (20/07/29 15:00), peterz@infradead.org wrote:
> > > On Wed, Jul 29, 2020 at 10:51:44PM +1000, Herbert Xu wrote:
> > > > On Wed, Jul 29, 2020 at 02:47:44PM +0200, peterz@infradead.org wrote:
> > > > >
> > [..]
> > > > > Anyway, the patches look sane enough, I'll go stick them in
> > > > > tip/locking/core or somesuch.
> > > > 
> > > > Perhaps add them on top of the other two patches in locking/header?
> > > 
> > > Can do,
> > 
> > locking/header would be better
> 
> Done.

Thanks a lot!
I'll run some cross-compilation tests.

	-ss
Sergey Senozhatsky July 29, 2020, 5:50 p.m. UTC | #9
On (20/07/29 22:28), Herbert Xu wrote:
> This miniseries breaks a header loop involving qspinlock_types.h.
> The issue is that qspinlock_types.h includes atomic.h, which then
> eventually includes kernel.h which could lead back to the original
> file via spinlock_types.h.
> 
> The first patch moves ATOMIC_INIT into linux/types.h while the second
> patch actuallys breaks the loop by no longer including atomic.h
> in qspinlock_types.h.

Thanks for staying on this.

I pulled locking/header, run some cross-compilation tests locally
(powerpc, arm, arm64, x86) and pushed printk/for-next. Let's see.

	-ss
Herbert Xu July 30, 2020, 12:59 a.m. UTC | #10
On Wed, Jul 29, 2020 at 06:04:57PM +0300, Andy Shevchenko wrote:
> On Wed, Jul 29, 2020 at 4:35 PM Waiman Long <longman@redhat.com> wrote:
> > On 7/29/20 8:28 AM, Herbert Xu wrote:
> 
> ...
> 
> > This patch series looks good to me. I just wonder if we should also move
> > ATOMIC64_INIT() to types.h for symmetry purpose. Anyway,
> 
> Same question here.

Yes I almost started doing it but at least one architecture (arc)
had a custom atomic64_t so I kept it out just to be on the safe
side.

We certainly could do this as a follow-up patch.

Cheers,
Andy Shevchenko July 30, 2020, 7:47 a.m. UTC | #11
On Thu, Jul 30, 2020 at 4:00 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Wed, Jul 29, 2020 at 06:04:57PM +0300, Andy Shevchenko wrote:
> > On Wed, Jul 29, 2020 at 4:35 PM Waiman Long <longman@redhat.com> wrote:
> > > On 7/29/20 8:28 AM, Herbert Xu wrote:
> >
> > ...
> >
> > > This patch series looks good to me. I just wonder if we should also move
> > > ATOMIC64_INIT() to types.h for symmetry purpose. Anyway,
> >
> > Same question here.
>
> Yes I almost started doing it but at least one architecture (arc)
> had a custom atomic64_t so I kept it out just to be on the safe
> side.

We may ask Synopsys folks to look at this as well.
Vineet, any ideas if we may unify ATOMIC64_INIT() across the architectures?

> We certainly could do this as a follow-up patch.
Herbert Xu July 30, 2020, 7:50 a.m. UTC | #12
On Thu, Jul 30, 2020 at 10:47:16AM +0300, Andy Shevchenko wrote:
>
> We may ask Synopsys folks to look at this as well.
> Vineet, any ideas if we may unify ATOMIC64_INIT() across the architectures?

I don't think there is any technical difficulty.  The custom
atomic64_t simply adds an alignment requirement so the initialisor
remains the same.

I just didn't want to add more complexity to the existing patch.
As a follow-up patch it should be quite straightforward.

Thanks,
Andy Shevchenko July 30, 2020, 7:55 a.m. UTC | #13
On Thu, Jul 30, 2020 at 10:51 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Thu, Jul 30, 2020 at 10:47:16AM +0300, Andy Shevchenko wrote:
> >
> > We may ask Synopsys folks to look at this as well.
> > Vineet, any ideas if we may unify ATOMIC64_INIT() across the architectures?
>
> I don't think there is any technical difficulty.  The custom
> atomic64_t simply adds an alignment requirement so the initialisor
> remains the same.
>
> I just didn't want to add more complexity to the existing patch.
> As a follow-up patch it should be quite straightforward.

Ah, I see what you mean. I considered the follow up patch as well, I
thought there were some bigger impediments.
Vineet Gupta Aug. 6, 2020, 3:37 a.m. UTC | #14
On 7/30/20 12:50 AM, Herbert Xu wrote:
> On Thu, Jul 30, 2020 at 10:47:16AM +0300, Andy Shevchenko wrote:
>> We may ask Synopsys folks to look at this as well.
>> Vineet, any ideas if we may unify ATOMIC64_INIT() across the architectures?
> I don't think there is any technical difficulty.  The custom
> atomic64_t simply adds an alignment requirement so the initialisor
> remains the same.

Exactly so.

FWIW the alignment requirement is because ARC ABI allows 64-bit data to be 32-bit
aligned provided hardware deals fine with 4 byte aligned for the non-atomic
double-load/store LDD/STD instructions. The 64-bit alignement however is required
for atomic double load/store LLOCKD/SCONDD instructions hence the definition of
ARC atomic64_t

-Vineet