Re: [dpdk-dev] [RFC] mbuf: support dynamic fields and flags

["Wiles, Keith" <keith.wiles@intel.com>]

> On Jul 11, 2019, at 2:53 AM, Olivier Matz <olivier.matz@6wind.com> wrote:
> 
> Hi Keith,
> 
> On Wed, Jul 10, 2019 at 06:12:16PM +0000, Wiles, Keith wrote:
>> 
>> 
>>> On Jul 10, 2019, at 12:49 PM, Stephen Hemminger <stephen@networkplumber.org> wrote:
>>> 
>>> On Wed, 10 Jul 2019 11:29:07 +0200
>>> Olivier Matz <olivier.matz@6wind.com> wrote:
>>> 
>>>> /**
>>>> * Indicate that the metadata field in the mbuf is in use.
>>>> @@ -738,6 +741,8 @@ struct rte_mbuf {
>>>> 	 */
>>>> 	struct rte_mbuf_ext_shared_info *shinfo;
>>>> 
>>>> +	uint64_t dynfield1; /**< Reserved for dynamic fields. */
>>>> +	uint64_t dynfield2; /**< Reserved for dynamic fields. */
>>>> } __rte_cache_aligned;
>>> 
>>> Growing mbuf is a fundamental ABI break and this needs
>>> higher level approval.  Why not one pointer?
>>> 
>>> It looks like you are creating something like FreeBSD m_tag.
>>> Why not use that?
>> 
>> Changing the mbuf structure causes a big problem for a number reasons as Stephen states.
> 
> Can you elaborate?
> 
> This is indeed an ABI break, but I think this is only due to the adding
> of rte_mbuf_dynfield_copy() in rte_pktmbuf_attach(). The size of the
> mbuf does not change and the fields are not initialized when creating a
> new mbuf. So I think there is no ABI change for code that is not using
> rte_pktmbuf_attach().
> 
> I don't think it's a problem to have one ABI change, if it avoids many
> others in the future.
> 
>> If we leave the mbuf stucture alone and add this feature to the
>> headroom space between the mbuf structure and the packet. When setting
>> up the mempool/mbuf pool we define a headroom to hold the extra data
>> when the mbuf pool is created or just use the current headroom
>> space. Using this method we can eliminate the mbuf structure change
>> and add the data to the packet buffer. We can do away with dynfield1
>> and 2 as we know where headroom space begins and ends. Just a thought.
> 
> The size of the mbuf metadata (between the mbuf structure and the
> buffer) is configured per pool, so it can be different accross
> mbufs. So, the access to the dynamic field would be slower:
> *(mbuf + dynfield_offset + metadata_size(mbuf))

We can force that space to be a minimum size when the mempool is created in the case of a cloned mbuf. The cloned mbuf is a small use case, but am important one and increasing the size for those special mbufs by a cache line should not be a huge problem.

I think most allocations do not change the size from the default value of the headroom (128). The mbuf + buffer are normally rounded to 2K or a bit bigger, which gives a bit more space in those cases of a packet size of 1518-1522. Jumbo frames are the same. Using the headroom size for an application needs to be defined and setup for the max size anyway for the application needs, so normally all mbuf creates should contain the same size to account for mbuf moments within the system.

That is my $0.02.

> 
> Also, the size of the data buffer can be 0: it happens for mbuf pools
> that are dedicated to mbuf clones (that reference data in another mbuf
> or in an external buffer). In this case, there is no room after metadata
> to store the dynamic fields.
> 
> Thanks,
> Olivier

Regards,
Keith