Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751694AbdH3KAP (ORCPT ); Wed, 30 Aug 2017 06:00:15 -0400 Received: from foss.arm.com ([217.140.101.70]:42034 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751330AbdH3KAP (ORCPT ); Wed, 30 Aug 2017 06:00:15 -0400 Date: Wed, 30 Aug 2017 10:58:54 +0100 From: Mark Rutland To: Yury Norov Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Will Deacon , Laura Abbott , Ard Biesheuvel , Catalin Marinas , James Morse Subject: Re: [RFC PATCH] arm64: move THREAD_* definitions to separated header Message-ID: <20170830095854.GA17353@leverpostej> References: <20170830092249.20638-1-ynorov@caviumnetworks.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20170830092249.20638-1-ynorov@caviumnetworks.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4076 Lines: 108 On Wed, Aug 30, 2017 at 12:22:49PM +0300, Yury Norov wrote: > Hi Mark, all. Hi Yury, > In patch 'dbc9344a68e506f19f8 ("arm64: clean up THREAD_* definitions")' > you move THREAD_* definitions from arch/arm64/include/asm/thread_info.h > to asm/memory.h. After that asm/thread_info.h starts to depend on > asm/memory.h. > > When I try to apply ilp32 series on top of it [1], it causes circular > dependencies like this one: > > In file included from ./arch/arm64/include/asm/memory.h:30:0, > from ./arch/arm64/include/asm/thread_info.h:30, > from ./include/linux/thread_bits.h:20, > from ./include/linux/thread_info.h:13, > from ./include/asm-generic/preempt.h:4, > from ./arch/arm64/include/generated/asm/preempt.h:1, > from ./include/linux/preempt.h:80, > from ./include/linux/rcupdate.h:40, > from ./include/linux/rculist.h:10, > from ./include/linux/pid.h:4, > from ./include/linux/sched.h:13, > from arch/arm64/kernel/asm-offsets.c:21: > ./arch/arm64/include/asm/is_compat.h: In function ‘is_a32_compat_task’: > ./arch/arm64/include/asm/is_compat.h:25:9: error: implicit declaration of function ‘test_thread_flag’ [-Werror=implicit-function-declaration] > return test_thread_flag(TIF_32BIT); > ^~~~~~~~~~~~~~~~ > > The problem is that asm/memory.h depends on asm/is_compat.h to define > TASK_SIZE, which in turn requires asm/thread_info.h. ... and include , giving a circular dependency. In other architectures, TASK_SIZE is defined in processor.h. Can we not move TASK_SIZE instead of THREAD_SIZE, given that TASK_SIZE is what causes the dependency? We'd need a new __ASSEMBLY__ guard, but otherwise it looks like that would solve the issue. > The most obvious solution for it is to create is_compat.c file and make > is_*_compat() real functions. The other option is to move THREAD_* > definitions to separated macro. I would prefer 2nd one because of following > reasons: > - TASK_SIZE macro is used many times in kernel, including hot paths; > - asm/memory.h is included widely, as well as asm/thread_info.h, and it's > better not to make them depend one from another; > - THREAD_SIZE etc are not memory-related definitions. I disagree with this last point. THREAD_SIZE is in because it affects the kernel's memory layout, as with other definitions at the top of . I would much prefer keeping those definitions together. > In this patch THREAD_* definitions moved to separated asm/thread_size.h > header. It's enough to resolve dependency above. > > If you find this approach useful, I can prepare other patch that moves > TASK_* definitions from asm/memory.h to new header to remove the dependency > from asm/is_compat.h. I'd prefer that we moved the TASK_SIZE* definitions first, but if necessary, I'm ok with seeing the THREAD_SIZE* definitions move. [...] > diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h > -#include > +#include > #include > #include Nit: please keep includes ordered alphabetically. [...] > diff --git a/arch/arm64/include/asm/thread_size.h b/arch/arm64/include/asm/thread_size.h > +#ifdef __KERNEL__ This can go. We haven't needed __KERNEL__ ifdefs for a long time now... [...] > diff --git a/arch/arm64/kernel/hibernate-asm.S b/arch/arm64/kernel/hibernate-asm.S > index e56d848b6466..46b91702ea26 100644 > --- a/arch/arm64/kernel/hibernate-asm.S > +++ b/arch/arm64/kernel/hibernate-asm.S > @@ -22,7 +22,6 @@ > #include > #include > #include > -#include > #include > #include AFAICT, we can also lose , , and . Changes to this file might be better as a preparatory cleanup. Thanks, Mark.