patchlwIP - A Lightweight TCP/IP stack - Patches: patch #5453, pbuf patch for...


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

patch #5453: pbuf patch for SYS_LIGHTWEIGHT_PROT=0

Submitted by:  Simon Goldschmidt <goldsimon>
Submitted on:  Wed 11 Oct 2006 09:43:56 AM UTC  
Category:  None Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  Simon Goldschmidt <goldsimon> Open/Closed:  Closed
Planned Release:  None

( Jump to the original submission)

Mon 07 May 2007 07:56:17 PM UTC, comment #22: 

As there are no protests coming in about this change, I'm closing it as done. memp.c is still to be done, though, I'm submitting a new patch for this.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Mon 30 Apr 2007 01:19:02 PM UTC, comment #21: 

OK, I'm checking in my patch, then. Note I made some small modifications (moved pbuf_pool_free() into static function like pbuf_pool_alloc() and moved initialization of pbuf_pool from pbuf_init() into new static function pbuf_pool_init()) to make implementation-change of pbuf pool (->memp) easy.

-> This is ready for test now, in my opinion.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Mon 30 Apr 2007 11:13:05 AM UTC, comment #20: 

Assigning to you Simon as I'm unlikely to get a chance to give some time to this in the next week.

Kieran Mansley <kieranm>
Project Member
Sun 29 Apr 2007 10:53:50 AM UTC, comment #19: 

I just realized my last patch was missing SYS_ARCH_(UN)PROTECT() around "p->next = pbuf_pool; pbuf_pool = p;" as the protection in pbuf_free() has changed in the last 2 months, my original patch was a little older. I'm attaching the new patch (which has only 2 lines more).

Just another point where SYS_LIGHTWEIGHT_PROT seems to be interpreted differently: in memp.c, there's the following:

static sys_sem_t mutex;

which means a mutex is used to protect the pools if SYS_LIGHTWEIGHT_PROT=0, which is the opposite of the behaviour in pbuf.c.

