linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH v1 1/9] timer: fix circular header dependency
       [not found] <cover.1537802981.git.christophe.leroy@c-s.fr>
@ 2018-09-24 15:52 ` Christophe Leroy
  2018-09-25  5:34   ` Christophe LEROY
  0 siblings, 1 reply; 6+ messages in thread
From: Christophe Leroy @ 2018-09-24 15:52 UTC (permalink / raw)
  To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
	aneesh.kumar, npiggin, Thomas Gleixner, Alexander Viro,
	John Stultz, Stephen Boyd
  Cc: linux-kernel, linuxppc-dev, linux-fsdevel

When switching powerpc to CONFIG_THREAD_INFO_IN_TASK, include/sched.h
has to be included in asm/smp.h for the following change, in order
to avoid uncomplete definition of task_struct:

-#define raw_smp_processor_id() (current_thread_info()->cpu)
+#define raw_smp_processor_id() (current->cpu)

But this generates the following compilation error, due to circular
header dependency.

  CC      kernel/time/alarmtimer.o
In file included from ./arch/powerpc/include/asm/smp.h:31,
                 from ./include/linux/smp.h:64,
                 from ./include/linux/percpu.h:7,
                 from ./include/linux/hrtimer.h:22,
                 from kernel/time/alarmtimer.c:19:
./include/linux/sched.h:558:19: error: field 'dl_timer' has incomplete type
  struct hrtimer   dl_timer;
                   ^~~~~~~~
./include/linux/sched.h:567:17: error: field 'inactive_timer' has incomplete type
  struct hrtimer inactive_timer;
                 ^~~~~~~~~~~~~~
make[1]: *** [kernel/time/alarmtimer.o] Error 1
make: *** [kernel/time/alarmtimer.o] Error 2

  CC      fs/timerfd.o
In file included from ./arch/powerpc/include/asm/smp.h:31,
                 from ./include/linux/smp.h:64,
                 from ./include/linux/percpu.h:7,
                 from ./include/linux/hrtimer.h:22,
                 from ./include/linux/alarmtimer.h:6,
                 from fs/timerfd.c:12:
./include/linux/sched.h:558:19: error: field 'dl_timer' has incomplete type
  struct hrtimer   dl_timer;
                   ^~~~~~~~
./include/linux/sched.h:567:17: error: field 'inactive_timer' has incomplete type
  struct hrtimer inactive_timer;
                 ^~~~~~~~~~~~~~
make[1]: *** [fs/timerfd.o] Error 1
make: *** [fs/timerfd.o] Error 2

This patch fixes it by including linux/hrtimer.h after linux/sched.h

Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 Should it be fixed in powerpc instead ? In that case, how ? Any idea ?

 fs/timerfd.c             | 2 +-
 kernel/time/alarmtimer.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/timerfd.c b/fs/timerfd.c
index d69ad801eb80..0fc01b241382 100644
--- a/fs/timerfd.c
+++ b/fs/timerfd.c
@@ -9,12 +9,12 @@
  *
  */
 
-#include <linux/alarmtimer.h>
 #include <linux/file.h>
 #include <linux/poll.h>
 #include <linux/init.h>
 #include <linux/fs.h>
 #include <linux/sched.h>
+#include <linux/alarmtimer.h>
 #include <linux/kernel.h>
 #include <linux/slab.h>
 #include <linux/list.h>
diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
index fa5de5e8de61..5fb75c9b3f06 100644
--- a/kernel/time/alarmtimer.c
+++ b/kernel/time/alarmtimer.c
@@ -16,11 +16,11 @@
  * published by the Free Software Foundation.
  */
 #include <linux/time.h>
-#include <linux/hrtimer.h>
 #include <linux/timerqueue.h>
 #include <linux/rtc.h>
 #include <linux/sched/signal.h>
 #include <linux/sched/debug.h>
+#include <linux/hrtimer.h>
 #include <linux/alarmtimer.h>
 #include <linux/mutex.h>
 #include <linux/platform_device.h>
-- 
2.13.3

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC PATCH v1 1/9] timer: fix circular header dependency
  2018-09-24 15:52 ` [RFC PATCH v1 1/9] timer: fix circular header dependency Christophe Leroy
@ 2018-09-25  5:34   ` Christophe LEROY
  2018-09-25  6:33     ` Christophe LEROY
  2018-09-25 12:11     ` Mark Rutland
  0 siblings, 2 replies; 6+ messages in thread
