| Message ID | 1399045930-17364-2-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Rejected, archived |
| Headers | show |
On 02/05/14 16:52, Julien Grall wrote: > The macro boot_cpu will be used to get CPU variable from the boot CPU. > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > Suggested-by: Ian Campbell <ian.campbell@citrix.com> > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Keir Fraser <keir@xen.org> > --- > xen/include/xen/percpu.h | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/xen/include/xen/percpu.h b/xen/include/xen/percpu.h > index abe0b11..0e848bf 100644 > --- a/xen/include/xen/percpu.h > +++ b/xen/include/xen/percpu.h > @@ -16,6 +16,9 @@ > /* Preferred on Xen. Also see arch-defined per_cpu(). */ > #define this_cpu(var) __get_cpu_var(var) > > +/* Access variable on boot CPU */ > +#define boot_cpu(var) per_cpu(var, 0) > + What is this actually used for? In x86 with arbitrary cpu hotplug, we are trying to move away from the false assumption than cpu0 is the boot cpu. Actual boot data is stored sideways as boot_$FOO variables, and not accessed with the per-cpu mechanism. ~Andrew
>>> On 02.05.14 at 17:52, <julien.grall@linaro.org> wrote: > --- a/xen/include/xen/percpu.h > +++ b/xen/include/xen/percpu.h > @@ -16,6 +16,9 @@ > /* Preferred on Xen. Also see arch-defined per_cpu(). */ > #define this_cpu(var) __get_cpu_var(var) > > +/* Access variable on boot CPU */ > +#define boot_cpu(var) per_cpu(var, 0) > + > /* Linux compatibility. */ > #define get_cpu_var(var) this_cpu(var) > #define put_cpu_var(var) I can only second Andrew's comment - without it becoming clear when and why this would be used, this isn't going to be acceptable (and if it really was correct and worthwhile, this small a change wouldn't need separating out from the code that's intended to actually make use of it). Jan
Hi Andrew, On 05/02/2014 04:57 PM, Andrew Cooper wrote: > On 02/05/14 16:52, Julien Grall wrote: >> The macro boot_cpu will be used to get CPU variable from the boot CPU. >> >> Signed-off-by: Julien Grall <julien.grall@linaro.org> >> Suggested-by: Ian Campbell <ian.campbell@citrix.com> >> Cc: Ian Jackson <ian.jackson@eu.citrix.com> >> Cc: Jan Beulich <jbeulich@suse.com> >> Cc: Keir Fraser <keir@xen.org> >> --- >> xen/include/xen/percpu.h | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/xen/include/xen/percpu.h b/xen/include/xen/percpu.h >> index abe0b11..0e848bf 100644 >> --- a/xen/include/xen/percpu.h >> +++ b/xen/include/xen/percpu.h >> @@ -16,6 +16,9 @@ >> /* Preferred on Xen. Also see arch-defined per_cpu(). */ >> #define this_cpu(var) __get_cpu_var(var) >> >> +/* Access variable on boot CPU */ >> +#define boot_cpu(var) per_cpu(var, 0) >> + > > What is this actually used for? I use it to retrieve PPIs (per-processor interrupt) type and initialize PPIs for the new CPUs. > In x86 with arbitrary cpu hotplug, we are trying to move away from the > false assumption than cpu0 is the boot cpu. Actual boot data is stored > sideways as boot_$FOO variables, and not accessed with the per-cpu > mechanism. My solution won't work for hotplug. As it's not supported for ARM right now, I think we can live with it. I can either drop the macro or make ARM specific for now. Regards,
On 05/05/2014 08:59 AM, Jan Beulich wrote: >>>> On 02.05.14 at 17:52, <julien.grall@linaro.org> wrote: >> --- a/xen/include/xen/percpu.h >> +++ b/xen/include/xen/percpu.h >> @@ -16,6 +16,9 @@ >> /* Preferred on Xen. Also see arch-defined per_cpu(). */ >> #define this_cpu(var) __get_cpu_var(var) >> >> +/* Access variable on boot CPU */ >> +#define boot_cpu(var) per_cpu(var, 0) >> + >> /* Linux compatibility. */ >> #define get_cpu_var(var) this_cpu(var) >> #define put_cpu_var(var) > > I can only second Andrew's comment - without it becoming clear when > and why this would be used, this isn't going to be acceptable (and if > it really was correct and worthwhile, this small a change wouldn't need > separating out from the code that's intended to actually make use of it). It's used in the next patch (ie #2). I thought it was better to move out this small change rather than introducing it in the patch. This macro is here for a shortcut of per_cpu(myvar, 0). We have few usage on ARM, most of them are during boot. There is another when a secondary CPUs is booting. We need to retrieve the PPIs type (level/edge...) from the boot CPU. As said in the answer to Andrew, I can either move this macro in an ARM specific header or open code per_cpu(...,0). Regards,
>>> On 06.05.14 at 17:58, <julien.grall@linaro.org> wrote: > On 05/05/2014 08:59 AM, Jan Beulich wrote: >>>>> On 02.05.14 at 17:52, <julien.grall@linaro.org> wrote: >>> --- a/xen/include/xen/percpu.h >>> +++ b/xen/include/xen/percpu.h >>> @@ -16,6 +16,9 @@ >>> /* Preferred on Xen. Also see arch-defined per_cpu(). */ >>> #define this_cpu(var) __get_cpu_var(var) >>> >>> +/* Access variable on boot CPU */ >>> +#define boot_cpu(var) per_cpu(var, 0) >>> + >>> /* Linux compatibility. */ >>> #define get_cpu_var(var) this_cpu(var) >>> #define put_cpu_var(var) >> >> I can only second Andrew's comment - without it becoming clear when >> and why this would be used, this isn't going to be acceptable (and if >> it really was correct and worthwhile, this small a change wouldn't need >> separating out from the code that's intended to actually make use of it). > > It's used in the next patch (ie #2). I thought it was better to move out > this small change rather than introducing it in the patch. > > This macro is here for a shortcut of per_cpu(myvar, 0). We have few > usage on ARM, most of them are during boot. > > There is another when a secondary CPUs is booting. We need to retrieve > the PPIs type (level/edge...) from the boot CPU. > > As said in the answer to Andrew, I can either move this macro in an ARM > specific header or open code per_cpu(...,0). Either is fine with me, but putting this in common code isn't. Jan
On 06/05/14 17:04, Jan Beulich wrote: >>>> On 06.05.14 at 17:58, <julien.grall@linaro.org> wrote: >> On 05/05/2014 08:59 AM, Jan Beulich wrote: >>>>>> On 02.05.14 at 17:52, <julien.grall@linaro.org> wrote: >>>> --- a/xen/include/xen/percpu.h >>>> +++ b/xen/include/xen/percpu.h >>>> @@ -16,6 +16,9 @@ >>>> /* Preferred on Xen. Also see arch-defined per_cpu(). */ >>>> #define this_cpu(var) __get_cpu_var(var) >>>> >>>> +/* Access variable on boot CPU */ >>>> +#define boot_cpu(var) per_cpu(var, 0) >>>> + >>>> /* Linux compatibility. */ >>>> #define get_cpu_var(var) this_cpu(var) >>>> #define put_cpu_var(var) >>> I can only second Andrew's comment - without it becoming clear when >>> and why this would be used, this isn't going to be acceptable (and if >>> it really was correct and worthwhile, this small a change wouldn't need >>> separating out from the code that's intended to actually make use of it). >> It's used in the next patch (ie #2). I thought it was better to move out >> this small change rather than introducing it in the patch. >> >> This macro is here for a shortcut of per_cpu(myvar, 0). We have few >> usage on ARM, most of them are during boot. >> >> There is another when a secondary CPUs is booting. We need to retrieve >> the PPIs type (level/edge...) from the boot CPU. >> >> As said in the answer to Andrew, I can either move this macro in an ARM >> specific header or open code per_cpu(...,0). > Either is fine with me, but putting this in common code isn't. > > Jan Agreed - this should not be common. If you eventually want to support hotplug, making an arm specific boot_cpu() might help those efforts in the future. ~Andrew
On Fri, 2014-05-02 at 16:57 +0100, Andrew Cooper wrote: > On 02/05/14 16:52, Julien Grall wrote: > > The macro boot_cpu will be used to get CPU variable from the boot CPU. > > > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > > Suggested-by: Ian Campbell <ian.campbell@citrix.com> > > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > > Cc: Jan Beulich <jbeulich@suse.com> > > Cc: Keir Fraser <keir@xen.org> > > --- > > xen/include/xen/percpu.h | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/xen/include/xen/percpu.h b/xen/include/xen/percpu.h > > index abe0b11..0e848bf 100644 > > --- a/xen/include/xen/percpu.h > > +++ b/xen/include/xen/percpu.h > > @@ -16,6 +16,9 @@ > > /* Preferred on Xen. Also see arch-defined per_cpu(). */ > > #define this_cpu(var) __get_cpu_var(var) > > > > +/* Access variable on boot CPU */ > > +#define boot_cpu(var) per_cpu(var, 0) > > + > > What is this actually used for? > > In x86 with arbitrary cpu hotplug, we are trying to move away from the > false assumption than cpu0 is the boot cpu. Actual boot data is stored > sideways as boot_$FOO variables, and not accessed with the per-cpu > mechanism. Presumably in that case you would want it to expand into per_cpu(var, somevar_with_boot_cpu_nr_in_it) and being able to write that as boot_cpu would be preferable to open coding, especially since people would forever be writing 0, at least this way its in one place. Ian.
On Tue, 2014-05-06 at 16:54 +0100, Julien Grall wrote: > Hi Andrew, > > On 05/02/2014 04:57 PM, Andrew Cooper wrote: > > On 02/05/14 16:52, Julien Grall wrote: > >> The macro boot_cpu will be used to get CPU variable from the boot CPU. > >> > >> Signed-off-by: Julien Grall <julien.grall@linaro.org> > >> Suggested-by: Ian Campbell <ian.campbell@citrix.com> > >> Cc: Ian Jackson <ian.jackson@eu.citrix.com> > >> Cc: Jan Beulich <jbeulich@suse.com> > >> Cc: Keir Fraser <keir@xen.org> > >> --- > >> xen/include/xen/percpu.h | 3 +++ > >> 1 file changed, 3 insertions(+) > >> > >> diff --git a/xen/include/xen/percpu.h b/xen/include/xen/percpu.h > >> index abe0b11..0e848bf 100644 > >> --- a/xen/include/xen/percpu.h > >> +++ b/xen/include/xen/percpu.h > >> @@ -16,6 +16,9 @@ > >> /* Preferred on Xen. Also see arch-defined per_cpu(). */ > >> #define this_cpu(var) __get_cpu_var(var) > >> > >> +/* Access variable on boot CPU */ > >> +#define boot_cpu(var) per_cpu(var, 0) > >> + > > > > What is this actually used for? > > I use it to retrieve PPIs (per-processor interrupt) type and initialize > PPIs for the new CPUs. I wonder if perhaps this shouldn't be saved in a per-cpu variable then?
>>> On 08.05.14 at 11:55, <Ian.Campbell@citrix.com> wrote: > On Fri, 2014-05-02 at 16:57 +0100, Andrew Cooper wrote: >> On 02/05/14 16:52, Julien Grall wrote: >> > The macro boot_cpu will be used to get CPU variable from the boot CPU. >> > >> > Signed-off-by: Julien Grall <julien.grall@linaro.org> >> > Suggested-by: Ian Campbell <ian.campbell@citrix.com> >> > Cc: Ian Jackson <ian.jackson@eu.citrix.com> >> > Cc: Jan Beulich <jbeulich@suse.com> >> > Cc: Keir Fraser <keir@xen.org> >> > --- >> > xen/include/xen/percpu.h | 3 +++ >> > 1 file changed, 3 insertions(+) >> > >> > diff --git a/xen/include/xen/percpu.h b/xen/include/xen/percpu.h >> > index abe0b11..0e848bf 100644 >> > --- a/xen/include/xen/percpu.h >> > +++ b/xen/include/xen/percpu.h >> > @@ -16,6 +16,9 @@ >> > /* Preferred on Xen. Also see arch-defined per_cpu(). */ >> > #define this_cpu(var) __get_cpu_var(var) >> > >> > +/* Access variable on boot CPU */ >> > +#define boot_cpu(var) per_cpu(var, 0) >> > + >> >> What is this actually used for? >> >> In x86 with arbitrary cpu hotplug, we are trying to move away from the >> false assumption than cpu0 is the boot cpu. Actual boot data is stored >> sideways as boot_$FOO variables, and not accessed with the per-cpu >> mechanism. > > Presumably in that case you would want it to expand into > per_cpu(var, somevar_with_boot_cpu_nr_in_it) > and being able to write that as boot_cpu would be preferable to open > coding, especially since people would forever be writing 0, at least > this way its in one place. Except that in the general case you shouldn't even assume that the boot CPU is still online, i.e. such a macro might "point" into nowhere. Jan
On Thu, 2014-05-08 at 11:00 +0100, Jan Beulich wrote: > >>> On 08.05.14 at 11:55, <Ian.Campbell@citrix.com> wrote: > > On Fri, 2014-05-02 at 16:57 +0100, Andrew Cooper wrote: > >> On 02/05/14 16:52, Julien Grall wrote: > >> > The macro boot_cpu will be used to get CPU variable from the boot CPU. > >> > > >> > Signed-off-by: Julien Grall <julien.grall@linaro.org> > >> > Suggested-by: Ian Campbell <ian.campbell@citrix.com> > >> > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > >> > Cc: Jan Beulich <jbeulich@suse.com> > >> > Cc: Keir Fraser <keir@xen.org> > >> > --- > >> > xen/include/xen/percpu.h | 3 +++ > >> > 1 file changed, 3 insertions(+) > >> > > >> > diff --git a/xen/include/xen/percpu.h b/xen/include/xen/percpu.h > >> > index abe0b11..0e848bf 100644 > >> > --- a/xen/include/xen/percpu.h > >> > +++ b/xen/include/xen/percpu.h > >> > @@ -16,6 +16,9 @@ > >> > /* Preferred on Xen. Also see arch-defined per_cpu(). */ > >> > #define this_cpu(var) __get_cpu_var(var) > >> > > >> > +/* Access variable on boot CPU */ > >> > +#define boot_cpu(var) per_cpu(var, 0) > >> > + > >> > >> What is this actually used for? > >> > >> In x86 with arbitrary cpu hotplug, we are trying to move away from the > >> false assumption than cpu0 is the boot cpu. Actual boot data is stored > >> sideways as boot_$FOO variables, and not accessed with the per-cpu > >> mechanism. > > > > Presumably in that case you would want it to expand into > > per_cpu(var, somevar_with_boot_cpu_nr_in_it) > > and being able to write that as boot_cpu would be preferable to open > > coding, especially since people would forever be writing 0, at least > > this way its in one place. > > Except that in the general case you shouldn't even assume that the > boot CPU is still online, i.e. such a macro might "point" into nowhere. That's a good argument. Ian.
Hi Ian, On 05/08/2014 10:57 AM, Ian Campbell wrote: > On Tue, 2014-05-06 at 16:54 +0100, Julien Grall wrote: >> Hi Andrew, >> >> On 05/02/2014 04:57 PM, Andrew Cooper wrote: >>> On 02/05/14 16:52, Julien Grall wrote: >>>> The macro boot_cpu will be used to get CPU variable from the boot CPU. >>>> >>>> Signed-off-by: Julien Grall <julien.grall@linaro.org> >>>> Suggested-by: Ian Campbell <ian.campbell@citrix.com> >>>> Cc: Ian Jackson <ian.jackson@eu.citrix.com> >>>> Cc: Jan Beulich <jbeulich@suse.com> >>>> Cc: Keir Fraser <keir@xen.org> >>>> --- >>>> xen/include/xen/percpu.h | 3 +++ >>>> 1 file changed, 3 insertions(+) >>>> >>>> diff --git a/xen/include/xen/percpu.h b/xen/include/xen/percpu.h >>>> index abe0b11..0e848bf 100644 >>>> --- a/xen/include/xen/percpu.h >>>> +++ b/xen/include/xen/percpu.h >>>> @@ -16,6 +16,9 @@ >>>> /* Preferred on Xen. Also see arch-defined per_cpu(). */ >>>> #define this_cpu(var) __get_cpu_var(var) >>>> >>>> +/* Access variable on boot CPU */ >>>> +#define boot_cpu(var) per_cpu(var, 0) >>>> + >>> >>> What is this actually used for? >> >> I use it to retrieve PPIs (per-processor interrupt) type and initialize >> PPIs for the new CPUs. > > I wonder if perhaps this shouldn't be saved in a per-cpu variable then? As talked on patch #2, I will drop this patch and use a static variable to store PPIs type. Regards,
diff --git a/xen/include/xen/percpu.h b/xen/include/xen/percpu.h index abe0b11..0e848bf 100644 --- a/xen/include/xen/percpu.h +++ b/xen/include/xen/percpu.h @@ -16,6 +16,9 @@ /* Preferred on Xen. Also see arch-defined per_cpu(). */ #define this_cpu(var) __get_cpu_var(var) +/* Access variable on boot CPU */ +#define boot_cpu(var) per_cpu(var, 0) + /* Linux compatibility. */ #define get_cpu_var(var) this_cpu(var) #define put_cpu_var(var)
The macro boot_cpu will be used to get CPU variable from the boot CPU. Signed-off-by: Julien Grall <julien.grall@linaro.org> Suggested-by: Ian Campbell <ian.campbell@citrix.com> Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Keir Fraser <keir@xen.org> --- xen/include/xen/percpu.h | 3 +++ 1 file changed, 3 insertions(+)