Re: [dpdk-dev] [PATCH RFC] mbuf/ip_frag: Move mbuf chaining to common code

[Olivier MATZ <olivier.matz@6wind.com>]

Hi,

>>> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
>>> index 8c2db1b..ef47256 100644
>>> --- a/lib/librte_mbuf/rte_mbuf.h
>>> +++ b/lib/librte_mbuf/rte_mbuf.h
>>> @@ -1801,6 +1801,29 @@ static inline int rte_pktmbuf_is_contiguous(const struct rte_mbuf *m)
>>>  }
>>>
>>>  /**
>>> + * Chain an mbuf to another, thereby creating a segmented packet.
>>> + *
>>> + * @param head the head of the mbuf chain (the first packet)
>>> + * @param tail the mbuf to put last in the chain
>>> + */
>>> +static inline void rte_pktmbuf_chain(struct rte_mbuf *head, struct rte_mbuf *tail)
>>> +{
>>> +	struct rte_mbuf *cur_tail;
>>> +
>>
>> Here, we could check if the pkt_len of tail mbuf is 0. If
>> it's the case, we can just free it and return. It would avoid
>> to have an empty segment inside the mbuf chain, which can be
>> annoying.
>>
>> if (unlikely(rte_pktmbuf_pkt_len(tail) == 0)) {
>> 	rte_pktmbuf_free(tail);
>> 	return;
>> }
> 
> Wonder why do we need to do that?
> Probably head mbuf is out of space and want to expand it using pktmbuf_chain()?
> So in that case seems logical:
> 1) allocate new mbuf (it's pkt_len will be 0)
> b) call pktmbuf_chain()

By experience, having empty segments in the middle of a mbuf
chain is problematic (functions getting ptr at offsets, some pmds
or hardware may behave badly), I wanted to avoid that risk.

Now, the use-case you described is legitimate. Another option would
be to have another function pktmbuf_append_new(m) that returns a new
mbuf that is already chained to the other.

But I'm also fine with removing the test, it's maybe simpler.

Regards,
Olivier