cleaner & greener (was Re: A patch for RTEMS4.10.0 PowerPC heap space initialization)
Kate Feng
feng1 at bnl.gov
Wed May 11 18:30:37 UTC 2011
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.
It does not necessary help to define CONFIGURE_MALLOC_BSP_SUPPORTS_SBRK
in bsp.h.
It has to be defined at the user's config.c file, where #include
<confdefs.h> is called.
Sometimes, <bsp.h> is forgotten to be included at config.c. Anyway, it
helps.
Can you or anyone see if the attached patch address all the issues, please ?
I did not update all the ChangeLog yet.
Cheers,
Kate
>
> + 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
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: rtems-4.10.0-green-heapspace_05102011.diff
Type: text/x-patch
Size: 7854 bytes
Desc: not available
URL: <http://lists.rtems.org/pipermail/users/attachments/20110511/9aaa5d7d/attachment-0001.bin>
More information about the users
mailing list