tasklwIP - A Lightweight TCP/IP stack - Tasks: task #7212, Add Mutex concept in sys_arch

 
 

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

task #7212: Add Mutex concept in sys_arch

Submitter:  Frédéric Bernon <fbernon>
Submitted:  Wed 15 Aug 2007 09:49:27 PM UTC
   
 
Category:  None Should Start On:  Tue 14 Aug 2007 10:00:00 PM UTC
Should be Finished on:  Tue 14 Aug 2007 10:00:00 PM UTC Priority:  1 - Later
Status:  Done Privacy:  Public
Assigned to:  goldsimon Percent Complete:  100%
Open/Closed:  Closed Planned Release:  1.4.0
Effort:  0.00

Jump to the original submission

Sat 13 Feb 2010 05:34:59 PM UTC, comment #14: 

Checked in, thanks.

Simon Goldschmidt <goldsimon>
Group administrator
Sat 13 Feb 2010 05:12:30 PM UTC, comment #13: 

Another patch for using SYS_STATS_INC_USED in unix and win32 ports.

(file #19695)

Stephane Lesage <slesage>
Sat 13 Feb 2010 04:53:38 PM UTC, comment #12: 


Here's a patch for mutex statistics.
It also defines a common STATS_INC_USED macro.




(file #19694)

Stephane Lesage <slesage>
Fri 12 Feb 2010 01:59:55 PM UTC, comment #11: 

Added mutexes in sys.h, much like proposed in comment #4, only that sys_mutex_lock must not fail (returning void) but instead assert if locking fails.

Simon Goldschmidt <goldsimon>
Group administrator
Sat 29 Aug 2009 07:52:43 AM UTC, comment #10: 

Now that 1.4.0 is under construction, where would we use mutexes?

All places that currently use sys_sem_new(1) could use mutexes:
- sockets.c: socksem, selectsem
- mem.c: mem_sem
- memp.c?

But I don't know if it's really desirable to include yet another option to let memp.c use mutexes instead of global int disable...

Anyway, for sockets.c and mem.c I think it's a good idea. Oh, and I'd vote for the function prototypes of Frédéric's proposal in comment #3 unless we change the rest of the functions, too, just to keep it consistent. That way, it's also easy to define it mutexes to binary semaphores if not available (or as standard for easy upgrading).

Simon Goldschmidt <goldsimon>
Group administrator
Wed 29 Aug 2007 09:13:42 AM UTC, comment #9: 

No problem to wait post-1.3.0 (that what I proposed in the initial comment).

Frédéric Bernon <fbernon>
Group Member
Wed 29 Aug 2007 09:00:23 AM UTC, comment #8: 

Unless anyone disagrees I'm going to suggest this is not a 1.3.0 issue.

Kieran Mansley <kieranm>
Group Member
Wed 22 Aug 2007 12:08:06 PM UTC, comment #7: 


> Perhaps it need an explain. In fact, my design choice answers to
> several contraints of my platform, and my first release was
> based on a IT which directly pass packets, with a
> SYS_ARCH_PROTECT based on disable interrupts. But, in my case,
> interrupts have to be really short to have no impact on the
> global performance of the system due to cache management (it's a
> DSP, and there is some tasks which have to be less interrupted as
> possible, and the network sub system is not the one with the
> higher priority, disable interrupts give my stalls delays I have
> to reduce). That's why in my case, my system got a better
> performance with this design than with the one you find better
> for eCos port. But, this is another subject of course, and such
> design choice depend of the target to my point of view.


Sure, that makes sense. In fact with eCos SYS_ARCH_PROTECT only disables tasking, it doesn't disable interrupts. This is enough to stop any re-entrancy (given how the ethernet devices work), but high priority interrupts can still operate.

Anyway, for others, that is not the way they could do it, and adding another thread would be worse for memory use. Adding mutex locks into the memp/pbuf code will force them to.

Maybe Simon's suggestion is the best compromise (although there also needs to be something to match SYS_ARCH_DECL_PROTECT, and to declare the mutex).

Jonathan Larmour <jifl>
Group Member
Wed 22 Aug 2007 06:19:40 AM UTC, comment #6: 

[Frédéric's task design]

That's the same with me: tcpip isn't the highest prio. But it still should be efficient!

About the direction of this task: where exactly do you want to use these semaphores? It seems like Thomas wants to change memp.c to use semaphores, a thing we just removed a while ago, since it is rather inefficient for embedded systems.

Instead, I'd propose to make different defines (e.g. LOCK_MEMP()/UNLOCK_MEMP() or something like that), which could be defined to SYS_ARCH_PROTECT or to file-scope mutexes.

Simon Goldschmidt <goldsimon>
Group administrator
Tue 21 Aug 2007 09:56:13 PM UTC, comment #5: 


>That's rather inefficient. You need a task just to process incoming packets, separate from the tcp/ip task. That's a whole extra thread stack and context switch overhead. I don't think that's what most developers would do. (or at least would want to do if they had an alternative). They'd want to pass the packets directly to the lwip tcpip thread. We must not rely on such a model. My port does not do it for sure. The old eCos port did, but that was very wasteful.


Perhaps it need an explain. In fact, my design choice answers to several contraints of my platform, and my first release was based on a IT which directly pass packets, with a SYS_ARCH_PROTECT based on disable interrupts. But, in my case, interrupts have to be really short to have no impact on the global performance of the system due to cache management (it's a DSP, and there is some tasks which have to be less interrupted as possible, and the network sub system is not the one with the higher priority, disable interrupts give my stalls delays I have to reduce). That's why in my case, my system got a better performance with this design than with the one you find better for eCos port. But, this is another subject of course, and such design choice depend of the target to my point of view.

>err_t sys_mutex_new...


About these functions, I have try to be coherent with current sys_* ones. The error checking should done by comparing the sys_mutex_t with a SYS_MUTEX_NULL for new, like for sem & mbox. Of course, if we talk to redesign all sys functions, this is another story (but, to minimize impact on current source code, you should specify that your sys_mutex_new set to SYS_MUTEX_NULL in case of error, same for sem & mbox). I think we should have a coherence with all sys when/if we introduce mutex. So, saying my proposition is better with current sys.h, and, when/if you change the others (mbox & sem), your proposition will be better...

>Note that I have sys_mutex_lock() returning success/failure. That's because some OSes can return errors on lock (e.g. if they are broken out of the wait). Although I'm not sure whether that's something we should worry about or not. Perhaps we should leave it to the port to just throw its own assertion failure in that case. Thoughts?


In this case, I suppose the problem should be the same for sys_arch_sem_wait? But I think that throw an assert failure inside the sys_arch is the right thing to do in these error cases.


Frédéric Bernon <fbernon>
Group Member
Tue 21 Aug 2007 08:11:17 PM UTC, comment #4: 

Re comment #2:

Yes I guess it could be done like the unix port. It just makes me uncomfortable to optimise for the uncommon case. But I guess it's no big deal.

>>The problem with mutexes in some places like the memp code is
>> that, for example, you may be doing a pbuf_alloc(PBUF_POOL,...
>> from an interrupt context (due to a new incoming packet), and
>> in an interrupt context you cannot block. Which means you can't
>> block on a mutex.
>
> I suppose the solution I use is the same for most of
> developers:
> I got a Rx-Task which wait a Rx-event. The Rx-Interrupt disable
> the RX interruption, and signal the Rx-event. So, the Rx-Task
> is wake-up, process all incoming packets, put them in
> PBUF_POOL, and last, enable the RX interruption. It allow to
> process in the thread the job the IT should do (a kind of DPC
> instead of ISR processing).


That's rather inefficient. You need a task just to process incoming packets, separate from the tcp/ip task. That's a whole extra thread stack and context switch overhead. I don't think that's what most developers would do. (or at least would want to do if they had an alternative). They'd want to pass the packets directly to the lwip tcpip thread.

We must not rely on such a model. My port does not do it for sure. The old eCos port did, but that was very wasteful.

That's why SYS_ARCH_PROTECT exists for memp and pbuf allocation.  Otherwise it would have always used only semaphores.

Re comment #3: While it is consistent with the sys API, it suffers the same problem as the current sys API as it implies dynamic allocation of mutexes, or allocation from a fixed pool. This has caused me problems on my own port meaning calling mem_malloc, which is a bit poor really since most of the mboxes or semaphores in use at present could have their memory allocated statically.

Instead how about:
err_t sys_mutex_new(sys_mutex_t *mutex);
err_t sys_mutex_lock(sys_mutex_t *mutex);
void sys_mutex_unlock(sys_mutex_t *mutex);
void sys_mutex_free(sys_mutex_t *mutex);

where sys_mutex_t is a type defined by the port - probably a straight typedef of the OS's mutex type.

I'd love to change the semaphore and mbox APIs too, but I'll leave that thought for another day.

Note that I have sys_mutex_lock() returning success/failure. That's because some OSes can return errors on lock (e.g. if they are broken out of the wait). Although I'm not sure whether that's something we should worry about or not. Perhaps we should leave it to the port to just throw its own assertion failure in that case. Thoughts?

Jonathan Larmour <jifl>
Group Member
Tue 21 Aug 2007 07:17:42 PM UTC, comment #3: 

/* Mutex functions. */
sys_mutex_t sys_mutex_new();
void sys_mutex_lock(sys_mutex_t mutex);
void sys_mutex_unlock(sys_mutex_t mutex);
void sys_mutex_free(sys_mutex_t mutex);

could be prototypes for sys_arch...

Frédéric Bernon <fbernon>
Group Member
Tue 21 Aug 2007 06:21:50 PM UTC, comment #2: 


>Yes mutexes might help, although not all systems have recursive mutexes. I'm interested to see that recursive mutexes are in SUS2. They are not in POSIX 1003.1c-1995, so they aren't that portable in practice.


Yes, but since it would be defined in sys_arch.c, for OS without native reentrant mutexes, ports maintainers can implement reentrance like in the unix port, right? So, I think we can integrate this concept.

>The problem with mutexes in some places like the memp code is that, for example, you may be doing a pbuf_alloc(PBUF_POOL,... from an interrupt context (due to a new incoming packet), and in an interrupt context you cannot block. Which means you can't block on a mutex.


I suppose the solution I use is the same for most of developers: I got a Rx-Task which wait a Rx-event. The Rx-Interrupt disable the RX interruption, and signal the Rx-event. So, the Rx-Task is wake-up, process all incoming packets, put them in PBUF_POOL, and last, enable the RX interruption. It allow to process in the thread the job the IT should do (a kind of DPC instead of ISR processing).

Frédéric Bernon <fbernon>
Group Member
Wed 15 Aug 2007 11:20:03 PM UTC, comment #1: 

That seems reasonable. We can provide a default mutex implementation based on semaphores, and leave it to the developer to override if he so chooses.

Jared Grubb <jgrubb>
Group Member
Wed 15 Aug 2007 09:49:27 PM UTC, original submission:  

I would like to know your point of view about the fact to add the "mutex" concept in sys_arch (in a post-1.3.0)?

There is already the sys_arch_protect/sys_arch_unprotect feature, which could be very useful since like it's said in sys_arch.txt "...This function should support recursive calls from the same task or interrupt. In other words, sys_arch_protect() could be called while already protected...". But there is a limitation since the "protect" is global, and we can have several ones in the source code. More, since this one is global, it can be used for locking the core. Actually, most of semaphores in the code are used like mutex...

Your comments ?

Frédéric Bernon <fbernon>
Group Member

 

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

Attached Files
file #19695:  sys_stats_unix_win32.patch added by slesage (5KiB - application/octet-stream)
file #19694:  stats.patch added by slesage (4KiB - 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 slesage (Updated the item)
  • -email is unavailable- added by kieranm (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 jgrubb (Posted a comment)
  • -email is unavailable- added by fbernon (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 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2010-02-13 slesage Attached File- Added sys_stats_unix_win32.patch, #19695
    2010-02-13 slesage Attached File- Added stats.patch, #19694
    2010-02-12 goldsimon StatusNone Done
        Percent Complete0% 100%
        Assigned toNone goldsimon
        Open/ClosedOpen Closed
    2008-03-25 kieranm Planned ReleaseNone 1.4.0
    2007-11-02 fbernon Priority5 - Normal 1 - Later

    Back to the top

    Powered by Savane 3.13-caa5.
    Corresponding source code