add external pktmbuf metadata API #97
Conversation
|
Let's hold off on this merge until after the CNET TCP #91 , as it may change that code a bit. |
KeithWiles
force-pushed
the
pktmbuf_external_metadata
branch
2 times, most recently
from
0df682c
to
976ccc5
Compare
The metadata was contained in the headroom of the pktmbuf and it is limited in size to max headroom size normally 128 bytes. The headroom size is also used for adding protcol headers and if the room is consumed by the metadata then headers can not be added. Created a new API pktmbuf_pool_cfg_create() to be able to create a external metadata buffer array indexed by the pktmbuf_t.metadata_idx value. The new metadata array of buffers must be a multiple of a cacheline (64 bytes) and the number of buffers will match the number of pktmbuf_t buffers in the mempool. If the new API pktmbuf_pool_cfg_create() is not used the metadata buffer will be part of the headroom in the pktmbuf buffer. Signed-off-by: Keith Wiles <keith.wiles@intel.com>
KeithWiles
force-pushed
the
pktmbuf_external_metadata
branch
from
976ccc5
to
75b9dca
Compare
| * @return | ||
| * -1 if pktmbuf pointer is NULL or size of metadata buffer or headroom size if internal. | ||
| */ | ||
| static inline uint32_t |
There was a problem hiding this comment.
Should you returned a int32_t if you plan on returning -1? YOu could create a define PKTMBUF_INV_MD_SZ (u32)-1 and return that instead if you want ot keep it a uint
| pktmbuf_pool_cfg(pktmbuf_pool_cfg_t *c, void *addr, uint32_t bufcnt, uint32_t bufsz, | ||
| uint32_t cache_sz, void *metadata, uint32_t metadata_bufsz, mbuf_ops_t *ops) | ||
| { | ||
| if (c) { |
There was a problem hiding this comment.
It's a little challenging to immediately follow the nested if statements with all the returns. Would you consider checking all the values for null/zero at the top? I think you can avoid some of hte nested if statements. I think the following more clearly conveys the constraints.
if (!c || !addr || !bufcnt || !bufsz)
return -1;
if ((!metadata && metadata_bufsz) || (metadata && !metadata_bufsz))
return -1;
if (metadata && ((metadata_bufsz % CNE_CACHE_LINE_SIZE) != 0))
return -1;
c->addr = addr;
c->bufcnt = bufcnt;
c->bufsz = bufsz;
c->cache_sz = cache_sz;
c->ops = ops;
c->metadata = metadata;
c->metadata_bufsz = metadata_bufsz;
return 0;
Also, this doesn't really need to be a static inline function. Would you consider putting this in pktmbuf.c?
There was a problem hiding this comment.
Your suggestion is much easy to read and understand. I just tried to make it too hard :-(
|
|
||
| CNE_MAX_SET(cache_sz, MEMPOOL_CACHE_MAX_SIZE); | ||
| if (c->bufcnt == 0 || c->bufsz == 0) |
There was a problem hiding this comment.
This contradicts the code that occurs a few lines down which allows these to be set to 0 to indicate a default could be used. I'd prefer not to change those semantics.
CNE_DEFAULT_SET(c->bufcnt, 0, DEFAULT_MBUF_COUNT);
There was a problem hiding this comment.
restored to previous.
| * @param addr | ||
| * Address of the pktmbuf buffers, must not be NULL | ||
| * @param bufcnt | ||
| * Number of buffers in the pktmbuf pool, must not be zero |
There was a problem hiding this comment.
The pktmbuf_pool_create() function (prior to this change) let users pass a value of 0 for bufcnt and/or bufsz to indicate a default value should be chosen. Do you think we should keep that? I like it, since its less to configure...
There was a problem hiding this comment.
Changed the text to allow them to zero and I do not see if the code needs to change now after the above change.
|
I could not get the conflicts to be resolved in a clean way, so I will close this PR and open a new one based on the current main branch. |
The metadata was contained in the headroom of the pktmbuf and it is
limited in size to max headroom size normally 128 bytes.
The headroom size is also used for adding protocol headers and if the
room is consumed by the metadata then headers can not be added.
Created a new API pktmbuf_pool_cfg_create() to be able to create a
external metadata buffer array indexed by the pktmbuf_t.metadata_idx
value. The new metadata array of buffers must be a multiple of a
cacheline (64 bytes) and the number of buffers will match the number of
pktmbuf_t buffers in the mempool.
If the new API pktmbuf_pool_cfg_create() is not used the metadata buffer
will be part of the headroom in the pktmbuf buffer.
Signed-off-by: Keith Wiles keith.wiles@intel.com