From: Christophe LEROY @ 2018-09-25  5:34 UTC (permalink / raw)
  To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
	aneesh.kumar, npiggin, Thomas Gleixner, Alexander Viro,
	John Stultz, Stephen Boyd
  Cc: linux-fsdevel, linuxppc-dev, linux-kernel



Le 24/09/2018 à 17:52, Christophe Leroy a écrit :
> When switching powerpc to CONFIG_THREAD_INFO_IN_TASK, include/sched.h
> has to be included in asm/smp.h for the following change, in order
> to avoid uncomplete definition of task_struct:
> 
> -#define raw_smp_processor_id() (current_thread_info()->cpu)
> +#define raw_smp_processor_id() (current->cpu)
> 
> But this generates the following compilation error, due to circular
> header dependency.
> 
>    CC      kernel/time/alarmtimer.o
> In file included from ./arch/powerpc/include/asm/smp.h:31,
>                   from ./include/linux/smp.h:64,
>                   from ./include/linux/percpu.h:7,
>                   from ./include/linux/hrtimer.h:22,
>                   from kernel/time/alarmtimer.c:19:
> ./include/linux/sched.h:558:19: error: field 'dl_timer' has incomplete type
>    struct hrtimer   dl_timer;
>                     ^~~~~~~~
> ./include/linux/sched.h:567:17: error: field 'inactive_timer' has incomplete type
>    struct hrtimer inactive_timer;
>                   ^~~~~~~~~~~~~~
> make[1]: *** [kernel/time/alarmtimer.o] Error 1
> make: *** [kernel/time/alarmtimer.o] Error 2
> 
>    CC      fs/timerfd.o
> In file included from ./arch/powerpc/include/asm/smp.h:31,
>                   from ./include/linux/smp.h:64,
>                   from ./include/linux/percpu.h:7,
>                   from ./include/linux/hrtimer.h:22,
>                   from ./include/linux/alarmtimer.h:6,
>                   from fs/timerfd.c:12:
> ./include/linux/sched.h:558:19: error: field 'dl_timer' has incomplete type
>    struct hrtimer   dl_timer;
>                     ^~~~~~~~
> ./include/linux/sched.h:567:17: error: field 'inactive_timer' has incomplete type
>    struct hrtimer inactive_timer;
>                   ^~~~~~~~~~~~~~
> make[1]: *** [fs/timerfd.o] Error 1
> make: *** [fs/timerfd.o] Error 2
> 
> This patch fixes it by including linux/hrtimer.h after linux/sched.h
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
>   Should it be fixed in powerpc instead ? In that case, how ? Any idea ?


Looks like there are several other places where the problem occurs, so 
it has to be fixed in the powerpc headers instead.

Seems like RISC arch faced the same issue, and fixed it the following way:

/*
  * This is particularly ugly: it appears we can't actually get the 
definition
  * of task_struct here, but we need access to the CPU this task is 
running on.
  * Instead of using C we're using asm-offsets.h to get the current 
processor
  * ID.
  */
#define raw_smp_processor_id() (*((int*)((char*)get_current() + 
TASK_TI_CPU)))


Unless someone has a better idea, I'll fixed it that way.

Christophe


