Hi, On Wed, Jul 10, 2019 at 05:14:33PM +0000, Wang, Haiyue wrote: > Hi, > > Sounds cool, just have some questions inline. > > > -----Original Message----- > > From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Olivier Matz > > Sent: Wednesday, July 10, 2019 17:29 > > To: dev@dpdk.org > > Subject: [dpdk-dev] [RFC] mbuf: support dynamic fields and flags > > > > Many features require to store data inside the mbuf. As the room in mbuf > > structure is limited, it is not possible to have a field for each > > feature. Also, changing fields in the mbuf structure can break the API > > or ABI. > > > > This commit addresses these issues, by enabling the dynamic registration > > of fields or flags: > > > > - a dynamic field is a named area in the rte_mbuf structure, with a > > given size (>= 1 byte) and alignment constraint. > > - a dynamic flag is a named bit in the rte_mbuf structure. > > > > The typical use case is a PMD that registers space for an offload > > feature, when the application requests to enable this feature. As > > the space in mbuf is limited, the space should only be reserved if it > > is going to be used (i.e when the application explicitly asks for it). > > > > The registration can be done at any moment, but it is not possible > > to unregister fields or flags for now. > > > > Signed-off-by: Olivier Matz <olivier.m...@6wind.com>
(...) > > +/** > > + * @file > > + * RTE Mbuf dynamic fields and flags > > + * > > + * Many features require to store data inside the mbuf. As the room in > > + * mbuf structure is limited, it is not possible to have a field for > > + * each feature. Also, changing fields in the mbuf structure can break > > + * the API or ABI. > > + * > > + * This module addresses this issue, by enabling the dynamic > > + * registration of fields or flags: > > + * > > + * - a dynamic field is a named area in the rte_mbuf structure, with a > > + * given size (>= 1 byte) and alignment constraint. > > + * - a dynamic flag is a named bit in the rte_mbuf structure. > > + * > > + * The typical use case is a PMD that registers space for an offload > > + * feature, when the application requests to enable this feature. As > > + * the space in mbuf is limited, the space should only be reserved if it > > + * is going to be used (i.e when the application explicitly asks for it). > > + * > > + * The registration can be done at any moment, but it is not possible > > + * to unregister fields or flags for now. > > + * > > + * Example of use: > > + * > > + * - RTE_MBUF_DYN_<feature>_(ID|SIZE|ALIGN) are defined in this file > > Does it means that all PMDs define their own > 'RTE_MBUF_DYN_<feature>_(ID|SIZE|ALIGN)' > here ? In other words, each PMD can expose its private DYN_<feature> here for > public > using ? For generic fields, I think they should be declared in this file. For instance, if we decide to replace the current m->timestamp field by a dynamic field, we should add like this: #define RTE_MBUF_DYN_TIMESTAMP_ID "rte_timestamp" #define RTE_MBUF_DYN_TIMESTAMP_SIZE sizeof(uint64_t) #define RTE_MBUF_DYN_TIMESTAMP_ALIGN __alignof__(uint64_t) If the feature is PMD-specific, the defines could be exposed in a PMD header. > How about adding another eth_dev_ops API definitions to show the PMD's > supporting feature > names, sizes, align in run time for testpmd ? And also another eth_dev_ops > API for showing > the data saved in rte_mbuf by 'dump_pkt_burst' ? Adding a new command for > testpmd to set > the dynamic feature may be good for PMD test. > > > + * - If the application asks for the feature, the PMD use > > How does the application ask for the feature ? By ' > rte_mbuf_dynfield_register()' ? No change in this area. If we take again the timestamp example, the feature is asked by the application through the ethdev layer by passing DEV_RX_OFFLOAD_TIMESTAMP to port or queue configuration. > > > + * rte_mbuf_dynfield_register() to get the dynamic offset and stores > > + * in a global variable. > > In case, the PMD calls 'rte_mbuf_dynfield_register()' for 'dyn_feature' > firstly, this > means that PMD requests the dynamic feature itself if I understand correctly. > Should > PMD calls 'rte_mbuf_dynfield_lookup' for 'dyn_feature' to query the name > exists, the > size and align are right as expected ? If exists, but size and align are not > right, may > be for PMD change its definition, then PMD can give a warning or error > message. If name > exists, both size and align are expected, then PMD think that the application > request > the right dynamic features. The PMD should only call rte_mbuf_dynfield_register() if the application requests the feature (through ethdev, or through another mean if it's a PMD-specific feature). The goal is to only reserve the area in the mbuf for features that are actually needed. Hope this is clearer now. I think I need to enhance the documentation in next version ;) Thanks for the feedback.
