buglwIP - A Lightweight TCP/IP stack - Bugs: bug #51908, PPPOS uses PBUF_POOL in transmit...

 
 

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

bug #51908: PPPOS uses PBUF_POOL in transmit pathway

Submitted by:  Joel Cunningham <jcunningham>
Submitted on:  Thu 31 Aug 2017 07:12:36 PM UTC  
 
Category:  PPP Severity:  3 - Normal
Item Group:  Change Request Status:  Fixed
Privacy:  Public Assigned to:  Simon Goldschmidt <goldsimon>
Open/Closed:  Closed Planned Release:  None
lwIP version:  git head

( Jump to the original submission)

Thu 14 Jun 2018 01:50:32 PM UTC, comment #8:

> Also, remember that PBUF_POOL is reserver for RX only to not starve TCP if it cannot receive ACKs any more. This cannot be the case here, as the buffer is allocated and directly freed.


Agreed, the short lifetime of the PBUF_POOL won't cause RX starvation

> ...but I don't know if this is worth it...


Agreed, I originally brought the issue to the list because it was confusing that we violated our own rule (PBUF_POOL is only for RX) but you've added some comments explaining the situation, so hopefully future devs aren't confused

Joel Cunningham <jcunningham>
Project Member
Thu 14 Jun 2018 01:34:19 PM UTC, comment #7:

Yes, I know. And yes, the author has made up a new mechanism to allocate a buffer to encode PPP data. There is absolutely no need for this buffer to be a pbuf. However, using a PBUF_POOL here is better (I think) than using PBUF_RAM because of the smaller allocation overhead.

Also, remember that PBUF_POOL is reserver for RX only to not starve TCP if it cannot receive ACKs any more. This cannot be the case here, as the buffer is allocated and directly freed.

If we really wanted to clean this up, we would have to use some kind of static/global PPP encoding buffer, but I don't know if this is worth it...

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Thu 14 Jun 2018 01:28:17 PM UTC, comment #6:

I think you can still use PBUF_RAM here, we just need to choose a pre-allocated size and maybe introduce a define for it. See pppos_output_append() where it uses PBUF_POOL_BUFSIZE to determine the maximum size

From what I can tell, there's nothing about PBUF_POOL_BUFSIZE that is specific to pppos. The author must have chosen it rather than introduce a new mechanism

Joel Cunningham <jcunningham>
Project Member
Wed 13 Jun 2018 07:38:54 AM UTC, comment #5:

Thanks for testing, should be fixed. After all, using PBUF_POOL is ok in this very special case as here we know the pbuf is not kept allocated and thus cannot starve rx.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Wed 13 Jun 2018 07:06:11 AM UTC, comment #4:

I just found PPPoS stop working after below commit:
commit 31bc2f9b207e fixed bug #51908: PPPOS uses PBUF_POOL in transmit pathway

Axel Lin <axellin>
Project Member
Tue 12 Jun 2018 08:40:12 AM UTC, comment #3:

Done as suggested in comment #2.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Fri 17 Nov 2017 08:53:44 PM UTC, comment #2:

Sylvain, if you have no objections, I'll just change those into PBUF_RAM.

There's separate work under way to mark the code that must have PBUF_RAM in once piece, so that the rest can use a pool resulting in chained pbufs (PBUF_POOL-like).

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Thu 31 Aug 2017 07:58:37 PM UTC, comment #1:

Places are:
- pppos.c: pppos_write()
- pppos.c: pppos_netif_output()
- vj.c: vj_compress_tcp()

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Thu 31 Aug 2017 07:12:36 PM UTC, original submission:

See discussion here: http://lists.nongnu.org/archive/html/lwip-devel/2017-08/msg00150.html

Initial consensus is that PBUF_RAM should be used in place of PBUF_POOL for transmits

Joel Cunningham <jcunningham>
Project Member

 

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by axellin (Posted a comment)
  • -email is unavailable- added by dziegel (Updated the item)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by jcunningham (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 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-06-12 goldsimon StatusNone => Fixed
        Assigned togradator => goldsimon
        Open/ClosedOpen => Closed
    2017-11-05 dziegel Assigned toNone => gradator

    Back to the top


    Powered by Savane 3.3