[ovs-dev] [RFC v7 05/13] dp-packet: Handle multi-seg mbufs in helper funcs.

Lam, Tiago tiago.lam at intel.com
Wed May 30 07:21:00 UTC 2018


On 28/05/2018 16:41, Loftus, Ciara wrote:
>>
>> Most helper functions in dp-packet assume that the data held by a
>> dp_packet is contiguous, and perform operations such as pointer
>> arithmetic under that assumption. However, with the introduction of
>> multi-segment mbufs, where data is non-contiguous, such assumptions are
>> no longer possible. Some examples of Such helper functions are
>> dp_packet_tail(), dp_packet_tailroom(), dp_packet_end(),
>> dp_packet_get_allocated() and dp_packet_at().
>>
>> Thus, instead of assuming contiguous data in dp_packet, they  now
>> iterate over the (non-contiguous) data in mbufs to perform their
>> calculations.
>>
>> Co-authored-by: Mark Kavanagh <mark.b.kavanagh at intel.com>
>>
>> Signed-off-by: Mark Kavanagh <mark.b.kavanagh at intel.com>
>> Signed-off-by: Tiago Lam <tiago.lam at intel.com>
>> ---
>>  lib/dp-packet.h | 252
>> +++++++++++++++++++++++++++++++++++++++++++++-----------
>>  1 file changed, 205 insertions(+), 47 deletions(-)
>>
>> diff --git a/lib/dp-packet.h b/lib/dp-packet.h
>> index e79fb24..4d4b420 100644
>> --- a/lib/dp-packet.h
>> +++ b/lib/dp-packet.h
>> @@ -80,6 +80,11 @@ struct dp_packet {
>>      };
>>  };
>>
>> +#ifdef DPDK_NETDEV
>> +#define MBUF_BUF_END(BUF_ADDR, BUF_LEN) \
>> +    (char *) (((char *) BUF_ADDR) + BUF_LEN)
>> +#endif
>> +
>>  static inline void *dp_packet_data(const struct dp_packet *);
>>  static inline void dp_packet_set_data(struct dp_packet *, void *);
>>  static inline void *dp_packet_base(const struct dp_packet *);
>> @@ -133,6 +138,10 @@ static inline void *dp_packet_at(const struct
>> dp_packet *, size_t offset,
>>                                   size_t size);
>>  static inline void *dp_packet_at_assert(const struct dp_packet *,
>>                                          size_t offset, size_t size);
>> +#ifdef DPDK_NETDEV
>> +static inline void * dp_packet_at_offset(const struct dp_packet *b,
>> +                                         size_t offset);
>> +#endif
>>  static inline void *dp_packet_tail(const struct dp_packet *);
>>  static inline void *dp_packet_end(const struct dp_packet *);
>>
>> @@ -180,40 +189,6 @@ dp_packet_delete(struct dp_packet *b)
>>      }
>>  }
>>
>> -/* If 'b' contains at least 'offset + size' bytes of data, returns a pointer to
>> - * byte 'offset'.  Otherwise, returns a null pointer. */
>> -static inline void *
>> -dp_packet_at(const struct dp_packet *b, size_t offset, size_t size)
>> -{
>> -    return offset + size <= dp_packet_size(b)
>> -           ? (char *) dp_packet_data(b) + offset
>> -           : NULL;
>> -}
>> -
>> -/* Returns a pointer to byte 'offset' in 'b', which must contain at least
>> - * 'offset + size' bytes of data. */
>> -static inline void *
>> -dp_packet_at_assert(const struct dp_packet *b, size_t offset, size_t size)
>> -{
>> -    ovs_assert(offset + size <= dp_packet_size(b));
>> -    return ((char *) dp_packet_data(b)) + offset;
>> -}
>> -
>> -/* Returns a pointer to byte following the last byte of data in use in 'b'. */
>> -static inline void *
>> -dp_packet_tail(const struct dp_packet *b)
>> -{
>> -    return (char *) dp_packet_data(b) + dp_packet_size(b);
>> -}
>> -
>> -/* Returns a pointer to byte following the last byte allocated for use (but
>> - * not necessarily in use) in 'b'. */
>> -static inline void *
>> -dp_packet_end(const struct dp_packet *b)
>> -{
>> -    return (char *) dp_packet_base(b) + dp_packet_get_allocated(b);
>> -}
>> -
>>  /* Returns the number of bytes of headroom in 'b', that is, the number of
>> bytes
>>   * of unused space in dp_packet 'b' before the data that is in use.  (Most
>>   * commonly, the data in a dp_packet is at its beginning, and thus the
>> @@ -224,19 +199,63 @@ dp_packet_headroom(const struct dp_packet *b)
>>      return (char *) dp_packet_data(b) - (char *) dp_packet_base(b);
>>  }
>>
>> +#ifdef DPDK_NETDEV
>> +static inline struct rte_mbuf *
>> +dp_packet_mbuf_tail(const struct dp_packet *b)
>> +{
>> +    struct rte_mbuf *buf = CONST_CAST(struct rte_mbuf *, &b->mbuf);
>> +    if (b->source == DPBUF_DPDK) {
>> +
>> +        /* Find last segment where data ends, meaning the tail of the chained
>> +           mbufs is there */
>> +        struct rte_mbuf *pmbuf;
>> +        while (buf) {
>> +            if (MBUF_BUF_END(buf->buf_addr, buf->buf_len) !=
>> +                rte_pktmbuf_mtod_offset(buf, char *, buf->data_len)) {
>> +                break;
>> +            }
>> +
>> +            pmbuf = buf;
>> +            buf = buf->next;
>> +        }
>> +
>> +        return (buf == NULL) ? pmbuf : buf;
>> +    } else {
>> +        return buf;
>> +    }
>> +}
>> +#endif
> 
> dp_packet_tail() seems to perform the exact same function. Can these functions be consolidated into one?
> 

