Versions Compared

Key

  • This line was added.
  • This line was removed.
  • Formatting was changed.

Files: 

  • cpsw_buf.h
  • cpsw_buf.cc
  • cpsw_bufq.cc

Classes:

  • Public:
    • IBuf: Single buffer in a linked chain
    • IBufChain: Linked chain of IBufs
    • IBufQueue; Queue container for IBufChains
    • Buf (IBuf in shared pointer)
    • IBufChain: Linked chain of IBufs
    • BufChain (IBufChain in shared pointer)
    • IBufQueue; Queue container for IBufChains
    • BufQueue (IBufQueue in shared pointer)
  • Private:
    • CBufImpl: Implementation of IBuf with ability to be a CFreeListNode
    • BufImpl (CBufImpl in shared pointer)
    • CBufChainImpl: Implementation of IBufChain with ability to be a CFreeListNodeBufImpl (CBufImpl in shared pointer)
    • BufChainImpl (CBufChainImpl in shared pointer)

IBuf class methods implemented in CBufImpl:

  • Buf getBuf(size_t capa, bool clip = false): Get a new buffer from free list or create new. Reduced to allowed size if size exceeds max available buffer when clip = true.
  • numBufsAlloced(): Buffers created from heap.
  • numBufsFree(): Total on free list.
  • numBufsInUse(): Number of buffers currently used

IBuf virtual methods implemented in CBufImpl:

...

    • CBufQueue: implementation of IBufQueue
    • IBufSync:
    • BufSync: (IBufSync in shared pointer)
    • CBufQueueBase (typedef of queue: 

      typedef queue< IBufChain *, boost::lockfree::fixed_sized< true > > CBufQueueBase;)

    • CEventBufSync

    • CBufQueue

Buf

CBufImpl private variables:

...

  • void addToChain(BufChainImpl c); Add this buffer, and the rest of the list, to an empty chain
  • void reinit(): Reset the payload pointers (beg = end = HEADROOM)
  • BufImpl getNextImpl(): Get next entry
  • BufImpl getPrevImpl(): Get prev entry
  • BufChainImpl getChainImpl(): Get chain pointer
  • unlinkCheap(CBufChainImpl::key k): 

IBufChain class methods implemented in CBufChainImpl:

...

BufChain

...

IBufChain virtual methods implemented in CBufChainImpl:

...

...

CBufChainImpl private variables:

...

  • BufImpl getHeadImpl(): get head of list
  • BufImpl getTailImpl(): get tail of list
  • void addSIze(ssize_t s) add bytes to total size
  • void addLen(int l) add entries to total length
  • setSize(size_t s): set total size in bytes
  • setLen(unsigned l): set buffer length

BufQueue

IBufQueue class methods:

  • BufQueue create(unsigned size): create an object

IBufQueue virtual methods:

  • BufChain pop(const CTimeout *abs_timeout): Pop an entry from the queue with timeout
  • BufChain tryPop(): try to pop without timeout?
  • bool push(BufChain b, const CTimeout *abs_timeout): add an entry to the queue with timeout
  • bool tryPush(BufChain b): add an entry without timeout?
  • CTimeout getAbsTimeoutPop(const CTimeout *rel_timeout): ?????
  • CTimeout getAbsTimeoutPush(const CTimeout *rel_timeout): ????
  • IEventSource getReadEventSource(): ????
  • IEventSource getWriteEventSource(): ????
  • bool isFull(): queue is full
  • bool isEmpty() queue is full
  • void shutdown(): ????
  • void startup(): ????

CBufQueue private attributes



Questions:

Till,

I am going through the CPSW code and have some questions and comments, starting with the CPSW buffer implementation. BTW I like the approach to the buffer chains which minimizes the number of copies to the copy from the UDP receive and the copy to user space. 

1. It appears you base the buffer size constants and container variables around the assumption that we will limit each buffer element size to 64K. I am not sure if that is a safe assumption once we add support for interfaces that are not TCP/IP or UDP. With PGP or axi-stream we can have much larger buffers. Also you seem to hardcode some ethernet based constants in the .h file and I am not sure that is the best place for a generic implementation. (more notes on size later)

2. In adjPayload you have this line:

if ( delta > beg || delta > old_size )
return false;

Why do you care if the shift size is greater than the beginning payload offset? Should this be an unsigned compare to check for left shifts beyond the beginning of the buffer?

...

Lastly a code convention question. Is it standard practice to declare classes, even internal ones, within the .cc files? It is hard to use tools such as doxygen with this approach. It is also hard to follow the code structure. 

9. Additional notes on question. It seems that a public free list
allocator would also make sense to set the headroom size for each buffer
at creation. Different implementations will need different headroom
depending on protocol. Of course one could just allocated a large enough
space for all possibilities, but it is hard to anticipate that and some
headroom needs may be excessive compared to the norm.

10. Perhaps the IBufChain::insert functions should include a passed
value which determines the amount of space to copy to each buffer. It
seems now that the max possible size is used for each buffer.