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

Submitter:  Joel Cunningham <jcunningham>
Submitted:  Thu 31 Aug 2017 07:12:36 PM UTC
   
 
Category:  PPP Severity:  3 - Normal
Item Group:  Change Request Status:  Fixed
Privacy:  Public Assigned to:  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>
Group 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>
Group administrator
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>
Group 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>
Group administrator
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>
Group Member
Tue 12 Jun 2018 08:40:12 AM UTC, comment #3: 

Done as suggested in comment #2.

Simon Goldschmidt <goldsimon>
Group administrator
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>
Group administrator
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>
Group administrator
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>
Group Member

 

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

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)
  •  

    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 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.13-f8d8.
    Corresponding source code