Thanks for pointing that out. I think we will still need both as
internally we want a way to find the tail mbuf and to keep the external
API we have to return the last used byte within the tail mbuf. But
there's definitely space for improvement, so I'll include a refactor of
this in the next iteration.

> Am I right to assume that the tail will not necessarily be the last segment? Otherwise would rte_pktmbuf_lastseg() (http://dpdk.org/doc/api/rte__mbuf_8h.html#a0c2d001cd113362dd123d663210afcbb) be useful here?
> 

Yeah, exactly! `rte_pktmbuf_lastseg()`, together with
`rte_pktmbuf_tailroom()`, work nicely if the end mbuf contains the last
byte of data in use. But we may have mbufs chained together where the
end mbufs don't contain the last byte of data in use (their `data_len`
is 0). Hence why `dp_packet_mbuf_tail()` tries to find the first mbuf
where the address of data_len != address of buf_len.

>> +
>>  /* Returns the number of bytes that may be appended to the tail end of
>>   * dp_packet 'b' before the dp_packet must be reallocated. */
>>  static inline size_t
>>  dp_packet_tailroom(const struct dp_packet *b)
>>  {
>> +#ifdef DPDK_NETDEV
>> +    struct rte_mbuf *mbuf = dp_packet_mbuf_tail(b);
>> +
>> +    size_t tailroom = 0;
>> +    while (mbuf) {
>> +        tailroom += rte_pktmbuf_tailroom(mbuf);
>> +
>> +        mbuf = mbuf->next;
>> +    }
>> +
>> +    return tailroom;
>> +#else
>>      return (char *) dp_packet_end(b) - (char *) dp_packet_tail(b);
>> +#endif
>>  }
>>
>>  /* Clears any data from 'b'. */
>>  static inline void
>>  dp_packet_clear(struct dp_packet *b)
>>  {
>> +#ifdef DPDK_NETDEV
>> +    rte_pktmbuf_reset(&b->mbuf);
>> +#else
>>      dp_packet_set_data(b, dp_packet_base(b));
>> +#endif
>>      dp_packet_set_size(b, 0);
>>  }
>>
>> @@ -355,10 +374,37 @@ dp_packet_set_l4(struct dp_packet *b, void *l4)
>>  static inline size_t
>>  dp_packet_l4_size(const struct dp_packet *b)
>>  {
>> +#ifdef DPDK_NETDEV
>> +    if (b->l4_ofs == UINT16_MAX) {
>> +        return 0;
>> +    }
>> +
>> +    struct rte_mbuf *hmbuf = CONST_CAST(struct rte_mbuf *, &b->mbuf);
>> +    struct rte_mbuf *tmbuf = dp_packet_tail(b);
>> +
>> +    size_t l4_size = 0;
>> +    if (hmbuf == tmbuf) {
>> +        l4_size = hmbuf->data_len - b->l4_ofs;
>> +    } else {
>> +        l4_size = hmbuf->data_len - b->l4_ofs;
>> +        hmbuf = hmbuf->next;
>> +
>> +        while (hmbuf) {
>> +            l4_size += tmbuf->data_len;
>> +
>> +            hmbuf = hmbuf->next;
>> +        }
>> +    }
>> +
>> +    l4_size -= dp_packet_l2_pad_size(b);
>> +
>> +    return l4_size;
>> +#else
>>      return b->l4_ofs != UINT16_MAX
>>          ? (const char *)dp_packet_tail(b) - (const char *)dp_packet_l4(b)
>>          - dp_packet_l2_pad_size(b)
>>          : 0;
>> +#endif
>>  }
>>
>>  static inline const void *
>> @@ -493,7 +539,7 @@ __packet_set_data(struct dp_packet *b, uint16_t v)
>>  static inline uint16_t
>>  dp_packet_get_allocated(const struct dp_packet *b)
>>  {
>> -    return b->mbuf.buf_len;
>> +    return b->mbuf.nb_segs * b->mbuf.buf_len;
>>  }
>>
>>  static inline void
>> @@ -501,7 +547,119 @@ dp_packet_set_allocated(struct dp_packet *b,
>> uint16_t s)
>>  {
>>      b->mbuf.buf_len = s;
>>  }
>> +
>> +static inline void *
>> +dp_packet_at_offset(const struct dp_packet *b, size_t offset)
>> +{
>> +    if (b->source == DPBUF_DPDK) {
>> +        struct rte_mbuf *buf = CONST_CAST(struct rte_mbuf *, &b->mbuf);
>> +
>> +        while (buf && offset > buf->data_len) {
>> +            offset -= buf->data_len;
>> +
>> +            buf = buf->next;
>> +        }
>> +        return buf ? rte_pktmbuf_mtod_offset(buf, char *, offset) : NULL;
>> +    } else {
>> +        return (char *) dp_packet_data(b) + offset;
>> +    }
>> +}
>> +
>> +/* If 'b' contains at least 'offset + size' bytes of data, returns a pointer to
>> + * byte 'offset'.  Otherwise, returns a null pointer. */
>> +static inline void *
>> +dp_packet_at(const struct dp_packet *b, size_t offset, size_t size)
>> +{
>> +    return offset + size <= dp_packet_size(b)
>> +        ? dp_packet_at_offset(b, offset)
>> +        : NULL;
>> +}
>> +
>> +/* Returns a pointer to byte 'offset' in 'b', which must contain at least
>> + * 'offset + size' bytes of data. */
>> +static inline void *
>> +dp_packet_at_assert(const struct dp_packet *b, size_t offset, size_t size)
>> +{
>> +    ovs_assert(offset + size <= dp_packet_size(b));
>> +    return dp_packet_at_offset(b, offset);
>> +}
>> +
>> +/* Returns a pointer to byte following the last byte of data in use in 'b'. */
>> +static inline void *
>> +dp_packet_tail(const struct dp_packet *b)
>> +{
>> +    struct rte_mbuf *buf = CONST_CAST(struct rte_mbuf *, &b->mbuf);
>> +    if (b->source == DPBUF_DPDK) {
>> +
>> +        /* Find last segment where data ends, meaning the tail of the chained
>> +           mbufs is there */
>> +        struct rte_mbuf *pmbuf;
>> +        while (buf) {
>> +            if (MBUF_BUF_END(buf->buf_addr, buf->buf_len) !=
>> +                rte_pktmbuf_mtod_offset(buf, char *, buf->data_len)) {
>> +                break;
>> +            }
>> +
>> +            pmbuf = buf;
>> +            buf = buf->next;
>> +        }
>> +
>> +        buf = (buf == NULL) ? pmbuf : buf;
>> +    }
>> +
>> +    return rte_pktmbuf_mtod_offset(buf, void *, buf->data_len);
>> +}
>> +
>> +/* Returns a pointer to byte following the last byte allocated for use (but
>> + * not necessarily in use) in 'b'. */
>> +static inline void *
>> +dp_packet_end(const struct dp_packet *b)
>> +{
>> +    if (b->source == DPBUF_DPDK) {
>> +        struct rte_mbuf *buf = CONST_CAST(struct rte_mbuf *, &(b->mbuf));
>> +
>> +        buf = rte_pktmbuf_lastseg(buf);
>> +
>> +        return (char *) buf->buf_addr + buf->buf_len;
>> +    } else {
>> +        return (char *) dp_packet_base(b) + dp_packet_get_allocated(b);
>> +    }
>> +}
>>  #else
>> +/* If 'b' contains at least 'offset + size' bytes of data, returns a pointer to
>> + * byte 'offset'.  Otherwise, returns a null pointer. */
>> +static inline void *
>> +dp_packet_at(const struct dp_packet *b, size_t offset, size_t size)
>> +{
>> +    return offset + size <= dp_packet_size(b)
>> +           ? (char *) dp_packet_data(b) + offset
>> +           : NULL;
>> +}
>> +
>> +/* Returns a pointer to byte 'offset' in 'b', which must contain at least
>> + * 'offset + size' bytes of data. */
>> +static inline void *
>> +dp_packet_at_assert(const struct dp_packet *b, size_t offset, size_t size)
>> +{
>> +    ovs_assert(offset + size <= dp_packet_size(b));
>> +    return ((char *) dp_packet_data(b)) + offset;
>> +}
>> +
>> +/* Returns a pointer to byte following the last byte of data in use in 'b'. */
>> +static inline void *
>> +dp_packet_tail(const struct dp_packet *b)
>> +{
>> +    return (char *) dp_packet_data(b) + dp_packet_size(b);
>> +}
>> +
>> +/* Returns a pointer to byte following the last byte allocated for use (but
>> + * not necessarily in use) in 'b'. */
>> +static inline void *
>> +dp_packet_end(const struct dp_packet *b)
>> +{
>> +    return (char *) dp_packet_base(b) + dp_packet_get_allocated(b);
>> +}
>> +
>>  static inline void *
>>  dp_packet_base(const struct dp_packet *b)
>>  {
>> @@ -514,6 +672,18 @@ dp_packet_set_base(struct dp_packet *b, void *d)
>>      b->base_ = d;
>>  }
>>
>> +static inline uint16_t
>> +dp_packet_get_allocated(const struct dp_packet *b)
>> +{
>> +    return b->allocated_;
>> +}
>> +
>> +static inline void
>> +dp_packet_set_allocated(struct dp_packet *b, uint16_t s)
>> +{
>> +    b->allocated_ = s;
>> +}
>> +
>>  static inline uint32_t
>>  dp_packet_size(const struct dp_packet *b)
>>  {
>> @@ -537,18 +707,6 @@ __packet_set_data(struct dp_packet *b, uint16_t v)
>>  {
>>      b->data_ofs = v;
>>  }
>> -
>> -static inline uint16_t
>> -dp_packet_get_allocated(const struct dp_packet *b)
>> -{
>> -    return b->allocated_;
>> -}
>> -
>> -static inline void
>> -dp_packet_set_allocated(struct dp_packet *b, uint16_t s)
>> -{
>> -    b->allocated_ = s;
>> -}
>>  #endif
> 
> Is it necessary to move the set_allocated and get_allocated functions above?
> 

Nope. I'll move them back.

Tiago.

>>
>>  static inline void
>> --
>> 2.7.4
>>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev


More information about the dev mailing list