patchlwIP - A Lightweight TCP/IP stack - Patches: patch #7449, allow tcpip callback from...

 
 

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

patch #7449: allow tcpip callback from interrupt with static memory message

Submitted by:  Stephane Lesage <slesage>
Submitted on:  Sat 22 Jan 2011 11:28:28 PM 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)

Sat 14 May 2011 12:31:26 PM UTC, comment #14: 

Checked in.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Fri 13 May 2011 08:43:00 AM UTC, comment #13: 

> I forgot about this


Me too, or else I would have triggered you again since march :-)

> might be worth checking this in before the IPv6 code


Yep, I'll do that.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Fri 13 May 2011 08:37:01 AM UTC, comment #12: 

My apologies: I forgot about this when making the release.  Lets get it in now so it can't be forgotten any longer.  It might be worth checking this in before the IPv6 code so Stephane at least has a point in CVS (albeit not an official release) that is close to 1.4.0, doesn't have any new unstable code added, and yet has this code included. 

Kieran Mansley <kieranm>
Project Member
Fri 13 May 2011 07:37:24 AM UTC, comment #11: 

Sadly, this hasn't made it into 1.4.0, either. I'll check it in these days, now since 1.4.0 is out.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Thu 31 Mar 2011 11:37:48 AM UTC, comment #10: 

I checked my driver, it runs fine.

just use TCPIP_MSG_CALLBACK_STATIC in tcpip_callbackmsg_new()

Stephane Lesage <slesage>
Wed 30 Mar 2011 10:08:43 PM UTC, comment #9: 

Your patch is good globally, but there's a typo:
tcpip_callbackmsg_new() function must create a TCPIP_MSG_CALLBACK_STATIC message and not TCPIP_MSG_CALLBACK.

It compiles, I'll test tomorrow.

Stephane Lesage <slesage>
Wed 30 Mar 2011 08:14:42 PM UTC, comment #8: 

I'd like to review this before committing, but might not get the chance before tomorrow.  Please hold off committing before then

Kieran Mansley <kieranm>
Project Member
Wed 30 Mar 2011 05:14:50 PM UTC, comment #7: 

... and here it is. I'll check it in tomorrow unless there are any objections ;-)

(file #23061)

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Wed 30 Mar 2011 04:49:46 PM UTC, comment #6: 

> struct tcpip_msg* tcpip_alloccallbackmsg(tcpip_callback_fn function, void ctx);
>
> then call in the interrupt:
> err_t tcpip_trycallback(struct tcpip_msg* msg);
>
> struct tcpip_msg can be only declared and not defined if we decide to hide all internal stuff in the future ?


I like that idea, although I would introduce another struct for this to prevent mixing the types. Plus we would nedd a freecalbackmsg() function.

I'll prepare a patch for this.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Tue 29 Mar 2011 08:45:33 PM UTC, comment #5: 

>However, I think the function tcpip_trypost() would have to be changed to something more like tcpip_callback() because it should
>a) resemble that function if it does similar things and


ok

>b) not require the "user" to think about the contents of 'struct tcpip_msg'.
>In fact, tcpip_msg should not even be in that header file at all.


I like this idea to keep internals hidden from the user.
(there is work to do in this area, splitting header files into internal and public API, but this is another story for 1.4.1)

>Instead, I suggest to change tcpip_trypost() to:
>err_t tcpip_trycallback(tcpip_callback_fn function, void ctx, struct tcpip_msg msg_buffer);


I see some problems with this:
a. we will fill the same structure again and again for nothing
b. the user may think it can make multiple calls to different callbacks with the same msg_buffer.
(we typically need PHY and MAC callbacks and maybe split RX and TX)

A simpler solution may be to allocate a message from the pool at init, using a dedicated function:

struct tcpip_msg* tcpip_alloccallbackmsg(tcpip_callback_fn function, void ctx);

then call in the interrupt:
err_t tcpip_trycallback(struct tcpip_msg* msg);

struct tcpip_msg can be only declared and not defined if we decide to hide all internal stuff in the future ?

