patchlwIP - A Lightweight TCP/IP stack - Patches: patch #6822, Patch places memory pools in...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

patch #6822: Patch places memory pools in separate arrays

Submitter:  Bill Auerbach <billauerbach>
Submitted:  Fri 01 May 2009 01:30:35 PM UTC
   
 
Category:  None Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  goldsimon Open/Closed:  Closed
Planned Release:  None

Jump to the original submission

Sun 10 Jan 2010 01:14:05 PM UTC, comment #16: 

I've checked in this patch (slightly modified: pools are named memp_memory_XXX_base, option moved to opt.h, comments changed) as it is essentially independent of how we relocate the individual pools: this patch only separates them.

After all, I think Bill's original idea (relocating the pools in cc.h) isn't that bad: it's quite difficult to do this with defines as we need one define per pool plus an automatic fallback if it isn't defined...

Closing this as done since the relocation to different sections is covered by bug #26133.

Simon Goldschmidt <goldsimon>
Group administrator
Mon 02 Nov 2009 02:08:19 PM UTC, comment #15: 

I'm pretty sure we do structure packing that way on systems that use "#pragma pack"

Kieran Mansley <kieranm>
Group Member
Mon 02 Nov 2009 01:49:13 PM UTC, comment #14: 

Kieran, the point I'm trying to make is that if a compiler requires a #pragma to change a declaration's section, a #pragma can't be generated from a macro.

Bill Auerbach <billauerbach>
Mon 02 Nov 2009 09:06:47 AM UTC, comment #13: 

Fair enough, so make the MEM_ONCHIP macro more complex, so that it can control the entire declaration.  I meant my example to illustrate where in the source the different bits of code should go, rather than the right code to use.

Kieran Mansley <kieranm>
Group Member
Fri 30 Oct 2009 05:49:01 PM UTC, comment #12: 

Kieran, that's fine for GCC and other compilers that modify a function declaration but it will not work to inject a #pragma if that's what a compiler requires.  I think that someone developing with lwIP to the point of having a reason to move pools probably can figure out to do it differently than you prescribe.

Bill Auerbach <billauerbach>
Fri 30 Oct 2009 04:14:38 PM UTC, comment #11: 

Seems like I posted a bit of code about
moving the heap on the wrong thread.
I'll post it as new task. Sorry for the noise.

Iordan Neshev <iordan_neshev>
Fri 30 Oct 2009 03:50:31 PM UTC, comment #10: 

My view is that cc.h should have the code that defines how to place these bits in different areas.  lwipopts should select which one of these definitions to use.

For example (but please don't think these are good names, just an illustration):

in cc.h

#define MEM_ONCHIP attribute((section(".onchip_mem")))

in lwipopts.h

#define UDP_PCB_MEM_LOCATION MEM_ONCHIP

in opt.h

#ifndef UDP_PCB_MEM_LOCATION
# define UDP_PCB_MEM_LOCATION
#endif

in wherever the array is defined:

extern u8_t UDP_PCB_MEM_LOCATION udp_pcb[]

Kieran Mansley <kieranm>
Group Member
Fri 30 Oct 2009 12:52:08 PM UTC, comment #9: 

Simon, it doesn't matter I guess.  I thought cc.h is for platform/compiler specific things, right?  lwipopts.h is getting really big too, but I can see the advantage to keeping them together.  Or add a comment "Add to cc.h the compiler dependent statements to move the pool arrays to new sections".

Maybe these section changing lines should be in a macro that is inserted in memp.c?  But this won't work if the additions are #pragmas.  You could force an #include file.  If MEMP_SEPARATE_POOLS is defined, the #include file is inserted which would generate an error if it was not present.

Probably not too many will use this feature, which is why not cluttering lwipopts.h with it might be a good thing.

Bill Auerbach <billauerbach>
Fri 30 Oct 2009 12:41:29 PM UTC, comment #8: 

Iordan, I didn't move the discussion - this patch started as per the subject originally by me to move memory pools to new sections.  I wasn't bringing up moving the heap to a new section - isn't this a different patch with a different goal?

Bill Auerbach <billauerbach>
Fri 30 Oct 2009 11:19:14 AM UTC, comment #7: 

This post would go to bug #26133, but Bill decided to
move the discussion here (if I understood it right).
(feel free to move the post back in bug #26133, if its
place is not here).

This is my suggestion how to move the heap with GCC.
It does not affect the current code if a section is not defined.


This should go in mem.c (above the heap declaration)

/** Defining this allows the heap to be placed in a user-defined

  • linker section (e.g. EXTERNAL RAM) */

#ifndef HEAP_SECT_PRE
#define HEAP_SECT_PRE
#endif  /* HEAP_SECT_PRE */

/** Defining this allows the heap to be placed in a user-defined

  • linker section (e.g. EXTERNAL RAM) */

#ifndef HEAP_SECT_POST
#define HEAP_SECT_POST
#endif  /* HEAP_SECT_POST */

mem.c, line 175: change 
static u8_t ram_heap[MEM_SIZE_ALIGNED + (2*SIZEOF_STRUCT_MEM) + MEM_ALIGNMENT];
to
static HEAP_SECT_PRE u8_t ram_heap[MEM_SIZE_ALIGNED + (2*SIZEOF_STRUCT_MEM) + MEM_ALIGNMENT] HEAP_SECT_POST;

We may add a note how to define the section
/** Place this in cc.h to tell GCC to place the heap in section .externalram:
    #define HEAP_SECT_PRE(/POST) _attribute_ ((section (".externalram")))
*/


Alternatively, the two #ifndef-#define-#endifs can be placed in opt.h. Then the _atribute_ would go to lwipopts.h

Iordan Neshev <iordan_neshev>
Thu 29 Oct 2009 10:03:11 PM UTC, comment #6: 

Oh, you mean you would place the extern definitions like the following into cc.h?

extern u8_t _attribute_((section(".onchip_mem"))) memp_UDP_PCB_base[];

That would mean if it's not defined, there would be a linker error, wouldn't it?

I'd prefer to define the sections (in lwipopts.h) and let LWIP_MEMPOOL() somehow use these defines. That way we can have one define (all memp pools in the same section) as well as the #ifndef fallback.

I didn't yet think about how to manage that, only I'd prefer this over the cc.h idea (it's more like the rest of the config options).

> I thought you meant how (since you said how :-) ) more than where.


Well, I meant 'how' in terms of 'the attached patch doesn't show it' - there's no note about cc.h in the patch! :-)

