Files: 

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

Classes:

  • Public:
    • IBuf: Single buffer in a linked chain
    • 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 CFreeListNode
    • BufChainImpl (CBufChainImpl in shared pointer)
    • 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:

  • weak_ptr<CBufChainImpl> chain_: Weak pointer to chain containing this buffer
  • BufImpl next_: Next entry (strong pointer)
  • weak_ptr<CBufImpl>: Previous entry (weak pointer)
  • unsigned short beg_, end_, capa_: Beginning of payload, end of payload and capacity
  • uint8_t data_[]: Array of bytes containing data

CBufImpl creator:

  • CBufImpl(CFreeListNodeKey<CbufImpl> k, unsigned short capa):

Sets beginning and end pointers to HEADROOM which is hard coded to rssi / packetizer size

Sets capacity to passed value, passes k to CFreeListNode creator (adding to free list?)

CBufImpl virtual protected methods:

  • void addToChain(BufImpl p, bool before): Add to chain before or after p.
  • void delFromChain(); Remove entry from chain pointers (does not adjust list)

CBufImpl virtual methods:

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

BufChain

CBufChainImpl private variables:

  • BufChainImpl strong_self_: pointer to self used to maintain valid pointer
  • BufImpl head_: head of chain
  • BufImpl tail_: tail of chain
  • unsigned len_t: length of chain
  • size_t size_: total size in bytes

CBufChainIml public class variables:

  • CFreeList<CBufChainImpl> freeList (this must be set externally before calling create class method)

CBufChainImpl creator:

  • CBufChainImpl( CFreeListNodeKey<CBufChainImpl> k );

CBufChainImpl virtual protected methods:

  • void setHead(BufImpl h): set head of chain
  • void setTail(BufImpl t): set tail of chain

CBufImpl class methods:

  • BufChainImpl createImpl()

CBufImpl virtual methods:

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

3. In getBuf(size capa, bool clip) you adjust capa based upon maxcap and clip, but the resulting variable value is not used when calling the free list allocate. It seems you are just always allocating maxcap and not using the passed variable size. Maybe this is just a sanity check for the exception? It seems that maybe the capa arg is not necessary. (see next item).

4. On the note of buffer sizes. I see the allocation size is hard coded in cpsw_buf.c and there is a uniform size for all buffers. Would it make more sense for the free list to be exposed to a higher level and using it as a argument to getBuf? This way a number of free lists can be created with a size most appropriate for where they are being used. For applications with a mix of data and command traffic, you may want large buffers for data and smaller for commands. Of course this would be for applications where the transport medium allows for larger buffers. We would still use buffer chains for really large frames (continue flag in the PGP and axi-stream drivers).

5. Why do you not pre-create the free list for the buffer chain like you do for the buffer entries. You seem to have it set externally to a public class variable.

6. unlinkCheap() seems to be a method to kill the next pointer in each buffer, which is used to clean up a buffer chain. But why is the key passed as an arg? Is it mean to used in classes that override the virtual method?

7. Why is IBufChain::take_ownership a class method instead of an instance method? Is this because it has to perform a cast to BufChainImpl?

8. In CBufChainImpl::insert. I think it is confusing to recreate a variable within a while loop with the same name as an existing variable. 

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.



 

 

 

 

 

 

  • No labels