| Message ID | 20190726210854.6408-5-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/percpu: Cleanup | expand |
Hi Andrew, On 7/26/19 10:08 PM, Andrew Cooper wrote: > These macros are identical across the architectures, and shouldn't be separate > from the DEFINE_PER_CPU*() infrastructure. > > This converts the final asm/percpu.h includes, which were all using > DECLARE_PER_CPU(), to include xen/percpu.h instead. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Acked-by: Julien Grall <julien.grall@arm.com> Cheers,
On Fri, Jul 26, 2019 at 10:08:54PM +0100, Andrew Cooper wrote: > These macros are identical across the architectures, and shouldn't be separate > from the DEFINE_PER_CPU*() infrastructure. > > This converts the final asm/percpu.h includes, which were all using > DECLARE_PER_CPU(), to include xen/percpu.h instead. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks.
On 26.07.2019 23:08, Andrew Cooper wrote: > --- a/xen/include/xen/percpu.h > +++ b/xen/include/xen/percpu.h > @@ -3,6 +3,12 @@ > > #include <asm/percpu.h> > > +#define DECLARE_PER_CPU(type, name) \ > + extern __typeof__(type) per_cpu__ ## name > + > +#define __DEFINE_PER_CPU(attr, type, name) \ > + attr __typeof__(type) per_cpu_ ## name > + > /* > * Separate out the type, so (int[3], foo) works. > * By moving things here you render stale the remainder of the comment in context above: No per-arch symbol name prefix is going to be possible anymore. I'm not against it, but this comment would then want adjusting. What's not immediately clear to me is whether the two-stage concatenation of an underscore each is then still necessary. Jan
On 29/07/2019 14:00, Jan Beulich wrote: > On 26.07.2019 23:08, Andrew Cooper wrote: >> --- a/xen/include/xen/percpu.h >> +++ b/xen/include/xen/percpu.h >> @@ -3,6 +3,12 @@ >> >> #include <asm/percpu.h> >> >> +#define DECLARE_PER_CPU(type, name) \ >> + extern __typeof__(type) per_cpu__ ## name >> + >> +#define __DEFINE_PER_CPU(attr, type, name) \ >> + attr __typeof__(type) per_cpu_ ## name >> + >> /* >> * Separate out the type, so (int[3], foo) works. >> * > By moving things here you render stale the remainder of the > comment in context above: No per-arch symbol name prefix is going > to be possible anymore. I'm not against it, but this comment > would then want adjusting. What's not immediately clear to me is > whether the two-stage concatenation of an underscore each is then > still necessary. Yes it is still necessary. See the TSS thread for why. ~Andrew
On 29.07.2019 15:23, Andrew Cooper wrote: > On 29/07/2019 14:00, Jan Beulich wrote: >> On 26.07.2019 23:08, Andrew Cooper wrote: >>> --- a/xen/include/xen/percpu.h >>> +++ b/xen/include/xen/percpu.h >>> @@ -3,6 +3,12 @@ >>> >>> #include <asm/percpu.h> >>> >>> +#define DECLARE_PER_CPU(type, name) \ >>> + extern __typeof__(type) per_cpu__ ## name >>> + >>> +#define __DEFINE_PER_CPU(attr, type, name) \ >>> + attr __typeof__(type) per_cpu_ ## name >>> + >>> /* >>> * Separate out the type, so (int[3], foo) works. >>> * >> By moving things here you render stale the remainder of the >> comment in context above: No per-arch symbol name prefix is going >> to be possible anymore. I'm not against it, but this comment >> would then want adjusting. What's not immediately clear to me is >> whether the two-stage concatenation of an underscore each is then >> still necessary. > > Yes it is still necessary. See the TSS thread for why. No, that thread doesn't explain it. From an initial look I think two-stage expansion is still necessary, but it could then be _ ## name on the first and per_cpu ## name on the second (i.e. no double underscore in the middle anymore). Of course there may be reasons why we actually _want_ a double underscore there. Jan
On 29/07/2019 14:54, Jan Beulich wrote: > On 29.07.2019 15:23, Andrew Cooper wrote: >> On 29/07/2019 14:00, Jan Beulich wrote: >>> On 26.07.2019 23:08, Andrew Cooper wrote: >>>> --- a/xen/include/xen/percpu.h >>>> +++ b/xen/include/xen/percpu.h >>>> @@ -3,6 +3,12 @@ >>>> >>>> #include <asm/percpu.h> >>>> >>>> +#define DECLARE_PER_CPU(type, name) \ >>>> + extern __typeof__(type) per_cpu__ ## name >>>> + >>>> +#define __DEFINE_PER_CPU(attr, type, name) \ >>>> + attr __typeof__(type) per_cpu_ ## name >>>> + >>>> /* >>>> * Separate out the type, so (int[3], foo) works. >>>> * >>> By moving things here you render stale the remainder of the >>> comment in context above: No per-arch symbol name prefix is going >>> to be possible anymore. I'm not against it, but this comment >>> would then want adjusting. What's not immediately clear to me is >>> whether the two-stage concatenation of an underscore each is then >>> still necessary. >> Yes it is still necessary. See the TSS thread for why. > No, that thread doesn't explain it. From an initial look I think > two-stage expansion is still necessary It is about preventing 'name' being expanded, due to the mess with cpumask_scratch, which requires a ## at least at the top level. I personally think that fixing cpumask_scratch is the right way to go, but I specifically didn't touch that so as to avoid wreaking havoc with Juergen's core-scheduling series. > , but it could then be > _ ## name on the first and per_cpu ## name on the second (i.e. > no double underscore in the middle anymore). Hmm, probably, but... > Of course there may > be reasons why we actually _want_ a double underscore there. ... I don't have the effort or energy - but most importantly, time - to rewrite Xen from scratch. If there is a concrete reason why dropping the double underscore is good/necessary/other, then it should be present as its own patch, not crowbarred into an unrelated cleanup patch. Until then, the inertia of "because its already like this" wins. ~Andrew
On 29.07.2019 20:03, Andrew Cooper wrote: > On 29/07/2019 14:54, Jan Beulich wrote: >> On 29.07.2019 15:23, Andrew Cooper wrote: >>> On 29/07/2019 14:00, Jan Beulich wrote: >>>> On 26.07.2019 23:08, Andrew Cooper wrote: >>>>> --- a/xen/include/xen/percpu.h >>>>> +++ b/xen/include/xen/percpu.h >>>>> @@ -3,6 +3,12 @@ >>>>> >>>>> #include <asm/percpu.h> >>>>> >>>>> +#define DECLARE_PER_CPU(type, name) \ >>>>> + extern __typeof__(type) per_cpu__ ## name >>>>> + >>>>> +#define __DEFINE_PER_CPU(attr, type, name) \ >>>>> + attr __typeof__(type) per_cpu_ ## name >>>>> + >>>>> /* >>>>> * Separate out the type, so (int[3], foo) works. >>>>> * >>>> By moving things here you render stale the remainder of the >>>> comment in context above: No per-arch symbol name prefix is going >>>> to be possible anymore. I'm not against it, but this comment >>>> would then want adjusting. What's not immediately clear to me is >>>> whether the two-stage concatenation of an underscore each is then >>>> still necessary. >>> Yes it is still necessary. See the TSS thread for why. >> No, that thread doesn't explain it. From an initial look I think >> two-stage expansion is still necessary > > It is about preventing 'name' being expanded, due to the mess with > cpumask_scratch, which requires a ## at least at the top level. > > I personally think that fixing cpumask_scratch is the right way to go, > but I specifically didn't touch that so as to avoid wreaking havoc with > Juergen's core-scheduling series. > >> , but it could then be >> _ ## name on the first and per_cpu ## name on the second (i.e. >> no double underscore in the middle anymore). > > Hmm, probably, but... > >> Of course there may >> be reasons why we actually _want_ a double underscore there. > > ... I don't have the effort or energy - but most importantly, time - to > rewrite Xen from scratch. > > If there is a concrete reason why dropping the double underscore is > good/necessary/other, then it should be present as its own patch, not > crowbarred into an unrelated cleanup patch. > > Until then, the inertia of "because its already like this" wins. And this is all fine, as long as the comment in question continues to reflect reality after this change. Jan
On 29.07.19 20:03, Andrew Cooper wrote: > On 29/07/2019 14:54, Jan Beulich wrote: >> On 29.07.2019 15:23, Andrew Cooper wrote: >>> On 29/07/2019 14:00, Jan Beulich wrote: >>>> On 26.07.2019 23:08, Andrew Cooper wrote: >>>>> --- a/xen/include/xen/percpu.h >>>>> +++ b/xen/include/xen/percpu.h >>>>> @@ -3,6 +3,12 @@ >>>>> >>>>> #include <asm/percpu.h> >>>>> >>>>> +#define DECLARE_PER_CPU(type, name) \ >>>>> + extern __typeof__(type) per_cpu__ ## name >>>>> + >>>>> +#define __DEFINE_PER_CPU(attr, type, name) \ >>>>> + attr __typeof__(type) per_cpu_ ## name >>>>> + >>>>> /* >>>>> * Separate out the type, so (int[3], foo) works. >>>>> * >>>> By moving things here you render stale the remainder of the >>>> comment in context above: No per-arch symbol name prefix is going >>>> to be possible anymore. I'm not against it, but this comment >>>> would then want adjusting. What's not immediately clear to me is >>>> whether the two-stage concatenation of an underscore each is then >>>> still necessary. >>> Yes it is still necessary. See the TSS thread for why. >> No, that thread doesn't explain it. From an initial look I think >> two-stage expansion is still necessary > > It is about preventing 'name' being expanded, due to the mess with > cpumask_scratch, which requires a ## at least at the top level. > > I personally think that fixing cpumask_scratch is the right way to go, > but I specifically didn't touch that so as to avoid wreaking havoc with > Juergen's core-scheduling series. I appreciate that, but I don't think a large series like mine should block efforts to make Xen cleaner. Especially this case should be rather easy to handle, as there is no change of the logic of the patches to be expected. Juergen
diff --git a/xen/arch/x86/cpu/mcheck/mce.h b/xen/arch/x86/cpu/mcheck/mce.h index f2cebccdd1..7dcea7b454 100644 --- a/xen/arch/x86/cpu/mcheck/mce.h +++ b/xen/arch/x86/cpu/mcheck/mce.h @@ -3,12 +3,13 @@ #define _MCE_H #include <xen/init.h> +#include <xen/percpu.h> #include <xen/sched.h> #include <xen/smp.h> + #include <asm/types.h> #include <asm/traps.h> #include <asm/atomic.h> -#include <asm/percpu.h> #include "x86_mca.h" #include "mctelem.h" diff --git a/xen/include/asm-arm/percpu.h b/xen/include/asm-arm/percpu.h index 5ce81a1707..f1a8768080 100644 --- a/xen/include/asm-arm/percpu.h +++ b/xen/include/asm-arm/percpu.h @@ -10,9 +10,6 @@ extern char __per_cpu_start[], __per_cpu_data_end[]; extern unsigned long __per_cpu_offset[NR_CPUS]; void percpu_init_areas(void); -#define __DEFINE_PER_CPU(attr, type, name) \ - attr __typeof__(type) per_cpu_ ## name - #define per_cpu(var, cpu) \ (*RELOC_HIDE(&per_cpu__##var, __per_cpu_offset[cpu])) #define this_cpu(var) \ @@ -23,8 +20,6 @@ void percpu_init_areas(void); #define this_cpu_ptr(var) \ (*RELOC_HIDE(var, READ_SYSREG(TPIDR_EL2))) -#define DECLARE_PER_CPU(type, name) extern __typeof__(type) per_cpu__##name - #endif #endif /* __ARM_PERCPU_H__ */ diff --git a/xen/include/asm-x86/cpuid.h b/xen/include/asm-x86/cpuid.h index 548108f948..1b00e832d6 100644 --- a/xen/include/asm-x86/cpuid.h +++ b/xen/include/asm-x86/cpuid.h @@ -2,11 +2,11 @@ #define __X86_CPUID_H__ #include <asm/cpufeatureset.h> -#include <asm/percpu.h> #ifndef __ASSEMBLY__ #include <xen/types.h> #include <xen/kernel.h> +#include <xen/percpu.h> #include <xen/lib/x86/cpu-policy.h> #include <xen/lib/x86/cpuid.h> diff --git a/xen/include/asm-x86/irq.h b/xen/include/asm-x86/irq.h index bc0c0c15d2..d3124f7b5d 100644 --- a/xen/include/asm-x86/irq.h +++ b/xen/include/asm-x86/irq.h @@ -6,10 +6,10 @@ #include <asm/atomic.h> #include <asm/numa.h> #include <xen/cpumask.h> +#include <xen/percpu.h> #include <xen/smp.h> #include <asm/hvm/irq.h> #include <irq_vectors.h> -#include <asm/percpu.h> extern unsigned int nr_irqs_gsi; extern unsigned int nr_irqs; diff --git a/xen/include/asm-x86/percpu.h b/xen/include/asm-x86/percpu.h index 5b6cef04c4..2b0c29a233 100644 --- a/xen/include/asm-x86/percpu.h +++ b/xen/include/asm-x86/percpu.h @@ -7,17 +7,12 @@ extern unsigned long __per_cpu_offset[NR_CPUS]; void percpu_init_areas(void); #endif -#define __DEFINE_PER_CPU(attr, type, name) \ - attr __typeof__(type) per_cpu_ ## name - /* var is in discarded region: offset to particular copy we want */ #define per_cpu(var, cpu) \ (*RELOC_HIDE(&per_cpu__##var, __per_cpu_offset[cpu])) #define this_cpu(var) \ (*RELOC_HIDE(&per_cpu__##var, get_cpu_info()->per_cpu_offset)) -#define DECLARE_PER_CPU(type, name) extern __typeof__(type) per_cpu__##name - #define this_cpu_ptr(var) \ (*RELOC_HIDE(var, get_cpu_info()->per_cpu_offset)) diff --git a/xen/include/xen/percpu.h b/xen/include/xen/percpu.h index 71a31cc361..48a43f769d 100644 --- a/xen/include/xen/percpu.h +++ b/xen/include/xen/percpu.h @@ -3,6 +3,12 @@ #include <asm/percpu.h> +#define DECLARE_PER_CPU(type, name) \ + extern __typeof__(type) per_cpu__ ## name + +#define __DEFINE_PER_CPU(attr, type, name) \ + attr __typeof__(type) per_cpu_ ## name + /* * Separate out the type, so (int[3], foo) works. * diff --git a/xen/xsm/flask/include/avc.h b/xen/xsm/flask/include/avc.h index 93386bd7a1..c14bd07a2b 100644 --- a/xen/xsm/flask/include/avc.h +++ b/xen/xsm/flask/include/avc.h @@ -11,8 +11,9 @@ #include <xen/errno.h> #include <xen/lib.h> +#include <xen/percpu.h> #include <xen/spinlock.h> -#include <asm/percpu.h> + #include "flask.h" #include "av_permissions.h" #include "security.h"
These macros are identical across the architectures, and shouldn't be separate from the DEFINE_PER_CPU*() infrastructure. This converts the final asm/percpu.h includes, which were all using DECLARE_PER_CPU(), to include xen/percpu.h instead. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Wei Liu <wl@xen.org> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Julien Grall <julien.grall@arm.com> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> --- xen/arch/x86/cpu/mcheck/mce.h | 3 ++- xen/include/asm-arm/percpu.h | 5 ----- xen/include/asm-x86/cpuid.h | 2 +- xen/include/asm-x86/irq.h | 2 +- xen/include/asm-x86/percpu.h | 5 ----- xen/include/xen/percpu.h | 6 ++++++ xen/xsm/flask/include/avc.h | 3 ++- 7 files changed, 12 insertions(+), 14 deletions(-)