Simon Goldschmidt <goldsimon>
Group administrator
Thu 29 Oct 2009 09:50:00 PM UTC, comment #5: 

I see.  Yes, I thought you meant how (since you said how :-) ) more than where.  Heh, I did find it a bit odd for you to ask that since I knew you knew GCC and probably the _attribute_ thing too.

Those externs and/or #pragmas belong in cc.h right?

Bill Auerbach <billauerbach>
Thu 29 Oct 2009 09:40:59 PM UTC, comment #4: 

Ehrm, I think I might not have made it too clear what I wanted to know... ;-)

I know GCC and its attributes (after all, I'm using lwIP on the same platform as you are - NIOS-II with its GCC). The one thing I didn't understand with this patch is: how do you actually change the section for, say, UDP_PCB? Do you have to edit memp.c for that? I would have thought of defining something in lwipopts.h (like "#define MEMP_UDP_PCB_SECTION _attribute_((section(".onchip_mem")))") and memp.c placing the pool accordingly by some preprocessor magic... ?


> memp_bases could be local - it's kind of unorthodox to use a
> #include within a function


Yeah, didn't think about the include, it's kind of hidden... Probably not a good idea, especially when the array is const (and thus may really be put where the code is).


Simon Goldschmidt <goldsimon>
Group administrator
Thu 29 Oct 2009 08:33:25 PM UTC, comment #3: 

Correction on saving RAM.

u8_t *const memp_bases[];

Keeps the array in code space, not RAM, so I was wrong about it saving space.  Doesn't the static (when put back in) change the scope as well as putting this in the function that uses it?  It also saves another #if/#endif conditional block you'd need in the function if it's there.  No matter - it works either way.

Bill Auerbach <billauerbach>
Thu 29 Oct 2009 08:25:39 PM UTC, comment #2: 

Sure, rename things as you see fit.  I had to do this for a project using GCC so is shown here for GCC.  It would apply to several platforms and any others that use an extern or #pragma to move memory.  It works well - I've finished one product where I moved 4 pools.

Without the extern statements, all pools are in BSS. In order to move one or more the compiler will have a #pragma or _attribute_ like GCC does.  So _attribute_((section( "section_name"))) is GGCs way to move an array to a new section "section_name".  (Maybe this isn't what you were asking?)

extern u8_t _attribute_((section(".onchip_mem"))) memp_TCP_PCB_base[];

moves memp_TCP_PCB_base to section .onchip_mem.  You assign the address of this section however the tool's linker specifies the final address of a section.  In my application, I already had this section and it was placed by the tools for me since .onchip_mem had a fixed address in the FPGA.

static limits the scope - without it you can see the allocations in the MAP file to check them out.  Since I would run out of space in .onchip_mem, I needed to see these arrays to know more about the locations.

memp_bases could be local - it's kind of unorthodox to use a #include within a function - I never thought about that, but it should be fine and does save RAM.

Bill Auerbach <billauerbach>
Thu 29 Oct 2009 08:04:16 PM UTC, comment #1: 

Just to make the code more readable, I'd rather name the separated pools memp_memory_*, i.e. memp_memory_UDP_PCB to stay with the current naming scheme.

How do you define where the pools are placed? Shouldn't there be a define for that somewhere? And why did you include "/*static*/" in the pool definitions? Oh, and I think it would be enough for memp_bases to be a local variable in memp_init() rather than a global variable.

Simon Goldschmidt <goldsimon>
Group administrator
Fri 01 May 2009 01:30:35 PM UTC, original submission:  

Re: https://savannah.nongnu.org/bugs/?26133

This patch places memory pools in separate arrays if MEMP_SEPARATE_POOLS is defined in lwipopts.h

One or more pools can be moved to a different section by preceding the pool definition with an extern declaration.  Code includes commented section for an example.

Patch is against current CVS head (memp.c 1.58)

Bill

Bill Auerbach <billauerbach>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #18070:  memp_separate_pools.patch added by billauerbach (2KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by iordan_neshev (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by billauerbach (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2010-01-10 goldsimon StatusNone Done
        Assigned toNone goldsimon
        Open/ClosedOpen Closed
    2009-05-01 billauerbach Attached File- Added memp_separate_pools.patch, #18070

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code