tasklwIP - A Lightweight TCP/IP stack - Tasks: task #7136, Centralize mempool settings in...

 
 

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

task #7136: Centralize mempool settings in separate file

Submitter:  Jared Grubb <jgrubb>
Submitted:  Wed 01 Aug 2007 12:26:33 AM UTC
   
 
Category:  None Should Start On:  Tue 31 Jul 2007 07:00:00 AM UTC
Should be Finished on:  Tue 31 Jul 2007 07:00:00 AM UTC Priority:  3 - Low
Status:  Done Privacy:  Public
Assigned to:  jgrubb Percent Complete:  100%
Open/Closed:  Closed Planned Release:  1.3.0
Effort:  0.00

Jump to the original submission

Wed 29 Aug 2007 09:03:11 AM UTC, comment #23: 

Closing as no problems raised.

Kieran Mansley <kieranm>
Group Member
Mon 20 Aug 2007 04:43:03 PM UTC, comment #22: 


> should be ... #include "lwip/memp_std.h"


Yes, sorry for the mistake.

Jared Grubb <jgrubb>
Group Member
Mon 20 Aug 2007 03:47:51 PM UTC, comment #21: 

Isn't it a mistake in stats.c ?

#include "lwip/memp.def"

should be ...

#include "lwip/memp_std.h"

I suppose. Right?

Frédéric Bernon <fbernon>
Group Member
Fri 17 Aug 2007 06:27:58 PM UTC, comment #20: 

The changes have been commited to CVS. Let me know how the new changes are, and if all's good, we can close this!

Jared Grubb <jgrubb>
Group Member
Wed 15 Aug 2007 03:53:52 PM UTC, comment #19: 


> I like the idea of auto-configurable and user-define additional pools, but I don't exactly understand why this has to be in a separate file instead of lwipopts.h!


The issue is that the stuff in lwippools.h must be "re-include-able". For example, memp.c has to include this file 3 separate times. It would be possible to give a structure to lwipopts.h that would make some parts of it re-includable, but I think that would make things more confusing.

I suggested an optional file called lwippools.h so that it at least looks like it is the twin brother of lwipopts.h.

Jared Grubb <jgrubb>
Group Member
Wed 15 Aug 2007 06:04:15 AM UTC, comment #18: 

I like the idea of auto-configurable and user-define additional pools, but I don't exactly understand why this has to be in a separate file instead of lwipopts.h!

I think one configuration file should be enough, shouldn't it?

Simon Goldschmidt <goldsimon>
Group administrator
Tue 14 Aug 2007 05:48:54 PM UTC, comment #17: 

There hasn't been much comment on this feature in a while, so I will post the latest memp_std.h file, which checks a MEMP_USE_CUSTOM_POOLS flag to determine whether it should attempt to include lwippools.h (a custom user file).

To summarize the changes:

  • memp_std.h - as attached
  • opt.h - patch attached
  • memp.c - (previously attached)
  • memp.h - (previously attached)
  • mem.c - (previously attached)
  • stats.c - (previously attached)


What do you all think?