> 
>   fs/timerfd.c             | 2 +-
>   kernel/time/alarmtimer.c | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/timerfd.c b/fs/timerfd.c
> index d69ad801eb80..0fc01b241382 100644
> --- a/fs/timerfd.c
> +++ b/fs/timerfd.c
> @@ -9,12 +9,12 @@
>    *
>    */
>   
> -#include <linux/alarmtimer.h>
>   #include <linux/file.h>
>   #include <linux/poll.h>
>   #include <linux/init.h>
>   #include <linux/fs.h>
>   #include <linux/sched.h>
> +#include <linux/alarmtimer.h>
>   #include <linux/kernel.h>
>   #include <linux/slab.h>
>   #include <linux/list.h>
> diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
> index fa5de5e8de61..5fb75c9b3f06 100644
> --- a/kernel/time/alarmtimer.c
> +++ b/kernel/time/alarmtimer.c
> @@ -16,11 +16,11 @@
>    * published by the Free Software Foundation.
>    */
>   #include <linux/time.h>
> -#include <linux/hrtimer.h>
>   #include <linux/timerqueue.h>
>   #include <linux/rtc.h>
>   #include <linux/sched/signal.h>
>   #include <linux/sched/debug.h>
> +#include <linux/hrtimer.h>
>   #include <linux/alarmtimer.h>
>   #include <linux/mutex.h>
>   #include <linux/platform_device.h>
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC PATCH v1 1/9] timer: fix circular header dependency
  2018-09-25  5:34   ` Christophe LEROY
@ 2018-09-25  6:33     ` Christophe LEROY
  2018-09-25 12:11     ` Mark Rutland
  1 sibling, 0 replies; 6+ messages in thread
From: Christophe LEROY @ 2018-09-25  6:33 UTC (permalink / raw)
  To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
	aneesh.kumar, npiggin, Thomas Gleixner, Alexander Viro,
	John Stultz, Stephen Boyd
  Cc: linux-fsdevel, linuxppc-dev, linux-kernel



Le 25/09/2018 à 07:34, Christophe LEROY a écrit :
> 
> 
> Le 24/09/2018 à 17:52, Christophe Leroy a écrit :
>> When switching powerpc to CONFIG_THREAD_INFO_IN_TASK, include/sched.h
>> has to be included in asm/smp.h for the following change, in order
>> to avoid uncomplete definition of task_struct:
>>
>> -#define raw_smp_processor_id() (current_thread_info()->cpu)
>> +#define raw_smp_processor_id() (current->cpu)
>>
>> But this generates the following compilation error, due to circular
>> header dependency.
>>
>>    CC      kernel/time/alarmtimer.o
>> In file included from ./arch/powerpc/include/asm/smp.h:31,
>>                   from ./include/linux/smp.h:64,
>>                   from ./include/linux/percpu.h:7,
>>                   from ./include/linux/hrtimer.h:22,
>>                   from kernel/time/alarmtimer.c:19:
>> ./include/linux/sched.h:558:19: error: field 'dl_timer' has incomplete 
>> type
>>    struct hrtimer   dl_timer;
>>                     ^~~~~~~~
>> ./include/linux/sched.h:567:17: error: field 'inactive_timer' has 
>> incomplete type
>>    struct hrtimer inactive_timer;
>>                   ^~~~~~~~~~~~~~
>> make[1]: *** [kernel/time/alarmtimer.o] Error 1
>> make: *** [kernel/time/alarmtimer.o] Error 2
>>
>>    CC      fs/timerfd.o
>> In file included from ./arch/powerpc/include/asm/smp.h:31,
>>                   from ./include/linux/smp.h:64,
>>                   from ./include/linux/percpu.h:7,
>>                   from ./include/linux/hrtimer.h:22,
>>                   from ./include/linux/alarmtimer.h:6,
>>                   from fs/timerfd.c:12:
>> ./include/linux/sched.h:558:19: error: field 'dl_timer' has incomplete 
>> type
>>    struct hrtimer   dl_timer;
>>                     ^~~~~~~~
>> ./include/linux/sched.h:567:17: error: field 'inactive_timer' has 
>> incomplete type
>>    struct hrtimer inactive_timer;
>>                   ^~~~~~~~~~~~~~
>> make[1]: *** [fs/timerfd.o] Error 1
>> make: *** [fs/timerfd.o] Error 2
>>
>> This patch fixes it by including linux/hrtimer.h after linux/sched.h
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>> ---
>>   Should it be fixed in powerpc instead ? In that case, how ? Any idea ?
> 
> 
> Looks like there are several other places where the problem occurs, so 
> it has to be fixed in the powerpc headers instead.
> 
> Seems like RISC arch faced the same issue, and fixed it the following way:
> 
> /*
>   * This is particularly ugly: it appears we can't actually get the 
> definition
>   * of task_struct here, but we need access to the CPU this task is 
> running on.
>   * Instead of using C we're using asm-offsets.h to get the current 
> processor
>   * ID.
>   */
> #define raw_smp_processor_id() (*((int*)((char*)get_current() + 
> TASK_TI_CPU)))
> 
> 
> Unless someone has a better idea, I'll fixed it that way.


Not easy. I'm stuck at the moment, because asm-offsets.h redefines a lot 
of things with the same name as in C, so when including asm-offsets.h in 
C it conflicts. Argh !. Need to fix all those duplicates ?

Christophe

> 
> Christophe
> 
> 
>>
>>   fs/timerfd.c             | 2 +-
>>   kernel/time/alarmtimer.c | 2 +-
>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/timerfd.c b/fs/timerfd.c
>> index d69ad801eb80..0fc01b241382 100644
>> --- a/fs/timerfd.c
>> +++ b/fs/timerfd.c
>> @@ -9,12 +9,12 @@
>>    *
>>    */
>> -#include <linux/alarmtimer.h>
>>   #include <linux/file.h>
>>   #include <linux/poll.h>
>>   #include <linux/init.h>
>>   #include <linux/fs.h>
>>   #include <linux/sched.h>
>> +#include <linux/alarmtimer.h>
>>   #include <linux/kernel.h>
>>   #include <linux/slab.h>
>>   #include <linux/list.h>
>> diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
>> index fa5de5e8de61..5fb75c9b3f06 100644
>> --- a/kernel/time/alarmtimer.c
>> +++ b/kernel/time/alarmtimer.c
>> @@ -16,11 +16,11 @@
>>    * published by the Free Software Foundation.
>>    */
>>   #include <linux/time.h>
>> -#include <linux/hrtimer.h>
>>   #include <linux/timerqueue.h>
>>   #include <linux/rtc.h>
>>   #include <linux/sched/signal.h>
>>   #include <linux/sched/debug.h>
>> +#include <linux/hrtimer.h>
>>   #include <linux/alarmtimer.h>
>>   #include <linux/mutex.h>
>>   #include <linux/platform_device.h>
>>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC PATCH v1 1/9] timer: fix circular header dependency
  2018-09-25  5:34   ` Christophe LEROY
  2018-09-25  6:33     ` Christophe LEROY
@ 2018-09-25 12:11     ` Mark Rutland
  2018-09-25 12:15       ` Christophe LEROY
  1 sibling, 1 reply; 6+ messages in thread
From: Mark Rutland @ 2018-09-25 12:11 UTC (permalink / raw)
  To: Christophe LEROY
  Cc: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
	aneesh.kumar, npiggin, Thomas Gleixner, Alexander Viro,
	John Stultz, Stephen Boyd, linux-fsdevel, linuxppc-dev,
	linux-kernel

On Tue, Sep 25, 2018 at 07:34:15AM +0200, Christophe LEROY wrote:
> 
> 
> Le 24/09/2018 à 17:52, Christophe Leroy a écrit :
> > When switching powerpc to CONFIG_THREAD_INFO_IN_TASK, include/sched.h
> > has to be included in asm/smp.h for the following change, in order
> > to avoid uncomplete definition of task_struct:
> > 
> > -#define raw_smp_processor_id() (current_thread_info()->cpu)
> > +#define raw_smp_processor_id() (current->cpu)
> > 
> > But this generates the following compilation error, due to circular
> > header dependency.
> > 
> >    CC      kernel/time/alarmtimer.o
> > In file included from ./arch/powerpc/include/asm/smp.h:31,
> >                   from ./include/linux/smp.h:64,
> >                   from ./include/linux/percpu.h:7,
> >                   from ./include/linux/hrtimer.h:22,
> >                   from kernel/time/alarmtimer.c:19:
> > ./include/linux/sched.h:558:19: error: field 'dl_timer' has incomplete type
> >    struct hrtimer   dl_timer;
> >                     ^~~~~~~~
> > ./include/linux/sched.h:567:17: error: field 'inactive_timer' has incomplete type
> >    struct hrtimer inactive_timer;
> >                   ^~~~~~~~~~~~~~
> > make[1]: *** [kernel/time/alarmtimer.o] Error 1
> > make: *** [kernel/time/alarmtimer.o] Error 2
> > 
> >    CC      fs/timerfd.o
> > In file included from ./arch/powerpc/include/asm/smp.h:31,
> >                   from ./include/linux/smp.h:64,
> >                   from ./include/linux/percpu.h:7,
> >                   from ./include/linux/hrtimer.h:22,
> >                   from ./include/linux/alarmtimer.h:6,
> >                   from fs/timerfd.c:12:
> > ./include/linux/sched.h:558:19: error: field 'dl_timer' has incomplete type
> >    struct hrtimer   dl_timer;
> >                     ^~~~~~~~
> > ./include/linux/sched.h:567:17: error: field 'inactive_timer' has incomplete type
> >    struct hrtimer inactive_timer;
> >                   ^~~~~~~~~~~~~~
> > make[1]: *** [fs/timerfd.o] Error 1
> > make: *** [fs/timerfd.o] Error 2
> > 
> > This patch fixes it by including linux/hrtimer.h after linux/sched.h
> > 
> > Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> > ---
> >   Should it be fixed in powerpc instead ? In that case, how ? Any idea ?
> 
> 
> Looks like there are several other places where the problem occurs, so it
> has to be fixed in the powerpc headers instead.
> 
> Seems like RISC arch faced the same issue, and fixed it the following way:
> 
> /*
>  * This is particularly ugly: it appears we can't actually get the
> definition
>  * of task_struct here, but we need access to the CPU this task is running
> on.
>  * Instead of using C we're using asm-offsets.h to get the current processor
>  * ID.
>  */
> #define raw_smp_processor_id() (*((int*)((char*)get_current() +
> TASK_TI_CPU)))
> 
> 
> Unless someone has a better idea, I'll fixed it that way.

To solve this on arm64 we made the cpu number a percpu variable; see
commit:

  57c82954e77fa12c ("arm64: make cpu number a percpu variable")

IIUC, you could do that by placing it in paca_struct.

Thanks,
Mark.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC PATCH v1 1/9] timer: fix circular header dependency
  2018-09-25 12:11     ` Mark Rutland
