cleaner & greener (was Re: A patch for RTEMS4.10.0 PowerPC heap space initialization)
Joel Sherrill
joel.sherrill at OARcorp.com
Wed May 11 18:53:02 UTC 2011
On 05/11/2011 01:39 PM, Till Straumann wrote:
> I would still argue that a simple and elegant
> way would be adding one extra argument (sbrk_amount)
> to bsp_get_work_area() and bootcard_bsp_libc_helper(),
> respectively.
>
> void
> bsp_get_work_area(
> void **work_area_begin,
> uintptr_t *work_area_size,
> void **heap_begin,
> uintptr_t *heap_size,
> uintptr_t *sbrk_amount);
>
> (BTW: why 'uintptr_t' - shouldn't that rather
> be 'size_t' ?)
>
> static void
> bootcard_bsp_libc_helper(
> void *work_area_begin,
> uintptr_t work_area_size,
> void *heap_begin,
> uintptr_t heap_size,
> uintptr_t sbrk_amount);
>
> This is really all that would be required.
> Adding the extra argument to bootcard_bsp_libc_helper()
> is a no-brainer since it is a 'static' routine,
> invisible outside of bootcard.c.
>
> Adding the argument to bsp_get_work_area() requires
> all BSPs which provide that routine to be changed,
> however.
>
And that's the type of thing that needs to be
avoided.
> Alternatively, yet another routine
>
> size_t bsp_get_sbrk_amount(void);
>
> could be declared and defaulted to do nothing
> using linker magic:
>
> bootcard.c:
>
> extern size_t bsp_get_sbrk_amount(void) __attribute__((weak));
>
> ...
> sbrk_amount = bsp_get_sbrk_amount ? bsp_get_sbrk_amount() : 0;
>
> a bit uglier but requires less changes to existing code.
>
I actually would prefer this but it is effectively the
same thing as my suggestion to add a required macro
to bsp.h when the BSP says it is using sbrk.
This looks simple and not much surgery.
> -- T.
>
> On 05/11/2011 10:03 AM, Joel Sherrill wrote:
>>
>> On 05/11/2011 09:40 AM, Kate Feng wrote:
>>> Can you or anyone see if the attached patch is acceptable, please ?
>>> It does not propagate information via global variables and
>>> there is no need to add an argument in any global function, either.
>>> My understanding is that only PPC port needs the
>>> rtems_malloc_sbrk_helpers.
>>>
>>> I am thinking to submit a PR if there is no further question, feedback,
>>> or suggestion after 24 hours.
>>>
>> This seems inordinately complicated and is breaking a couple
>> of patterns.
>>
>> + bsp.h is supposed to define CONFIGURE_MALLOC_BSP_SUPPORTS_SBRK
>> and NOT set the sbrk helper pointer in code.
>>
>> + I agree that we shouldn't pass 0 all the time to bsp_libc_init() as the
>> sbrk amount. This is clearly an oversight. If you added another macro
>> required to be set by the bsp.h when it defines
>> CONFIGURE_MALLOC_BSP_SUPPORTS_SBRK, then it could be the name
>> of a variable or constant to pass to bsp_libc_init() as the sbrk() amount.
>>
>> If the above is addressed, then I fail to see why this call in
>> RTEMS_Malloc_Initialize() isn't sufficient to let the BSP's
>> sbrk() work:
>>
>> if ( rtems_malloc_sbrk_helpers != NULL ) {
>> heap_begin = (*rtems_malloc_sbrk_helpers->initialize)(
>> heap_begin,
>> sbrk_amount
>> );
>> heap_size = (uintptr_t) sbrk_amount;
>> }
>>
>> The problem is that the sbrk amount is never flowed into the
>> code flow at the top and you are going to great pains to inject
>> it later.
>>
>> If the sbrk() plugin flow is wrong, let's fix that not work around it.
>>
>> --joel
>>
>>
>>> Thanks,
>>> Kate
>>>
>>> Till Straumann wrote:
>>>> I agree with Kate that ppc/sbrk is currently broken in 4.10
>>>> (bsp_libc_init() is always called with a zero sbrk_amount).
>>>>
>>>> I also suggest that this should be fixed but I'm not sure
>>>> the proposed approach (propagating information via global
>>>> variables) is the best solution.
>>>>
>>>> It may be cleaner to add an additional argument to
>>>> bsp_get_work_area() and bootcard_bsp_libc_helper()
>>>> to propagate the sbrk_amount.
>>>>
>>>> -- Till
>>>>
>>>>
>>>> On 05/10/2011 05:40 PM, Kate Feng wrote:
>>>>> Sebastian Huber wrote:
>>>>>> Hello,
>>>>>>
>>>>>> I have some questions about this patch.
>>>>> Can you or anyone see if the attached new patch acceptable, please ?
>>>>> I am thinking to submit a PR by tomorrow afternoon if there is
>>>>> no further question, feedback or suggestion. It seems a critical bug.
>>>>>
>>>>> Cheers,
>>>>> Kate
>>>>>> On 05/07/2011 02:44 PM, Kate Feng wrote:
>>>>>>> Hi Joel,
>>>>>>>
>>>>>>> There are bugs in RTEMS4.10.0 and its CVS for the PowerPC heap space
>>>>>>> allocation. All PPC boards are limited to 32M bytes of memory space
>>>>>>> due
>>>>>>> to the change in RTEMS4.10 for its work space. The attached patch
>>>>>>> fixed
>>>>>>> this.
>>>>>>> I consider that it is critical. Thus, I will submit a PR.
>>>>>>>
>>>>>>> Cheers,
>>>>>>> Kate Feng
>>>>>>>
>>>>>>>
>>>>>>> rtems-4.10.0-powerpc-heapspace.diff
>>>>>>>
>>>>>>>
>>>>>>> diff -Naur rtems-4.10.0.orig/c/src/lib/libbsp/powerpc/ChangeLog
>>>>>>> rtems-4.10.0/c/src/lib/libbsp/powerpc/ChangeLog
>>>>>>> --- rtems-4.10.0.orig/c/src/lib/libbsp/powerpc/ChangeLog 2011-03-04
>>>>>>> 11:56:23.000000000 -0500
>>>>>>> +++ rtems-4.10.0/c/src/lib/libbsp/powerpc/ChangeLog 2011-05-07
>>>>>>> 08:15:17.000000000 -0400
>>>>>>> @@ -1,3 +1,8 @@
>>>>>>> +2011-05-07 Kate Feng<feng at bnl.gov>
>>>>>>> +
>>>>>>> + * shared/startup/bspgetworkarea.c : Moved _bsp_sbrk_init to +
>>>>>>> c/src/lib/libbsp/shared/bootcard.c
>>>>>>> +
>>>>>>> 2011-03-04 Joel Sherrill<joel.sherrilL at OARcorp.com>
>>>>>>>
>>>>>>> * shared/start/start.S: Remove conflict markers in comment.
>>>>>>> diff -Naur
>>>>>>> rtems-4.10.0.orig/c/src/lib/libbsp/powerpc/shared/startup/bspgetworkarea.c
>>>>>>>
>>>>>>>
>>>>>>> rtems-4.10.0/c/src/lib/libbsp/powerpc/shared/startup/bspgetworkarea.c
>>>>>>> ---
>>>>>>> rtems-4.10.0.orig/c/src/lib/libbsp/powerpc/shared/startup/bspgetworkarea.c
>>>>>>>
>>>>>>>
>>>>>>> 2009-11-29 23:29:47.000000000 -0500
>>>>>>> +++
>>>>>>> rtems-4.10.0/c/src/lib/libbsp/powerpc/shared/startup/bspgetworkarea.c
>>>>>>> 2011-05-07 08:21:08.000000000 -0400
>>>>>>> @@ -31,15 +31,12 @@
>>>>>>> )
>>>>>>> {
>>>>>>> uintptr_t work_size;
>>>>>>> - uintptr_t spared;
>>>>>>> uintptr_t work_area;
>>>>>>>
>>>>>>> work_area = (uintptr_t)&__rtems_end +
>>>>>>> rtems_configuration_get_interrupt_stack_size();
>>>>>>> work_size = (uintptr_t)BSP_mem_size - work_area;
>>>>>>>
>>>>>>> - spared = _bsp_sbrk_init( work_area,&work_size );
>>>>>>> -
>>>>>>> *work_area_start = (void *)work_area,
>>>>>>> *work_area_size = work_size;
>>>>>>> *heap_start = BSP_BOOTCARD_HEAP_USES_WORK_AREA;
>>>>>>> @@ -54,7 +51,7 @@
>>>>>>> void *sp = __builtin_frame_address(0);
>>>>>>> void *end = *work_area_start + *work_area_size;
>>>>>>> printk(
>>>>>>> - "work_area_start = 0x%p\n"
>>>>>>> + "work_area_start = 0x%08\n"
>>>>>> What about 64-bit architectures?
>>>>>>
>>>>>>> "work_area_size = %d 0x%08x\n"
>>>>>>> "end = 0x%p\n"
>>>>>>> "heap_start = 0x%p\n"
>>>>>>> diff -Naur rtems-4.10.0.orig/c/src/lib/libbsp/shared/bootcard.c
>>>>>>> rtems-4.10.0/c/src/lib/libbsp/shared/bootcard.c
>>>>>>> --- rtems-4.10.0.orig/c/src/lib/libbsp/shared/bootcard.c 2009-10-12
>>>>>>> 16:22:18.000000000 -0400
>>>>>>> +++ rtems-4.10.0/c/src/lib/libbsp/shared/bootcard.c 2011-05-07
>>>>>>> 07:58:07.000000000 -0400
>>>>>>> @@ -77,8 +77,10 @@
>>>>>>> uintptr_t heap_size
>>>>>>> )
>>>>>>> {
>>>>>>> - if ( !rtems_unified_work_area&&
>>>>>>> - heap_start == BSP_BOOTCARD_HEAP_USES_WORK_AREA) {
>>>>>>> + uintptr_t spared;
>>>>>>> +
>>>>>>> + if ( (!rtems_unified_work_area)&&
>>>>>>> + (heap_start == BSP_BOOTCARD_HEAP_USES_WORK_AREA)) {
>>>>>> Why the extra parentheses?
>>>>>>
>>>>>>> uintptr_t work_space_size =
>>>>>>> rtems_configuration_get_work_space_size();
>>>>>>>
>>>>>>> heap_start = (char *) work_area_start + work_space_size;
>>>>>>> @@ -89,8 +91,13 @@
>>>>>>> heap_size = heap_size_default;
>>>>>>> }
>>>>>>> }
>>>>>>> + else /* Kate Feng: if (rtems_unified_work_area) */ {kkk
>>>>>> What is the purpose of this comment?
>>>>>>
>>>>>>> + heap_start = (char *) work_area_start ;
>>>>>> What is the purpose of this cast? Why the extra ' ' before the ';'?
>>>>>>
>>>>>>> + heap_size = work_area_size;
>>>>>>> + }
>>>>>>> + spared = _bsp_sbrk_init( (uintptr_t) heap_start ,&heap_size );
>>>>>> Here we force every BSP to provide this sbrk() mechanic. I don't think
>>>>>> this is
>>>>>> good. This _bsp_sbrk_init() only exists for the PowerPC at the moment.
>>>>>>
>>>>>>> - bsp_libc_init(heap_start, heap_size, 0);
>>>>>>> + bsp_libc_init((void *) heap_start, heap_size, spared);
>>>>>> What is the purpose of this cast?
>>>>>>
>>>>>>> }
>>>>>>>
>>>>>>> /*
>>>>>
>>>>> _______________________________________________
>>>>> rtems-users mailing list
>>>>> rtems-users at rtems.org
>>>>> http://www.rtems.org/mailman/listinfo/rtems-users
>>>> _______________________________________________
>>>> rtems-users mailing list
>>>> rtems-users at rtems.org
>>>> http://www.rtems.org/mailman/listinfo/rtems-users
>>
--
Joel Sherrill, Ph.D. Director of Research& Development
joel.sherrill at OARcorp.com On-Line Applications Research
Ask me about RTEMS: a free RTOS Huntsville AL 35805
Support Available (256) 722-9985
More information about the users
mailing list