cleaner & greener (was Re: A patch for RTEMS4.10.0 PowerPC heap space initialization)
Kate Feng
feng1 at bnl.gov
Thu May 12 00:35:32 UTC 2011
I am fine with whatever is decided to get the bug fix. However, I
think that there are some misunderstandings, which we should
clarify. Please see below.
Joel Sherrill wrote:
> 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.
I did not say 'every BSP'. Only the powerpc BSPs, which used the
shared code. At RTEMS4.10, only if the bsp.h defines
CONFIGURE_MALLOC_BSP_SUPPORTS_SBRK, then
the malloc_sbrk_initialize() and bsp_sbrk_init() would be called.
However, it is true that the PPC shared code has been sbrk() for
the entire remaining memory (not the page size) once the 1st 32 MB is
exhausted. If you read the code
the_size = ((size + sbrk_amount) / sbrk_amount * sbrk_amount);
starting_address = (void *) sbrk(the_size);
.....
in all the RTEMS4.8 and up releases, then you will discover this.
However sbrk(the_size) does not mean to allocate(the_size).
In fact, it is Protected_heap_Allocate( RTEMS_Malloc_Heap, size );
instead of Protected_heap_Allocate( RTEMS_Malloc_Heap, the_size );
^^^^^^^^^
The code would be still done this way even if we use Till's suggestion.
The code in RTEMS4.10 : malloc_sbrk_initialize() does not seem to be used by
any BSP.
if (!starting_address)........
IS there something missing.
> 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.
Then something would have to be change in the existing code
in malloc.c and malloc_sbrk_helpers.c.
>
> You also added to the BSP interface with _bsp_sbrk_init(). Adding
> to it, isn't bad but the name is.
_bsp_sbrk_init() is what we have been used in the shared code in the past.
It is defined in the libbsp/powerpc/shared/startup/sbrk.c. Till wrote
and named it.
I do not understand why the name is bad ?
>
> Eighty column limits and no tabs.
Not sure what you meant here.
>
> But if TIll's is as simple as I think it is, it is the way to go.
It still does the same code I mentioned earlier. Till can verify this.
Cheers,
Kate
>> 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
>>>
>
>
More information about the users
mailing list