@ 2018-09-25 12:15       ` Christophe LEROY
  2018-09-25 17:48         ` Christophe Leroy
  0 siblings, 1 reply; 6+ messages in thread
From: Christophe LEROY @ 2018-09-25 12:15 UTC (permalink / raw)
  To: Mark Rutland
  Cc: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
	aneesh.kumar, npiggin, Thomas Gleixner, Alexander Viro,
	John Stultz, Stephen Boyd, linux-fsdevel, linuxppc-dev,
	linux-kernel



Le 25/09/2018 à 14:11, Mark Rutland a écrit :
> On Tue, Sep 25, 2018 at 07:34:15AM +0200, Christophe LEROY wrote:
>>
>>
>> Le 24/09/2018 à 17:52, Christophe Leroy a écrit :
>>> When switching powerpc to CONFIG_THREAD_INFO_IN_TASK, include/sched.h
>>> has to be included in asm/smp.h for the following change, in order
>>> to avoid uncomplete definition of task_struct:
>>>
>>> -#define raw_smp_processor_id() (current_thread_info()->cpu)
>>> +#define raw_smp_processor_id() (current->cpu)
>>>
>>> But this generates the following compilation error, due to circular
>>> header dependency.
>>>
>>>     CC      kernel/time/alarmtimer.o
>>> In file included from ./arch/powerpc/include/asm/smp.h:31,
>>>                    from ./include/linux/smp.h:64,
>>>                    from ./include/linux/percpu.h:7,
>>>                    from ./include/linux/hrtimer.h:22,
>>>                    from kernel/time/alarmtimer.c:19:
>>> ./include/linux/sched.h:558:19: error: field 'dl_timer' has incomplete type
>>>     struct hrtimer   dl_timer;
>>>                      ^~~~~~~~
>>> ./include/linux/sched.h:567:17: error: field 'inactive_timer' has incomplete type
>>>     struct hrtimer inactive_timer;
>>>                    ^~~~~~~~~~~~~~
>>> make[1]: *** [kernel/time/alarmtimer.o] Error 1
>>> make: *** [kernel/time/alarmtimer.o] Error 2
>>>
>>>     CC      fs/timerfd.o
>>> In file included from ./arch/powerpc/include/asm/smp.h:31,
>>>                    from ./include/linux/smp.h:64,
>>>                    from ./include/linux/percpu.h:7,
>>>                    from ./include/linux/hrtimer.h:22,
>>>                    from ./include/linux/alarmtimer.h:6,
>>>                    from fs/timerfd.c:12:
>>> ./include/linux/sched.h:558:19: error: field 'dl_timer' has incomplete type
>>>     struct hrtimer   dl_timer;
>>>                      ^~~~~~~~
>>> ./include/linux/sched.h:567:17: error: field 'inactive_timer' has incomplete type
>>>     struct hrtimer inactive_timer;
>>>                    ^~~~~~~~~~~~~~
>>> make[1]: *** [fs/timerfd.o] Error 1
>>> make: *** [fs/timerfd.o] Error 2
>>>
>>> This patch fixes it by including linux/hrtimer.h after linux/sched.h
>>>
>>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>>> ---
>>>    Should it be fixed in powerpc instead ? In that case, how ? Any idea ?
>>
>>
>> Looks like there are several other places where the problem occurs, so it
>> has to be fixed in the powerpc headers instead.
>>
>> Seems like RISC arch faced the same issue, and fixed it the following way:
>>
>> /*
>>   * This is particularly ugly: it appears we can't actually get the
>> definition
>>   * of task_struct here, but we need access to the CPU this task is running
>> on.
>>   * Instead of using C we're using asm-offsets.h to get the current processor
>>   * ID.
>>   */
>> #define raw_smp_processor_id() (*((int*)((char*)get_current() +
>> TASK_TI_CPU)))
>>
>>
>> Unless someone has a better idea, I'll fixed it that way.
> 
> To solve this on arm64 we made the cpu number a percpu variable; see
> commit:
> 
>    57c82954e77fa12c ("arm64: make cpu number a percpu variable")