(file #13665, file #13666)

Jared Grubb <jgrubb>
Group Member
Fri 03 Aug 2007 04:11:52 PM UTC, comment #16: 


> I think that would be a overly complex for this, at least initially. Adding extra pools isn't likely to be a common task, and is also likely to be done by those who know what they're doing, so I don't mind if that involves modifying source rather than configuration.


I think this is considered "common" because this patch solves Task #6863 (and replaces that patch and removes all the MEM_POOL_SIZE_* options) with an easy way for users to make an arbitrary number of such pools for mem_malloc.


Jared Grubb <jgrubb>
Group Member
Fri 03 Aug 2007 08:06:38 AM UTC, comment #15: 

Re: comment #13...

I think that would be a overly complex for this, at least initially.  Adding extra pools isn't likely to be a common task, and is also likely to be done by those who know what they're doing, so I don't mind if that involves modifying source rather than configuration.

Kieran Mansley <kieranm>
Group Member
Thu 02 Aug 2007 07:03:15 PM UTC, comment #14: 

Attached are updated files:

  • memp_std.h - the "standard" pools, now includes checking for TCP, UDP, etc. (there is no checking for NETBUF, NETCONN, or TCPIP_MSG)
  • lwippools.h - the one I am using, for example
  • memp.c/h - removed the MEMP_TYPE_SIZE macro altogether, changed reference to memp_std.h


(these have no checking for the MEMP_USE_* flags that I proposed last comment)

(file #13555, file #13556, file #13557, file #13558)

Jared Grubb <jgrubb>
Group Member
Thu 02 Aug 2007 05:36:46 PM UTC, comment #13: 

I personally like seeing an explicit list of what the mempools are. But, you do bring up a valid argument: what happens if the user does minimal configuration. All the defaults should "just work" for the user. Also, if a new feature is added into CVS that requires a change to the lwippools.h, but the file is exclusively in the user's domain, then we have a maintanence issue.

If we leave the MEMP_NUM_* options in place, then I would suggest pulling out the standard pools into a lwip/include/memp_std.h file.  Then we create two new options:

  • MEMP_USE_STD_POOLS (default 1) to give the user an ability to disable all the standard pools and explicitly make them himself
  • MEMP_USE_CUSTOM_POOLS (default 0) to indicate that he wants to add custom pools to the list (e.g., the mem_malloc pools)

If MEMP_USE_STD_POOLS==0 or MEMP_USE_CUSTOM_POOLS==1, then the lwippools.h file will be included.

As far as dependencies and checking, I think that would be a great idea. We could create a header file whose sole job is to do all the sanity checks on options that we might want.

Jared Grubb <jgrubb>
Group Member
Thu 02 Aug 2007 08:09:21 AM UTC, comment #12: 

Agreed about keeping all the user-tunable stuff in one place: I know the documentation that Jared is working on will help this a lot, but there is lots of confusion already among new users about how to configure the stack, so simple, easy, and in one place is better.

Checking dependencies is also a good idea, but essentially a separate issue.  Feel free to raise a bug/task for that.

I like the aesthetic changes in comment #9

Kieran Mansley <kieranm>
Group Member
Thu 02 Aug 2007 05:42:14 AM UTC, comment #11: 


>> I think we could/should remove from opts.h and lwipopts.h all
>> MEMP_NUM_xxx and defined them directly in the same file.
>
> I have removed all MEMP_NUM_* from memp.def and changed them to
> the opt.h default values. I agree that it is better to keep the
> pool settings inside this file, and will make lwipopts.h and
> opt.h shorter and simpler.


Just my opinion, but I don't think that's a good idea. Firstly the idea, I thought, was to keep opt.h pristine, and the user only overrides what they want to change in lwipopts.h. So you can have different applications requiring different configs, but built from the same lwIP sources. This way they'd have to change the original memp.def too. Secondly, I think it's better that a user can find all the options they may want to tune in the same place, rather than in multiple different files - especially not one that looks like code. If you want to split up opt.h, I recommend making new files in a different subdirectory like include/lwip/config/ so at least it's clear that that place has the places with user-settable options. But it should still be overrideable from a single lwipopts.h.

One other good reason is that there are dependencies between the options, and things we should be sanity checking in a debug build (but don't yet): e.g. MEMP_NUM_RAW_PCB depends on LWIP_RAW, MEMP_NUM_UDP_PCB depends on LWIP_UDP, etc. There are some constraints we don't check at present but should e.g. MEMP_NUM_TCP_SEG should be at least as big as TCP_SND_QUEUELEN, MEMP_NUM_NETCONN should be at most the sum of MEMP_NUM_{TCP,RAW,UDP}_PCB, MEMP_NUM_NETBUF should be at most the sum of MEMP_NUM_PBUF_POOL and MEMP_NUM_PBUF, and so on.

Jonathan Larmour <jifl>
Group Member
Thu 02 Aug 2007 12:31:15 AM UTC, comment #10: 

Bugfix in memp.c version previously posted: need to change the MEMP_TYPE_SIZE macro! Yowzers that one hurt.

(file #13550)

Jared Grubb <jgrubb>
Group Member
Wed 01 Aug 2007 09:48:42 PM UTC, comment #9: 

As for aesthetics, perhaps:

  • Change the name of memp.def file to something like lwippools.h so that it has an .h extension (for syntax highlighting) and looks like it belongs next to lwipopts.h in the user's custom includes directory.
  • Possibly pull out the setup/cleanup code of memp.def into a second header file inside src/include/lwip. That way the user's lwippools.h is just a list of LWIP_MEMPOOL/LWIP_MALLOC_MEMPOOL definitions and is very clean to read. Also makes it harder for the user to accidentally mess up what needs to be there to make things work.
Jared Grubb <jgrubb>
Group Member
Wed 01 Aug 2007 09:33:09 PM UTC, comment #8: 

Here are updates to the changes to the memp.def and memp.h files, and new changes for mem.c.

> I was talking about the possibility to use that to fix Simon's problem of task #6863 (be able to add several kind of "mem poll" - actually, fixed to 4).


These new changes will allow an arbitrary number of "mem pools" and solves Task #6863. The way to find the first and last "mem pool" is a bit strange, but this is the only thing I could think of without using a const variable.

I have verified that the MEMP_POOL_FIRST/LAST are set correctly, but I don't use the mem_malloc. So someone that is using those pools should verify that it is working on their end as well.

> I think we could/should remove from opts.h and lwipopts.h all MEMP_NUM_xxx and defined them directly in the same file.


I have removed all MEMP_NUM_* from memp.def and changed them to the opt.h default values. I agree that it is better to keep the pool settings inside this file, and will make lwipopts.h and opt.h shorter and simpler.

(file #13543, file #13544, file #13545)

Jared Grubb <jgrubb>
Group Member
Wed 01 Aug 2007 03:43:52 PM UTC, comment #7: 


>I don't see that - memp.def only refers to the MEMP_NUM_* numbers from lwipopts, it doesn't replace them.


I was talking about the possibility to use that to fix Simon's problem of task #6863 (be able to add several kind of "mem poll" - actually, fixed to 4).

Frédéric Bernon <fbernon>
Group Member
Wed 01 Aug 2007 02:42:15 PM UTC, comment #6: 


> The only "bad" point is we have now two "app/port" specific
> tuning files: lwipopts.h, and this new one...


I don't see that - memp.def only refers to the MEMP_NUM_* numbers from lwipopts, it doesn't replace them.

Jonathan Larmour <jifl>
Group Member
Wed 01 Aug 2007 02:36:00 PM UTC, comment #5: 

I feel that like a nice solution (perhaps can be used for Simon's problem in https://savannah.nongnu.org/task/index.php?6863).

>Any chance that porters might hit problems if their preprocessor doesn't support this sort of thing?


I don't think, but, it seems to be a standard preprocessing.

The only "bad" point is we have now two "app/port" specific tuning files: lwipopts.h, and this new one...

I think we could/should remove from opts.h and lwipopts.h all MEMP_NUM_xxx and defined them directly in the same file. Else, to tune the pools, we should have to change lwipopts.h AND this one.

Frédéric Bernon <fbernon>
Group Member
Wed 01 Aug 2007 02:22:46 PM UTC, comment #4: 

Am I right in thinking that the main benefit of this is slightly smaller code?

I'm a bit hesitant about it, but for no good reason other than I shy away from using the preprocessor in clever ways like this!

Any chance that porters might hit problems if their preprocessor doesn't support this sort of thing?

Kieran Mansley <kieranm>
Group Member
Wed 01 Aug 2007 02:02:42 PM UTC, comment #3: 

Doh! I missed the stats.c patch that is there. In that case, if you haven't heard from anyone else in the next couple of days, I'd say just go ahead and check in.

Jonathan Larmour <jifl>
Group Member
Wed 01 Aug 2007 02:01:39 PM UTC, comment #2: 

Seems like a good idead to me. However there needs to be something in stats.c as well to bring that in line.

Since it should be no semantic change, I would have thought this would be ok before 1.3, but I don't know what the criteria are since there's an apparent informal feature freeze.

Jonathan Larmour <jifl>
Group Member
Wed 01 Aug 2007 12:32:46 AM UTC, comment #1: 

Sorry, COPY-PASTE error. The fixed lines are (removed the + from the end of the first line shown here):

// bad version (actually still compiles, but should be changed)
static u8_t memp_memory[MEM_ALIGNMENT - 1 +
#define LWIP_MEMPOOL(name,num,size,desc) + MEMP_TYPE_SIZE(num,size)

// fixed
static u8_t memp_memory[MEM_ALIGNMENT - 1
#define LWIP_MEMPOOL(name,num,size,desc) + MEMP_TYPE_SIZE(num,size)

Jared Grubb <jgrubb>
Group Member
Wed 01 Aug 2007 12:26:33 AM UTC, original submission:  

I would like to propose using the C preprocessor to generate the mempool pieces:

EXPLANATION: There will be a new file called memp.def (or something else if you like). It will contain a list of all the pools that will be managed by memp.c. The memp.c, memp.h, and stats.c files will use this file to construct what they need.

ADVANTAGE: Adding new mempools is very easy -- one line of code and all three files get updated automatically. As an added feature, the developer can customize this file and add in his own extra pools (for example mbox pools for sys_mbox_new, or even other pools for his applications)

NOTE: This uses the x-macro pattern in C. See http://en.wikipedia.org/wiki/C_preprocessor#X-Macros for an explanation of why this works.

I have tested this on my system, and the memp_sizes[], memp_num[] memp_t, and memp_memory do not change as a result of this change.

Jared Grubb <jgrubb>
Group Member

 

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

Attached Files
file #13665:  memp_std.h added by jgrubb (3KiB - text/plain)
file #13666:  opt.h.patch added by jgrubb (2KiB - text/x-patch)
file #13555:  memp.c added by jgrubb (10KiB - text/plain)
file #13556:  memp_std.h added by jgrubb (2KiB - text/plain)
file #13557:  lwippools.h added by jgrubb (706B - text/plain)
file #13558:  memp.h added by jgrubb (3KiB - text/plain)
file #13550:  memp.c.patch added by jgrubb (3KiB - text/x-patch)
file #13543:  memp.def added by jgrubb (3KiB - text/plain)
file #13544:  mem.c.patch added by jgrubb (586B - text/x-patch)
file #13545:  memp.h.patch added by jgrubb (2KiB - text/x-patch)
file #13536:  memp.def added by jgrubb (2KiB - text/plain)
file #13537:  memp.h.patch added by jgrubb (903B - text/x-patch)
file #13538:  memp.c.patch added by jgrubb (3KiB - text/x-patch)
file #13539:  stats.c.patch added by jgrubb (854B - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by fbernon (Posted a comment)
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by jifl (Posted a comment)
  • -email is unavailable- added by jgrubb (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 21 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2007-08-29 kieranm StatusReady For Test Done
        Open/ClosedOpen Closed
    2007-08-17 jgrubb StatusNone Ready For Test
        Percent Complete0% 100%
        Assigned toNone jgrubb
        Planned ReleaseNone 1.3.0
    2007-08-14 jgrubb Attached File- Added memp_std.h, #13665
        Attached File- Added opt.h.patch, #13666
    2007-08-02 jgrubb Attached File- Added memp.c, #13555
        Attached File- Added memp_std.h, #13556
        Attached File- Added lwippools.h, #13557
        Attached File- Added memp.h, #13558
    2007-08-02 jgrubb Attached File- Added memp.c.patch, #13550
    2007-08-01 jgrubb Attached File- Added memp.def, #13543
        Attached File- Added mem.c.patch, #13544
        Attached File- Added memp.h.patch, #13545
    2007-08-01 kieranm Priority5 - Normal 3 - Low
    2007-08-01 jgrubb Attached File- Added memp.def, #13536
        Attached File- Added memp.h.patch, #13537
        Attached File- Added memp.c.patch, #13538
        Attached File- Added stats.c.patch, #13539

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code