In my opinion, the mutex should be removed, resulting in no protection with SYS_LIGHTWEIGHT_PROT=0 and normal/fast protection with SYS_LIGHTWEIGHT_PROT=1. (Also, memp_malloc() could be tuned to shorten the time interrupts are disabled, but that's another issue.)

(file #12620)

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Fri 20 Apr 2007 07:57:41 AM UTC, comment #18: 

Thanks Simon - I'll try and find time to take a look.

Kieran Mansley <kieranm>
Project Member
Thu 19 Apr 2007 06:31:44 PM UTC, comment #17: 

Ooops, the last patch had a modification in dhcp.c included ;-)

Here's the clean version:

(file #12532)

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Thu 19 Apr 2007 06:29:29 PM UTC, comment #16: 

To get back to the original meaning of this patch: we have decided that SYS_LIGHTWEIGHT_PROT=0 means no protection.

The current pbuf.c implementation locks a task and interrupt context from concurrent access to pbuf_alloc (in this special case, a variable is enough as long as interrupts do not pre-empt each other). But it does not lock pbuf_alloc() against pbuf_free(). Also it uses a semaphore in pbuf_free() with SYS_LIGHTWEIGHT_PROT=0 (which is the absolute opposite of pbuf_alloc() :)

So we decided to remove the current SYS_LIGHTWEIGHT_PROT=0 code, since we said SYS_LIGHTWEIGHT_PROT=0 stands for no locking at all (everything in a big loop).

I'm attaching a patch for this.

(file #12531)

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Wed 04 Apr 2007 06:28:52 AM UTC, comment #15: 

Well, Kieran, seems I didn't see your message!

pbuf.c and memp.c both use semaphores with SYS_LIGHTWEIGHT_PROT=0, that should be changed. to no protection when SYS_LIGHTWEIGHT_PROT=0 right?
The question is then, do we need a second protection method like the one currently used in memp.c with SYS_LIGHTWEIGHT_PROT=0? I think we don't, but that's a point we should discuss before making changes.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Tue 03 Apr 2007 04:01:04 PM UTC, comment #14: 

OK, you're right about existing ports. But at least we need some serious documentation that SYS_LIGHTWEIGHT_PROT==0 means no protection at all!

And besides, I think at least we can throw away the code in pbuf.c for SYS_LIGHTWEIGHT_PROT==0 (the u8_t used to 'lock' and the pbuf_pool_free_sem which locks 2 calls to pbuf_free() against each other, which shouldn't happen with our new view of SYS_LIGHTWEIGHT_PROT==0)

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Tue 03 Apr 2007 03:44:02 PM UTC, comment #13: 

Agree with Jonathan about maintaining the name for existing ports, but we should also document it where it is set so that we don't get into this situation of not knowing what it's for again!

The current SYS_LIGHTWEIGHT_PROT=0 broken semaphore code should be removed.  I suggest leaving (some of) the #ifdef in place with comments inside to identify locations where synchronisation might be desirable.

This is assigned to me, so I hope to get round to it soon.

Kieran Mansley <kieranm>
Project Member
Tue 03 Apr 2007 03:34:53 PM UTC, comment #12: 

I'm not sure there's an imperative to change the name (and consequently break existing ports).

I think it's called "lightweight" because it's a simple and direct way to protect critical regions. Other solutions could also be implemented which could cause less system disturbance and work at a finger granularity, but they would not be as simple/lightweight.

It's possible someone might come up with an alternative more complex implementation at some point, in which case SYS_LIGHTWEIGHT_PROT==1 reflects this particular implementation, to allow existing ports to work.

Jonathan Larmour <jifl>
Project Member
Tue 03 Apr 2007 03:23:17 PM UTC, comment #11: 

comment #9, Yes, I'm agree. About name, SYS_SINGLETHREADED_CORE to match the Adam's document terms?

Frédéric Bernon <fbernon>
Project Member
Tue 03 Apr 2007 03:15:47 PM UTC, comment #10: 

Then we should rename it into SYS_HAVE_PROTECTION or something.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Tue 03 Apr 2007 03:05:39 PM UTC, comment #9: 

Re Frederic's comment #8: But if you are completely single-threaded, then you don't need the semaphore protection provided by SYS_LIGHTWEIGHT_PROT=0 in the current code.

Jonathan Larmour <jifl>
Project Member
Tue 03 Apr 2007 12:21:15 PM UTC, comment #8: 

Perhaps like described in "The lwIP single-threaded core" part of ?

Frédéric Bernon <fbernon>
Project Member
Tue 03 Apr 2007 11:18:11 AM UTC, comment #7: 

Hm, but what was Christiaan thinking SYS_LIGHTWEIGHT_PROT=0 is doing? I can only think of one scenario where this can work: if you have not a single context switch, meaning you have only lwip raw api (no OS, no threads) and your packets aren't delivered in interrupt context but through polling the ethernet device. In this special case, you needn't lock the pbuf pool, for example, and you're faster then!

But then, we should agree that SYS_LIGHTWEIGHT_PROT=0 does mean 'no protection' and not 'using semaphores as protection'. And we have to document that!

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Mon 26 Mar 2007 12:01:12 PM UTC, comment #6: 

You might want to run this by Christian, who was a staunch defender of the SYS_LIGHTWEIGHT_PROT=0 code.

As for the task, I think part of the problem is that some of the existing practice is inconsistent (e.g. the SYS_LIGHTWEIGHT_PROT=0 code :-)), so it's not really describing existing practice but rather, laying down principles which may not be satisfied by the current code. So something high level would seem better to me.

Jonathan Larmour <jifl>
Project Member
Mon 26 Mar 2007 10:48:41 AM UTC, comment #5: 

Right, so I think action 1 should be to remove the SYS_LIGHTWEIGHT_PROT=0 code. 

The documentation of what lwIP expects would be a great idea.  I will create a task for this, but don't expect it soon as to be accurate will require me to check a fair bit of code.  Perhaps I should start with something reasonably high level.

Kieran Mansley <kieranm>
Project Member
Fri 23 Mar 2007 05:22:19 AM UTC, comment #4: 

I think if you read my mail of 2006-11-19 22:23 to lwip-users, you'll get my views of what it does :-). Nothing!

In general, there isn't a good description anywhere of what lwIP expects with respect to threads, thread-safety and interactions with drivers. I think someone in a leadership role (Hi Kieran!) should work this out so we can answer Simon's question in comment #3.

I think we should try to get closure on this, but the mailing list is probably a better forum for it. (Maybe it has been in the last few weeks - I've got 497 unread messages due to people's recent burst of activity! Sorry if I'm what I'm saying is already obsoleted by discussions there).

Jonathan Larmour <jifl>
Project Member
Tue 27 Feb 2007 07:26:48 AM UTC, comment #3: 

I think we would have to decide what SYS_LIGHTWEIGTH_PROT should stand for. =1 is clearly a lightweight protection disabeling the interrupts. But what does =0 do? I don't think that's really clear, is it?

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Mon 19 Feb 2007 11:22:35 AM UTC, comment #2: 

That discussion was subsequent to this patch. The discussion tailed off by 2006-11-20 with no resolution. I still believe the SYS_LIGHTWEIGHT_PROT==0 code is at best redundant, as described in probably best detail in my mail of 2006-11-19 22:23

Jonathan Larmour <jifl>
Project Member
Mon 19 Feb 2007 11:02:38 AM UTC, comment #1: 

I recall a lot of discussion concerning SYS_LIGHTWEIGHT_PROT recently.  Did this patch come before that, or is it what was decided on as a result of that discussion? 

Kieran Mansley <kieranm>
Project Member
Wed 11 Oct 2006 09:43:56 AM UTC, original submission:  

If SYS_LIGHTWEIGHT_PROT is defined to 0, pbuf_pool_alloc() and pbuf_pool_free() are not locked against each other.
This patch fixes this by using one semaphore to lock all pbuf operations.

Also, the current locking implementation in pbuf_pool_alloc() is using simple variables and is thus not thread-safe.

ATTENTION: At least for me, using SYS_LIGHTWEIGHT_PROT=0 produces dead-locks if tcpip_tcp_timer() is active. When creating a seperate thread (or timer thread) for this timer, everything works fine.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.


Attached Files
file #10944:  pbuf_sem.patch added by goldsimon (5KiB - 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 fbernon (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by jifl (Posted a comment)
  • -email is unavailable- added by kieranm (Posted a comment)

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.




    Follow 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2007-05-07 goldsimon StatusReady For Test => Done
        Open/ClosedOpen => Closed
    2007-04-30 goldsimon StatusNone => Ready For Test
    2007-04-30 kieranm Assigned tokieranm => goldsimon
    2007-04-29 goldsimon Attached File- => Added pbuf_lightweightprot2.patch, #12620
    2007-04-19 goldsimon Attached File- => Added pbuf_lightweightprot.patch, #12532
    2007-04-19 goldsimon Attached File- => Added pbuf_lightweightprot.patch, #12531
    2007-03-26 kieranm Assigned toNone => kieranm
    2006-10-11 goldsimon Attached File- => Added pbuf_sem.patch, #10944

    Back to the top

    Powered by Savane 3.5