Thanks, I'll have a look

> IIUC, you could do that by placing it in paca_struct.

By the way, it is already in PACA struct for PPC64.
The issue is with PPC32, up to now it was in current_thread_info:

#ifdef CONFIG_PPC64
#define raw_smp_processor_id()	(local_paca->paca_index)
#define hard_smp_processor_id() (get_paca()->hw_cpu_id)
#else
/* 32-bit */
extern int smp_hw_index[];

#define raw_smp_processor_id()	(current_thread_info()->cpu)
#define hard_smp_processor_id() 	(smp_hw_index[smp_processor_id()])

[..]

#endif


Christophe

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC PATCH v1 1/9] timer: fix circular header dependency
  2018-09-25 12:15       ` Christophe LEROY
@ 2018-09-25 17:48         ` Christophe Leroy
  0 siblings, 0 replies; 6+ messages in thread
From: Christophe Leroy @ 2018-09-25 17:48 UTC (permalink / raw)
  To: Mark Rutland
  Cc: Stephen Boyd, linux-kernel, npiggin, Paul Mackerras,
	aneesh.kumar, linux-fsdevel, John Stultz, Thomas Gleixner,
	linuxppc-dev, Alexander Viro



On 09/25/2018 12:15 PM, Christophe LEROY wrote:
> 
> 
> Le 25/09/2018 à 14:11, Mark Rutland a écrit :
>> On Tue, Sep 25, 2018 at 07:34:15AM +0200, Christophe LEROY wrote:
>>>
>>>
>>> Le 24/09/2018 à 17:52, Christophe Leroy a écrit :
>>>> When switching powerpc to CONFIG_THREAD_INFO_IN_TASK, include/sched.h
>>>> has to be included in asm/smp.h for the following change, in order
>>>> to avoid uncomplete definition of task_struct:
>>>>
>>>> -#define raw_smp_processor_id() (current_thread_info()->cpu)
>>>> +#define raw_smp_processor_id() (current->cpu)
>>>>
>>>> But this generates the following compilation error, due to circular
>>>> header dependency.
>>>>
>>>>     CC      kernel/time/alarmtimer.o
>>>> In file included from ./arch/powerpc/include/asm/smp.h:31,
>>>>                    from ./include/linux/smp.h:64,
>>>>                    from ./include/linux/percpu.h:7,
>>>>                    from ./include/linux/hrtimer.h:22,
>>>>                    from kernel/time/alarmtimer.c:19:
>>>> ./include/linux/sched.h:558:19: error: field 'dl_timer' has 
>>>> incomplete type
>>>>     struct hrtimer   dl_timer;
>>>>                      ^~~~~~~~
>>>> ./include/linux/sched.h:567:17: error: field 'inactive_timer' has 
>>>> incomplete type
>>>>     struct hrtimer inactive_timer;
>>>>                    ^~~~~~~~~~~~~~
>>>> make[1]: *** [kernel/time/alarmtimer.o] Error 1
>>>> make: *** [kernel/time/alarmtimer.o] Error 2
>>>>
>>>>     CC      fs/timerfd.o
>>>> In file included from ./arch/powerpc/include/asm/smp.h:31,
>>>>                    from ./include/linux/smp.h:64,
>>>>                    from ./include/linux/percpu.h:7,
>>>>                    from ./include/linux/hrtimer.h:22,
>>>>                    from ./include/linux/alarmtimer.h:6,
>>>>                    from fs/timerfd.c:12:
>>>> ./include/linux/sched.h:558:19: error: field 'dl_timer' has 
>>>> incomplete type
>>>>     struct hrtimer   dl_timer;
>>>>                      ^~~~~~~~
>>>> ./include/linux/sched.h:567:17: error: field 'inactive_timer' has 
>>>> incomplete type
>>>>     struct hrtimer inactive_timer;
>>>>                    ^~~~~~~~~~~~~~
>>>> make[1]: *** [fs/timerfd.o] Error 1
>>>> make: *** [fs/timerfd.o] Error 2
>>>>
>>>> This patch fixes it by including linux/hrtimer.h after linux/sched.h
>>>>
>>>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>>>> ---
>>>>    Should it be fixed in powerpc instead ? In that case, how ? Any 
>>>> idea ?
>>>
>>>
>>> Looks like there are several other places where the problem occurs, 
>>> so it
>>> has to be fixed in the powerpc headers instead.
>>>
>>> Seems like RISC arch faced the same issue, and fixed it the following 
>>> way:
>>>
>>> /*
>>>   * This is particularly ugly: it appears we can't actually get the
>>> definition
>>>   * of task_struct here, but we need access to the CPU this task is 
>>> running
>>> on.
>>>   * Instead of using C we're using asm-offsets.h to get the current 
>>> processor
>>>   * ID.
>>>   */
>>> #define raw_smp_processor_id() (*((int*)((char*)get_current() +
>>> TASK_TI_CPU)))
>>>
>>>
>>> Unless someone has a better idea, I'll fixed it that way.
>>
>> To solve this on arm64 we made the cpu number a percpu variable; see
>> commit:
>>
>>    57c82954e77fa12c ("arm64: make cpu number a percpu variable")
> 
> Thanks, I'll have a look
> 
>> IIUC, you could do that by placing it in paca_struct.
> 
> By the way, it is already in PACA struct for PPC64.
> The issue is with PPC32, up to now it was in current_thread_info:
> 
> #ifdef CONFIG_PPC64
> #define raw_smp_processor_id()    (local_paca->paca_index)
> #define hard_smp_processor_id() (get_paca()->hw_cpu_id)
> #else
> /* 32-bit */
> extern int smp_hw_index[];
> 
> #define raw_smp_processor_id()    (current_thread_info()->cpu)
> #define hard_smp_processor_id()     (smp_hw_index[smp_processor_id()])
> 
> [..]
> 
> #endif
> 

