Re: [dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy

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

Hi Stephen,

Please find some comments below.

On 01/22/2016 02:40 PM, Mrzyglod, DanielX T wrote:
>> +/*
>> + * Creates a copy of the given packet mbuf.
>> + *
>> + * Walks through all segments of the given packet mbuf, and for each of them:
>> + *  - Creates a new packet mbuf from the given pool.
>> + *  - Copy segment to newly created mbuf.
>> + * Then updates pkt_len and nb_segs of the new packet mbuf to match values
>> + * from the original packet mbuf.
>> + *
>> + * @param md
>> + *   The packet mbuf to be copied.
>> + * @param mp
>> + *   The mempool from which the mbufs are allocated.
>> + * @return
>> + *   - The pointer to the new mbuf on success.
>> + *   - NULL if allocation fails.
>> + */
>> +static inline struct rte_mbuf *rte_pktmbuf_copy(struct rte_mbuf *md,
>> +		struct rte_mempool *mp)

You are usually against inline functions. Any reason why it's better
to inline the code in this case?

Also, I think the mbuf argument should be const.

>> +{
>> +	struct rte_mbuf *mc = NULL;
>> +	struct rte_mbuf **prev = &mc;
>> +
>> +	do {
>> +		struct rte_mbuf *mi;
>> +
>> +		mi = rte_pktmbuf_alloc(mp);
>> +		if (unlikely(mi == NULL)) {
>> +			rte_pktmbuf_free(mc);
>> +			return NULL;
>> +		}
>> +
>> +		mi->data_off = md->data_off;

I'm not a big fan of 'mi' and 'md' (and 'mc'). In rte_pktmbuf_attach(),
'md' means direct and 'mi' means indirect. Here, all mbufs are direct
(i.e. they points to their own data buffer).

I think that using 'm' and 'm2' (or m_dup) is clearer. Also, 'seg' looks
clearer for a mbuf that points to a rte_mbuf inside the chain.

>> +		mi->data_len = md->data_len;
>> +		mi->port = md->port;

We don't need to update port for all the segments here.
Same for vlan_tci, tx_offload, hash, pkt_len, nb_segs, ol_flags,
packet_type.

>> +		mi->vlan_tci = md->vlan_tci;
>> +		mi->tx_offload = md->tx_offload;
>> +		mi->hash = md->hash;
>> +
>> +		mi->next = NULL;
>> +		mi->pkt_len = md->pkt_len;
>> +		mi->nb_segs = md->nb_segs;
>> +		mi->ol_flags = md->ol_flags;
>> +		mi->packet_type = md->packet_type;
>> +
>> +		rte_memcpy(rte_pktmbuf_mtod(mi, char *),
>> +			   rte_pktmbuf_mtod(md, char *),
>> +			   md->data_len);
>> +
>> +		*prev = mi;
>> +		prev = &mi->next;

Using a double mbuf pointer here looks a bit overkill to me.
I suggest to do something like (just an example, not even compiled):

struct rte_mbuf *rte_pktmbuf_copy(const struct rte_mbuf *m,
	struct rte_mempool *mp)
{
	struct rte_mbuf *m2, *seg, *seg2;

	m2 = rte_pktmbuf_alloc(mp);
	if (unlikely(m2 == NULL)) {
		rte_pktmbuf_free(m2);
		return NULL;
	}
	m2->data_off = m->data_off;
	m2->data_len = m->data_len;
	m2->pkt_len = m->pkt_len;
	m2->tx_offload = m->tx_offload;
	/* ... */

	for (seg = m->next; seg != NULL; seg = seg->next) {

		seg2 = rte_pktmbuf_alloc(mp);
		if (unlikely(seg2 == NULL)) {
			rte_pktmbuf_free(m2);
			return NULL;
		}

		seg2->data_off = seg->data_off;
		/* ... */

		seg = seg->next;
	}
	return m2;
}


>> +	} while ((md = md->next) != NULL);
>> +
>> +	*prev = NULL;

why this?

>> +	__rte_mbuf_sanity_check(mc, 1);
>> +	return mc;
>> +}
>> +

I agree this kind of function could be useful. But if there is no
user of this code inside the dpdk, I think we must at least have
a test function for it in app/test/test_mbuf.c


Regards,
Olivier