Stephane Lesage <slesage>
Tue 29 Mar 2011 07:33:29 PM UTC, comment #4: 

I think the idea of this patch is good since it terms of speedup. However, I think the function tcpip_trypost() would have to be changed to something more like tcpip_callback() because it should
a) resemble that function if it does similar things and
b) not require the "user" to think about the contents of 'struct tcpip_msg'.

In fact, tcpip_msg should not even be in that header file at all. It's in there to make the memp-pools work (and it's needed there for the static messages of this patch, too), but that doesn't mean user code should fill in the values into it.

Instead, I suggest to change tcpip_trypost() to:

err_t tcpip_trycallback(tcpip_callback_fn function, void ctx, struct tcpip_msg msg_buffer);

That way, the caller must know the size of struct tcpip_msg to provide the buffer but does not have to fill in the correct parameters.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Tue 29 Mar 2011 07:27:07 PM UTC, comment #3: 

A patch like this would have made it clearer what you wanted.

(file #23054)

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Tue 29 Mar 2011 09:38:24 AM UTC, comment #2: 

>I don't understand the renaming of the message types, which seems to be the bulk of this patch.
>Can you explain what's going on here?
>I get the feeling that the pool/stat bit is really a flag rather than a whole new set of types.
>Perhaps we need a flags field in the tcpip_msg structure.


Hi, yes basically we need a flag to tag the message as static or dynamic. But this forces us to explicitely set it, and it adds a variable to the structure for only 1 bit.

Actually we don't need the flag for all message types.
In the current code, messages types already code implicitely their dynamic/static nature.

So I chose to add a single new type for callback request with a static message. And to make things clearer, rename messages types with explicit reference to their static/dynamic allocation.

(Another option would be to embedded the flag as MSB in the message type and casting)

Stephane Lesage <slesage>
Tue 29 Mar 2011 09:01:35 AM UTC, comment #1: 

I don't understand the renaming of the message types, which seems to be the bulk of this patch. Can you explain what's going on here?  I get the feeling that the pool/stat bit is really a flag rather than a whole new set of types.  Perhaps we need a flags field in the tcpip_msg structure.

Kieran Mansley <kieranm>
Project Member
Sat 22 Jan 2011 11:28:28 PM UTC, original submission:  

When we implement a DMA driver, we need 2 things:
1. free the pbuf after transmit done interrupt
2. pass a received pbuf from interrupt to tcpip_thread

Additionnaly on PHY interrupt, we need
3. call a Defered Callback, and the tcpip_thread is ready for this

Today we can use:
1. tcpip_input
2a. pbuf_free_callback or
2b. pbuf_free with LWIP_ALLOW_MEM_FREE_FROM_OTHER_CONTEXT
3. tcpip_callback (non blocking)

Dynamic allocation and setup of messages is not optimal, or impossible if the user needs to keep interrupts very short.
Furthermore, we need one message per pbuf...

The proposed patch adds a much simpler solution:
- static messages -> the callback will process the whole "packet completed" queue
- new function tcpip_trypost() calling sys_mbox_trypost() on the tcpip_thread mbox
- new message type for callbacks that the tcpip_thread will not release to the pool (exactly like TCPIP_MSG_API and TCPIP_MSG_NETIFAPI)

For clarification, the pach renames message types to TCPIP_POOLMSG_xxx or TCPIP_STATMSG_xxx

Stephane Lesage <slesage>

 

Attached Files
file #23061:  tcpip_trycallback_static.patch added by goldsimon (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 goldsimon (Updated the item)
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by slesage (Submitted the item)
  •  

    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 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2011-05-14 goldsimon StatusNone => Done
        Assigned toNone => goldsimon
        Open/ClosedOpen => Closed
    2011-03-30 goldsimon Attached File- => Added tcpip_trycallback_static.patch, #23061
    2011-03-29 goldsimon Attached File- => Added tcpip-trypost-staticmessages_lessrenaming.patch, #23054
    2011-01-22 slesage Attached File- => Added tcpip-trypost-staticmessages.patch, #22497

    Back to the top


    Powered by Savane 3.5