It seems not that easy. Adding the following leads to the below failure. 
I'm having a hard time identifying and fixing the issue. Looks again 
like a circular thing, because linux/percpu-defs.h is where 
raw_cpu_ptr() is defined.

--- a/arch/powerpc/include/asm/smp.h
+++ b/arch/powerpc/include/asm/smp.h
@@ -83,7 +83,14 @@ int is_cpu_dead(unsigned int cpu);
  /* 32-bit */
  extern int smp_hw_index[];

-#define raw_smp_processor_id() (current_thread_info()->cpu)
+DECLARE_PER_CPU_READ_MOSTLY(unsigned int, cpu_number);
+
+/*
+ * We don't use this_cpu_read(cpu_number) as that has implicit writes to
+ * preempt_count, and associated (compiler) barriers, that we'd like to 
avoid
+ * the expense of. If we're preemptible, the value can be stale at use 
anyway.
+ */
+#define raw_smp_processor_id()         (*this_cpu_ptr(&cpu_number))
  #define hard_smp_processor_id()        (smp_hw_index[smp_processor_id()])

  static inline int get_hard_smp_processor_id(int cpu)


   CC      arch/powerpc/kernel/asm-offsets.s
In file included from ././include/linux/compiler_types.h:64:0,
                  from <command-line>:0:
