| Message ID | 1450385974-12732-3-git-send-email-jonathan.creekmore@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 17/12/2015 20:59, Jonathan Creekmore wrote: > Creates a section to contain scheduler entry pointers that are gathered > together into an array. This will allow, in a follow-on patch, scheduler > entries to be automatically gathered together into the array for > automatic parsing. > > CC: Ian Campbell <ian.campbell@citrix.com> > CC: Stefano Stabellini <stefano.stabellini@citrix.com> > CC: Keir Fraser <keir@xen.org> > CC: Jan Beulich <jbeulich@suse.com> > CC: Andrew Cooper <andrew.cooper3@citrix.com> > Signed-off-by: Jonathan Creekmore <jonathan.creekmore@gmail.com> > --- > xen/arch/arm/xen.lds.S | 4 ++++ > xen/arch/x86/xen.lds.S | 4 ++++ > 2 files changed, 8 insertions(+) > > diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S > index 0488f37..39a4c86 100644 > --- a/xen/arch/arm/xen.lds.S > +++ b/xen/arch/arm/xen.lds.S > @@ -57,6 +57,10 @@ SECTIONS > . = ALIGN(PAGE_SIZE); > *(.data.page_aligned) > *(.data) > + . = ALIGN(8); > + __schedulers_start = .; > + *(.data.schedulers) > + __schedulers_end = .; These arrays are only ever used in __init scheduler_init(). They should be in .init.data rather than .data, which allows their memory to be reclaimed after boot. With that, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> On Dec 18, 2015, at 3:01 AM, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > On 17/12/2015 20:59, Jonathan Creekmore wrote: >> Creates a section to contain scheduler entry pointers that are gathered >> together into an array. This will allow, in a follow-on patch, scheduler >> entries to be automatically gathered together into the array for >> automatic parsing. >> >> CC: Ian Campbell <ian.campbell@citrix.com> >> CC: Stefano Stabellini <stefano.stabellini@citrix.com> >> CC: Keir Fraser <keir@xen.org> >> CC: Jan Beulich <jbeulich@suse.com> >> CC: Andrew Cooper <andrew.cooper3@citrix.com> >> Signed-off-by: Jonathan Creekmore <jonathan.creekmore@gmail.com> >> --- >> xen/arch/arm/xen.lds.S | 4 ++++ >> xen/arch/x86/xen.lds.S | 4 ++++ >> 2 files changed, 8 insertions(+) >> >> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S >> index 0488f37..39a4c86 100644 >> --- a/xen/arch/arm/xen.lds.S >> +++ b/xen/arch/arm/xen.lds.S >> @@ -57,6 +57,10 @@ SECTIONS >> . = ALIGN(PAGE_SIZE); >> *(.data.page_aligned) >> *(.data) >> + . = ALIGN(8); >> + __schedulers_start = .; >> + *(.data.schedulers) >> + __schedulers_end = .; > > These arrays are only ever used in __init scheduler_init(). They should > be in .init.data rather than .data, which allows their memory to be > reclaimed after boot. > > With that, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> So, they are used in scheduler_init() which is marked __init, but scheduler_alloc also uses that array (and did before my patch) and it is not marked __init.
On 18/12/15 16:40, Jonathan Creekmore wrote: >> On Dec 18, 2015, at 3:01 AM, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >> >> On 17/12/2015 20:59, Jonathan Creekmore wrote: >>> Creates a section to contain scheduler entry pointers that are gathered >>> together into an array. This will allow, in a follow-on patch, scheduler >>> entries to be automatically gathered together into the array for >>> automatic parsing. >>> >>> CC: Ian Campbell <ian.campbell@citrix.com> >>> CC: Stefano Stabellini <stefano.stabellini@citrix.com> >>> CC: Keir Fraser <keir@xen.org> >>> CC: Jan Beulich <jbeulich@suse.com> >>> CC: Andrew Cooper <andrew.cooper3@citrix.com> >>> Signed-off-by: Jonathan Creekmore <jonathan.creekmore@gmail.com> >>> --- >>> xen/arch/arm/xen.lds.S | 4 ++++ >>> xen/arch/x86/xen.lds.S | 4 ++++ >>> 2 files changed, 8 insertions(+) >>> >>> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S >>> index 0488f37..39a4c86 100644 >>> --- a/xen/arch/arm/xen.lds.S >>> +++ b/xen/arch/arm/xen.lds.S >>> @@ -57,6 +57,10 @@ SECTIONS >>> . = ALIGN(PAGE_SIZE); >>> *(.data.page_aligned) >>> *(.data) >>> + . = ALIGN(8); >>> + __schedulers_start = .; >>> + *(.data.schedulers) >>> + __schedulers_end = .; >> These arrays are only ever used in __init scheduler_init(). They should >> be in .init.data rather than .data, which allows their memory to be >> reclaimed after boot. >> >> With that, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> > So, they are used in scheduler_init() which is marked __init, but scheduler_alloc > also uses that array (and did before my patch) and it is not marked __init. > Ah yes - so they are. Apologies for the noise. This should be in .data and my R-b stands. ~Andrew
>>> On 18.12.15 at 17:48, <andrew.cooper3@citrix.com> wrote: > On 18/12/15 16:40, Jonathan Creekmore wrote: >>> On Dec 18, 2015, at 3:01 AM, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >>> >>> On 17/12/2015 20:59, Jonathan Creekmore wrote: >>>> Creates a section to contain scheduler entry pointers that are gathered >>>> together into an array. This will allow, in a follow-on patch, scheduler >>>> entries to be automatically gathered together into the array for >>>> automatic parsing. >>>> >>>> CC: Ian Campbell <ian.campbell@citrix.com> >>>> CC: Stefano Stabellini <stefano.stabellini@citrix.com> >>>> CC: Keir Fraser <keir@xen.org> >>>> CC: Jan Beulich <jbeulich@suse.com> >>>> CC: Andrew Cooper <andrew.cooper3@citrix.com> >>>> Signed-off-by: Jonathan Creekmore <jonathan.creekmore@gmail.com> >>>> --- >>>> xen/arch/arm/xen.lds.S | 4 ++++ >>>> xen/arch/x86/xen.lds.S | 4 ++++ >>>> 2 files changed, 8 insertions(+) >>>> >>>> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S >>>> index 0488f37..39a4c86 100644 >>>> --- a/xen/arch/arm/xen.lds.S >>>> +++ b/xen/arch/arm/xen.lds.S >>>> @@ -57,6 +57,10 @@ SECTIONS >>>> . = ALIGN(PAGE_SIZE); >>>> *(.data.page_aligned) >>>> *(.data) >>>> + . = ALIGN(8); >>>> + __schedulers_start = .; >>>> + *(.data.schedulers) >>>> + __schedulers_end = .; >>> These arrays are only ever used in __init scheduler_init(). They should >>> be in .init.data rather than .data, which allows their memory to be >>> reclaimed after boot. >>> >>> With that, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> >> So, they are used in scheduler_init() which is marked __init, but > scheduler_alloc >> also uses that array (and did before my patch) and it is not marked __init. > > Ah yes - so they are. Apologies for the noise. This should be in .data > and my R-b stands. In .rodata perhaps? Jan
On 18/12/15 17:07, Jan Beulich wrote: >>>> On 18.12.15 at 17:48, <andrew.cooper3@citrix.com> wrote: >> On 18/12/15 16:40, Jonathan Creekmore wrote: >>>> On Dec 18, 2015, at 3:01 AM, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >>>> >>>> On 17/12/2015 20:59, Jonathan Creekmore wrote: >>>>> Creates a section to contain scheduler entry pointers that are gathered >>>>> together into an array. This will allow, in a follow-on patch, scheduler >>>>> entries to be automatically gathered together into the array for >>>>> automatic parsing. >>>>> >>>>> CC: Ian Campbell <ian.campbell@citrix.com> >>>>> CC: Stefano Stabellini <stefano.stabellini@citrix.com> >>>>> CC: Keir Fraser <keir@xen.org> >>>>> CC: Jan Beulich <jbeulich@suse.com> >>>>> CC: Andrew Cooper <andrew.cooper3@citrix.com> >>>>> Signed-off-by: Jonathan Creekmore <jonathan.creekmore@gmail.com> >>>>> --- >>>>> xen/arch/arm/xen.lds.S | 4 ++++ >>>>> xen/arch/x86/xen.lds.S | 4 ++++ >>>>> 2 files changed, 8 insertions(+) >>>>> >>>>> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S >>>>> index 0488f37..39a4c86 100644 >>>>> --- a/xen/arch/arm/xen.lds.S >>>>> +++ b/xen/arch/arm/xen.lds.S >>>>> @@ -57,6 +57,10 @@ SECTIONS >>>>> . = ALIGN(PAGE_SIZE); >>>>> *(.data.page_aligned) >>>>> *(.data) >>>>> + . = ALIGN(8); >>>>> + __schedulers_start = .; >>>>> + *(.data.schedulers) >>>>> + __schedulers_end = .; >>>> These arrays are only ever used in __init scheduler_init(). They should >>>> be in .init.data rather than .data, which allows their memory to be >>>> reclaimed after boot. >>>> >>>> With that, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> >>> So, they are used in scheduler_init() which is marked __init, but >> scheduler_alloc >>> also uses that array (and did before my patch) and it is not marked __init. >> Ah yes - so they are. Apologies for the noise. This should be in .data >> and my R-b stands. > In .rodata perhaps? Ah yes - they don't need modifying at all. They are just pointers to each of the struct scheduler ops. .rodata it is. ~Andrew
> On Dec 18, 2015, at 11:07 AM, Jan Beulich <JBeulich@suse.com> wrote: > >>>> On 18.12.15 at 17:48, <andrew.cooper3@citrix.com> wrote: >> On 18/12/15 16:40, Jonathan Creekmore wrote: >>>> On Dec 18, 2015, at 3:01 AM, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >>>> >>>> On 17/12/2015 20:59, Jonathan Creekmore wrote: >>>>> Creates a section to contain scheduler entry pointers that are gathered >>>>> together into an array. This will allow, in a follow-on patch, scheduler >>>>> entries to be automatically gathered together into the array for >>>>> automatic parsing. >>>>> >>>>> CC: Ian Campbell <ian.campbell@citrix.com> >>>>> CC: Stefano Stabellini <stefano.stabellini@citrix.com> >>>>> CC: Keir Fraser <keir@xen.org> >>>>> CC: Jan Beulich <jbeulich@suse.com> >>>>> CC: Andrew Cooper <andrew.cooper3@citrix.com> >>>>> Signed-off-by: Jonathan Creekmore <jonathan.creekmore@gmail.com> >>>>> --- >>>>> xen/arch/arm/xen.lds.S | 4 ++++ >>>>> xen/arch/x86/xen.lds.S | 4 ++++ >>>>> 2 files changed, 8 insertions(+) >>>>> >>>>> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S >>>>> index 0488f37..39a4c86 100644 >>>>> --- a/xen/arch/arm/xen.lds.S >>>>> +++ b/xen/arch/arm/xen.lds.S >>>>> @@ -57,6 +57,10 @@ SECTIONS >>>>> . = ALIGN(PAGE_SIZE); >>>>> *(.data.page_aligned) >>>>> *(.data) >>>>> + . = ALIGN(8); >>>>> + __schedulers_start = .; >>>>> + *(.data.schedulers) >>>>> + __schedulers_end = .; >>>> These arrays are only ever used in __init scheduler_init(). They should >>>> be in .init.data rather than .data, which allows their memory to be >>>> reclaimed after boot. >>>> >>>> With that, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> >>> So, they are used in scheduler_init() which is marked __init, but >> scheduler_alloc >>> also uses that array (and did before my patch) and it is not marked __init. >> >> Ah yes - so they are. Apologies for the noise. This should be in .data >> and my R-b stands. > > In .rodata perhaps? I initially put it in .rodata, but the current algorithm for choosing the scheduler NULLs out the pointers in the array if the global_init() function fails. To emulate that behavior, I had to move the section to .data instead of .rodata.
On 18/12/15 17:11, Jonathan Creekmore wrote: >> On Dec 18, 2015, at 11:07 AM, Jan Beulich <JBeulich@suse.com> wrote: >> >>>>> On 18.12.15 at 17:48, <andrew.cooper3@citrix.com> wrote: >>> On 18/12/15 16:40, Jonathan Creekmore wrote: >>>>> On Dec 18, 2015, at 3:01 AM, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >>>>> >>>>> On 17/12/2015 20:59, Jonathan Creekmore wrote: >>>>>> Creates a section to contain scheduler entry pointers that are gathered >>>>>> together into an array. This will allow, in a follow-on patch, scheduler >>>>>> entries to be automatically gathered together into the array for >>>>>> automatic parsing. >>>>>> >>>>>> CC: Ian Campbell <ian.campbell@citrix.com> >>>>>> CC: Stefano Stabellini <stefano.stabellini@citrix.com> >>>>>> CC: Keir Fraser <keir@xen.org> >>>>>> CC: Jan Beulich <jbeulich@suse.com> >>>>>> CC: Andrew Cooper <andrew.cooper3@citrix.com> >>>>>> Signed-off-by: Jonathan Creekmore <jonathan.creekmore@gmail.com> >>>>>> --- >>>>>> xen/arch/arm/xen.lds.S | 4 ++++ >>>>>> xen/arch/x86/xen.lds.S | 4 ++++ >>>>>> 2 files changed, 8 insertions(+) >>>>>> >>>>>> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S >>>>>> index 0488f37..39a4c86 100644 >>>>>> --- a/xen/arch/arm/xen.lds.S >>>>>> +++ b/xen/arch/arm/xen.lds.S >>>>>> @@ -57,6 +57,10 @@ SECTIONS >>>>>> . = ALIGN(PAGE_SIZE); >>>>>> *(.data.page_aligned) >>>>>> *(.data) >>>>>> + . = ALIGN(8); >>>>>> + __schedulers_start = .; >>>>>> + *(.data.schedulers) >>>>>> + __schedulers_end = .; >>>>> These arrays are only ever used in __init scheduler_init(). They should >>>>> be in .init.data rather than .data, which allows their memory to be >>>>> reclaimed after boot. >>>>> >>>>> With that, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>> So, they are used in scheduler_init() which is marked __init, but >>> scheduler_alloc >>>> also uses that array (and did before my patch) and it is not marked __init. >>> Ah yes - so they are. Apologies for the noise. This should be in .data >>> and my R-b stands. >> In .rodata perhaps? > I initially put it in .rodata, but the current algorithm for choosing the scheduler NULLs out > the pointers in the array if the global_init() function fails. To emulate that behavior, I had to > move the section to .data instead of .rodata. Hmm. That is unfortunate. Best to leave it in .data, and leave fixing the scheduler initialisation to the other scheduler clean-up work; there is plenty of other cleanup work to do, and conflating the two issues is best avoided. ~Andrew
diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S index 0488f37..39a4c86 100644 --- a/xen/arch/arm/xen.lds.S +++ b/xen/arch/arm/xen.lds.S @@ -57,6 +57,10 @@ SECTIONS . = ALIGN(PAGE_SIZE); *(.data.page_aligned) *(.data) + . = ALIGN(8); + __schedulers_start = .; + *(.data.schedulers) + __schedulers_end = .; *(.data.rel) *(.data.rel.*) CONSTRUCTORS diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S index e18e08f..70caf85 100644 --- a/xen/arch/x86/xen.lds.S +++ b/xen/arch/x86/xen.lds.S @@ -88,6 +88,10 @@ SECTIONS . = ALIGN(PAGE_SIZE); *(.data.page_aligned) *(.data) + . = ALIGN(8); + __schedulers_start = .; + *(.data.schedulers) + __schedulers_end = .; *(.data.rel) *(.data.rel.*) CONSTRUCTORS
Creates a section to contain scheduler entry pointers that are gathered together into an array. This will allow, in a follow-on patch, scheduler entries to be automatically gathered together into the array for automatic parsing. CC: Ian Campbell <ian.campbell@citrix.com> CC: Stefano Stabellini <stefano.stabellini@citrix.com> CC: Keir Fraser <keir@xen.org> CC: Jan Beulich <jbeulich@suse.com> CC: Andrew Cooper <andrew.cooper3@citrix.com> Signed-off-by: Jonathan Creekmore <jonathan.creekmore@gmail.com> --- xen/arch/arm/xen.lds.S | 4 ++++ xen/arch/x86/xen.lds.S | 4 ++++ 2 files changed, 8 insertions(+)