[patch #6822] Patch places memory pools in separate arrays

[Sylvain Beucler-4]

URL:
  <http://savannah.nongnu.org/patch/?6822>

                 Summary: Patch places memory pools in separate arrays
                 Project: lwIP - A Lightweight TCP/IP stack
            Submitted by: billauerbach
            Submitted on: Fri 01 May 2009 09:30:35 AM EDT
                Category: None
                Priority: 5 - Normal
                  Status: None
                 Privacy: Public
             Assigned to: None
        Originator Email:
             Open/Closed: Open
         Discussion Lock: Any
         Planned Release: None

    _______________________________________________________

Details:

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



    _______________________________________________________

File Attachments:


-------------------------------------------------------
Date: Fri 01 May 2009 09:30:35 AM EDT  Name: memp_separate_pools.patch  Size:
2kB   By: billauerbach

<http://savannah.nongnu.org/patch/download.php?file_id=18070>

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?6822>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
lwip-devel@...
http://lists.nongnu.org/mailman/listinfo/lwip-devel

[Sylvain Beucler-4]

Follow-up Comment #1, patch #6822 (project lwip):

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.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?6822>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
lwip-devel@...
http://lists.nongnu.org/mailman/listinfo/lwip-devel

[Sylvain Beucler-4]

Follow-up Comment #2, patch #6822 (project lwip):

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.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?6822>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
lwip-devel@...
http://lists.nongnu.org/mailman/listinfo/lwip-devel

[Sylvain Beucler-4]

Follow-up Comment #3, patch #6822 (project lwip):

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.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?6822>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
lwip-devel@...
http://lists.nongnu.org/mailman/listinfo/lwip-devel

[Sylvain Beucler-4]

Follow-up Comment #4, patch #6822 (project lwip):

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).



    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?6822>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
lwip-devel@...
http://lists.nongnu.org/mailman/listinfo/lwip-devel

[Sylvain Beucler-4]

Follow-up Comment #5, patch #6822 (project lwip):

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?

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?6822>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
lwip-devel@...
http://lists.nongnu.org/mailman/listinfo/lwip-devel

[Sylvain Beucler-4]

Follow-up Comment #6, patch #6822 (project lwip):

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! :-)

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?6822>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
lwip-devel@...
http://lists.nongnu.org/mailman/listinfo/lwip-devel

[Sylvain Beucler-4]

Follow-up Comment #7, patch #6822 (project lwip):

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


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?6822>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
lwip-devel@...
http://lists.nongnu.org/mailman/listinfo/lwip-devel

[Sylvain Beucler-4]

Follow-up Comment #8, patch #6822 (project lwip):

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?

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?6822>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
lwip-devel@...
http://lists.nongnu.org/mailman/listinfo/lwip-devel

[Sylvain Beucler-4]

Follow-up Comment #9, patch #6822 (project lwip):

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.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?6822>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
lwip-devel@...
http://lists.nongnu.org/mailman/listinfo/lwip-devel

[Sylvain Beucler-4]

Follow-up Comment #10, patch #6822 (project lwip):

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[]

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?6822>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
lwip-devel@...
http://lists.nongnu.org/mailman/listinfo/lwip-devel

[Sylvain Beucler-4]

Follow-up Comment #11, patch #6822 (project lwip):

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.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?6822>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
lwip-devel@...
http://lists.nongnu.org/mailman/listinfo/lwip-devel

[Sylvain Beucler-4]

Follow-up Comment #12, patch #6822 (project lwip):

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.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?6822>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
lwip-devel@...
http://lists.nongnu.org/mailman/listinfo/lwip-devel

[Sylvain Beucler-4]

Follow-up Comment #13, patch #6822 (project lwip):

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.  

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?6822>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
lwip-devel@...
http://lists.nongnu.org/mailman/listinfo/lwip-devel

[Sylvain Beucler-4]

Follow-up Comment #14, patch #6822 (project lwip):

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.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?6822>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
lwip-devel@...
http://lists.nongnu.org/mailman/listinfo/lwip-devel

[Sylvain Beucler-4]

Follow-up Comment #15, patch #6822 (project lwip):

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


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?6822>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
lwip-devel@...
http://lists.nongnu.org/mailman/listinfo/lwip-devel