./include/linux/percpu-rwsem.h: In function 
'percpu_down_read_preempt_disable':
./include/linux/percpu-defs.h:254:27: error: implicit declaration of 
function 'raw_cpu_ptr' [-Werror=implicit-function-declaration]
  #define this_cpu_ptr(ptr) raw_cpu_ptr(ptr)
                            ^
./include/linux/compiler-gcc.h:58:26: note: in definition of macro 
'RELOC_HIDE'
   (typeof(ptr)) (__ptr + (off));     \
                           ^
./include/asm-generic/percpu.h:44:31: note: in expansion of macro 
'SHIFT_PERCPU_PTR'
  #define arch_raw_cpu_ptr(ptr) SHIFT_PERCPU_PTR(ptr, __my_cpu_offset)
                                ^
./include/asm-generic/percpu.h:31:25: note: in expansion of macro 
'per_cpu_offset'
  #define __my_cpu_offset per_cpu_offset(raw_smp_processor_id())
                          ^
./arch/powerpc/include/asm/smp.h:93:35: note: in expansion of macro 
'this_cpu_ptr'
  #define raw_smp_processor_id()  (*this_cpu_ptr(&cpu_number))
                                    ^
./include/asm-generic/percpu.h:31:40: note: in expansion of macro 
'raw_smp_processor_id'
  #define __my_cpu_offset per_cpu_offset(raw_smp_processor_id())
                                         ^
