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:52:42 UTC 2011
On 05/11/2011 01:30 PM, Kate Feng wrote:
> 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.
>
Then that's an application bug. There are other confdefs.h
macros which may be provided by bsp.h and need to be
honoured including preferred interrupt size, zeroing
workspace/heap, pre-requisite drivers, and post-requisite
drivers (off the top of my head).
> Can you or anyone see if the attached patch address all the issues, please ?
> I did not update all the ChangeLog yet.
>
See comments in Till's. I think his weak symbol suggestion
or the required sbrk macro is the way to go. If the BSP wants
to have a functional sbrk, I think it is perfectly fine to ask
it to somehow provide more information. The weak symbol
actually allows the user the option to control if it they like.
Till's overall suggestion is very simple and (I think) just a
few lines of code. It isn't very invasive.
I was mostly OK with your patch until you made the assumption
that every BSP would want to sbrk() for the entire remaining memory.
If you had some type of memory protection (which we could in
the not so distant future), then you would like to sbrk() by a smaller
fixed amount like a page size.
You also added to the BSP interface with _bsp_sbrk_init(). Adding
to it, isn't bad but the name is.
Eighty column limits and no tabs.
But if TIll's is as simple as I think it is, it is the way to go.
> 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
>>
--
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