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

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

Hi Simon,

I think it's a good idea. Please see some minor comments below.

On 08/31/2015 02:41 PM, Simon Kagstrom wrote:
> Chaining/segmenting mbufs can be useful in many places, so make it
> global.
> 
> Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
> Signed-off-by: Johan Faltstrom <johan.faltstrom@netinsight.net>
> ---
> NOTE! Only compile-tested.
> 
> We were looking for packet segmenting functionality in the MBUF API but
> didn't find it. This patch moves the implementation, apart from the
> things which look ip_frag-specific.
> 
>  lib/librte_ip_frag/ip_frag_common.h      | 23 -----------------------
>  lib/librte_ip_frag/rte_ipv4_reassembly.c |  7 +++++--
>  lib/librte_ip_frag/rte_ipv6_reassembly.c |  7 +++++--
>  lib/librte_mbuf/rte_mbuf.h               | 23 +++++++++++++++++++++++
>  4 files changed, 33 insertions(+), 27 deletions(-)
> 
> diff --git a/lib/librte_ip_frag/ip_frag_common.h b/lib/librte_ip_frag/ip_frag_common.h
> index 6b2acee..cde6ed4 100644

> [...]

> 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;
}

> +	/* Chain 'tail' onto the old tail */
> +	cur_tail = rte_pktmbuf_lastseg(head);
> +	cur_tail->next = tail;
> +
> +	/* accumulate number of segments and total length. */
> +	head->nb_segs = (uint8_t)(head->nb_segs + tail->nb_segs);

I'm wondering if we shouldn't check the overflow here. In
this case we would need to have a return value in case of
failure.

> +	head->pkt_len += tail->pkt_len;
> +
> +	/* reset pkt_len and nb_segs for chained fragment. */
> +	tail->pkt_len = tail->data_len;
> +	tail->nb_segs = 1;

I don't think it's required to reset this fields in the tail mbuf.
In any case, they will be reset again.

> +}
> +
> +/**
>   * Dump an mbuf structure to the console.
>   *
>   * Dump all fields for the given packet mbuf and all its associated
> 


Regards,
Olivier