./include/asm-generic/percpu.h:44:53: note: in expansion of macro 
'__my_cpu_offset'
  #define arch_raw_cpu_ptr(ptr) SHIFT_PERCPU_PTR(ptr, __my_cpu_offset)
                                                      ^
./include/linux/percpu-defs.h:244:2: note: in expansion of macro 
'arch_raw_cpu_ptr'
   arch_raw_cpu_ptr(ptr);      \
   ^
./include/asm-generic/percpu.h:76:3: note: in expansion of macro 
'raw_cpu_ptr'
   *raw_cpu_ptr(&(pcp)) op val;     \
    ^
./include/asm-generic/percpu.h:225:34: note: in expansion of macro 
'raw_cpu_generic_to_op'
  #define raw_cpu_add_1(pcp, val)  raw_cpu_generic_to_op(pcp, val, +=)
                                   ^
./include/linux/percpu-defs.h:379:11: note: in expansion of macro 
'raw_cpu_add_1'
    case 1: stem##1(variable, __VA_ARGS__);break;  \
            ^
./include/linux/percpu-defs.h:424:32: note: in expansion of macro 
'__pcpu_size_call'
  #define raw_cpu_add(pcp, val)  __pcpu_size_call(raw_cpu_add_, pcp, val)
                                 ^
./include/linux/percpu-defs.h:460:2: note: in expansion of macro 
'raw_cpu_add'
   raw_cpu_add(pcp, val);      \
   ^
./include/linux/percpu-defs.h:499:30: note: in expansion of macro 
'__this_cpu_add'
  #define __this_cpu_inc(pcp)  __this_cpu_add(pcp, 1)
                               ^
./include/linux/percpu-rwsem.h:47:2: note: in expansion of macro 
'__this_cpu_inc'
   __this_cpu_inc(*sem->read_count);
   ^
./arch/powerpc/include/asm/smp.h:93:34: error: invalid type argument of 
unary '*' (have 'int')
  #define raw_smp_processor_id()  (*this_cpu_ptr(&cpu_number))
                                   ^


[...]

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2018-09-25 23:57 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <cover.1537802981.git.christophe.leroy@c-s.fr>
2018-09-24 15:52 ` [RFC PATCH v1 1/9] timer: fix circular header dependency Christophe Leroy
2018-09-25  5:34   ` Christophe LEROY
2018-09-25  6:33     ` Christophe LEROY
2018-09-25 12:11     ` Mark Rutland
2018-09-25 12:15       ` Christophe LEROY
2018-09-25 17:48         ` Christophe Leroy

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).