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
