You are viewing an old version of this page. View the current version.

Compare with Current View Page History

« Previous Version 3 Next »

Files: 

  • cpsw_buf.h
  • cpsw_buf.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)
    • BufChain (IBufChain in shared pointer)
    • BufQueue (IBufQueue in shared pointer)
  • Private:
    • CBufImpl: Implementation of IBuf with ability to be a CFreeListNode
    • CBufChainImpl: Implementation of IBufChain with ability to be a CFreeListNode
    • BufImpl (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:

  • size_t getCapacity(): Get max capacity of buffer as created
  • uint8_t * getPayload(): Get pointer to start of payload within the buffer
  • size_t getSize(): Get size of usable payload within the buffer
  • size_t getAvail(): Get amount of room left (capacity - end of payload)
  • size_t getHeadRoom(): Get available data before start of payload (not sure why this would be needed?)
  • void setSize(size_t s): Set size of payload in the buffer
  • void setPayload(uint8_t *p): Set start of payload within the buffer to *p
  • bool adjPayload(ssize_t delta): Shift the payload pointer by a specified amount, return false if shift exceeds payload
  • Buf getNext(): Get next entry in chain
  • Buf getPrev(): Get previous entry in chain
  • BufChain getChain(): Get pointer to head of chain
  • void after(Buf buf): Insert this buffer after buf
  • void before(Buf buf): Insert this buffer before buf
  • void unlink(): Remove this buffer from the chain
  • void split(): Split the list at this entry. This buffer becomes the head of a new list, can not be on a chain

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

IBufChain class methods implemented in CBufChainImpl:

  • void take_ownership(BufChain p_owner): Set smart pointer inside the class to keep it from being destroyed
  • BufChain yield_ownership() Take ownership back and unlink internal smart pointer
  • BufChain create(): create a buffer chain.

IBufChain virtual methods implemented in CBufChainImpl:

  • Buf getHead(): get head of chain
  • Buf getTail(): get tail of chain
  • unsigned getLen(): get number of elements 
  • unsigned getSize(): get total number of payload bytes in chain
  • Buf createAtHead(size_t size, bool clip=false): create a new buffer at the heat
  • Buf createAtTail(size_t size, bool clip=false): create a new buffer at the tail
  • void addAtHead(Buf b): add a buffer to the head
  • void addAtTail(Buf b): add a buffer to the tail
  • uint64_t extract(void *buf, uint65_t off, unit64_t size): extract data to passed buffer
  • uint65_t insert(void *buf, uint64_t off, uint64_t size): insert data from passed buffer

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

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