[ovs-dev,RFC,v7,06/13] dp-packet: Handle multi-seg mbufs in put*() funcs.

The dp_packet_put*() function - dp_packet_put_uninit(), dp_packet_put()
and dp_packet_put_zeros() - are, in their current implementation,
operating on the data buffer of a dp_packet as if it were contiguous,
which in the case of multi-segment mbufs means they operate on the first
mbuf in the chain. However, in the case of dp_packet_put_uninit(), for
example, it is the data length of the last mbuf in the mbuf chain that
should be adjusted. These functions have thus been modified to support
multi-segment mbufs.

Additionally, most of the core logic in dp_pcket_put_uninit() was moved
to a new helper function, dp_packet_put_uninit()_, to abstract the
implementation details from the API, since in the case of multi-seg
mbufs a new struct is returned that holds the mbuf and offset that
constitute the tail. For the single mbuf case a pointer to the byte that
constitute the tail still returned.

Co-authored-by: Mark Kavanagh <mark.b.kavanagh@intel.com>

Signed-off-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
Signed-off-by: Tiago Lam <tiago.lam@intel.com>
---
 lib/dp-packet.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++++--------
 lib/dp-packet.h |  5 +++
 2 files changed, 89 insertions(+), 13 deletions(-)

> 
> The dp_packet_put*() function - dp_packet_put_uninit(), dp_packet_put()
> and dp_packet_put_zeros() - are, in their current implementation,
> operating on the data buffer of a dp_packet as if it were contiguous,
> which in the case of multi-segment mbufs means they operate on the first
> mbuf in the chain. However, in the case of dp_packet_put_uninit(), for
> example, it is the data length of the last mbuf in the mbuf chain that
> should be adjusted. These functions have thus been modified to support
> multi-segment mbufs.
> 
> Additionally, most of the core logic in dp_pcket_put_uninit() was moved
> to a new helper function, dp_packet_put_uninit()_, to abstract the
> implementation details from the API, since in the case of multi-seg
> mbufs a new struct is returned that holds the mbuf and offset that
> constitute the tail. For the single mbuf case a pointer to the byte that
> constitute the tail still returned.
> 
> Co-authored-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
> 
> Signed-off-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
> Signed-off-by: Tiago Lam <tiago.lam@intel.com>
> ---
>  lib/dp-packet.c | 97
> +++++++++++++++++++++++++++++++++++++++++++++++++--------
>  lib/dp-packet.h |  5 +++
>  2 files changed, 89 insertions(+), 13 deletions(-)
> 
> diff --git a/lib/dp-packet.c b/lib/dp-packet.c
> index 782e7c2..1b39946 100644
> --- a/lib/dp-packet.c
> +++ b/lib/dp-packet.c
> @@ -312,27 +312,66 @@ dp_packet_shift(struct dp_packet *b, int delta)
>      }
>  }
> 
> -/* Appends 'size' bytes of data to the tail end of 'b', reallocating and
> - * copying its data if necessary.  Returns a pointer to the first byte of the
> - * new data, which is left uninitialized. */

Is the function rte_pktmbuf_append() (http://dpdk.org/doc/api/rte__mbuf_8h.html#ad5b0cd686ad3bcbb83416ca8395a080b) of any use here?

> -void *
> -dp_packet_put_uninit(struct dp_packet *b, size_t size)
> +static void *
> +dp_packet_put_uninit_(struct dp_packet *b, size_t size)
>  {
>      void *p;
>      dp_packet_prealloc_tailroom(b, size);
> +#ifdef DPDK_NETDEV
> +    p = dp_packet_mbuf_tail(b);
> +    /* In the case of multi-segment mbufs, the data length of the last mbuf
> +     * should be adjusted by 'size' bytes. The packet length of the entire
> +     * mbuf chain (stored in the first mbuf of said chain) is adjusted in
> +     * the normal execution path below.
> +     */
> +
> +    struct rte_mbuf *tmbuf = (struct rte_mbuf *) p;
> +    struct mbuf_tail *mbuf_tail = xmalloc(sizeof(*mbuf_tail));
> +    size_t pkt_len = size;
> +
> +    mbuf_tail->mbuf = tmbuf;
> +    mbuf_tail->ofs = tmbuf->data_len;
> +
> +    /* Adjust size of intermediate mbufs from current tail to end */
> +    while (tmbuf && pkt_len > 0) {
> +        tmbuf->data_len = MIN(pkt_len, tmbuf->buf_len - tmbuf->data_off);
> +        pkt_len -= tmbuf->data_len;
> +
> +        tmbuf = tmbuf->next;
> +    }
> +
> +    p = (void *) mbuf_tail;
> +#else
>      p = dp_packet_tail(b);
> +#endif
>      dp_packet_set_size(b, dp_packet_size(b) + size);
> +
>      return p;
>  }
> 
> -/* Appends 'size' zeroed bytes to the tail end of 'b'.  Data in 'b' is
> - * reallocated and copied if necessary.  Returns a pointer to the first byte of
> - * the data's location in the dp_packet. */
> +static void *
> +dp_packet_tail_to_addr(struct dp_packet *b OVS_UNUSED, void *p) {
> +#ifdef DPDK_NETDEV
> +    struct mbuf_tail *mbuf_tail = (struct mbuf_tail *) p;
> +    p = (void *) rte_pktmbuf_mtod_offset(mbuf_tail->mbuf, void *,
> +                                         mbuf_tail->ofs);
> +#endif
> +
> +    return p;
> +}
> +
> +/* Appends 'size' bytes of data to the tail end of 'b', reallocating and
> + * copying its data if necessary.  Returns a pointer to the first byte of the
> + * new data, which is left uninitialized. */
>  void *
> -dp_packet_put_zeros(struct dp_packet *b, size_t size)
> +dp_packet_put_uninit(struct dp_packet *b, size_t size)
>  {
> -    void *dst = dp_packet_put_uninit(b, size);
> -    memset(dst, 0, size);
> +    void *tail = dp_packet_put_uninit_(b, size);
> +
> +    void *dst = dp_packet_tail_to_addr(b, tail);
> +
> +    free(tail);

For the !DPDK_NETDEV case, the code remains the same (as it should) except this call to free(). It could maybe cause a problem.

> +
>      return dst;
>  }
> 
> @@ -342,8 +381,40 @@ dp_packet_put_zeros(struct dp_packet *b, size_t
> size)
>  void *
>  dp_packet_put(struct dp_packet *b, const void *p, size_t size)
>  {
> -    void *dst = dp_packet_put_uninit(b, size);
> -    memcpy(dst, p, size);
> +    void *tail = dp_packet_put_uninit_(b, size);
> +#ifdef DPDK_NETDEV
> +    struct mbuf_tail *mbuf_tail = (struct mbuf_tail *) tail;
> +
> +    dp_packet_mbuf_write(mbuf_tail->mbuf, mbuf_tail->ofs, size, p);

This function doesn't exist yet.

> +#else
> +    memcpy(tail, p, size);
> +#endif
> +
> +    void *dst = dp_packet_tail_to_addr(b, tail);
> +
> +    free(tail);
> +
> +    return dst;
> +}
> +
> +/* Appends 'size' zeroed bytes to the tail end of 'b'.  Data in 'b' is
> + * reallocated and copied if necessary.  Returns a pointer to the first byte of
> + * the data's location in the dp_packet. */
> +void *
> +dp_packet_put_zeros(struct dp_packet *b, size_t size)
> +{
> +    void *dst;
> +#ifdef DPDK_NETDEV
> +    char zeros[size];
> +    memset(zeros, 0, size);
> +
> +    dst = dp_packet_put(b, zeros, size);
> +
> +    return dst;
> +#endif
> +    dst = dp_packet_put_uninit(b, size);
> +    memset(dst, 0, size);
> +
>      return dst;
>  }
> 
> diff --git a/lib/dp-packet.h b/lib/dp-packet.h
> index 4d4b420..2fa6205 100644
> --- a/lib/dp-packet.h
> +++ b/lib/dp-packet.h
> @@ -83,6 +83,11 @@ struct dp_packet {
>  #ifdef DPDK_NETDEV
>  #define MBUF_BUF_END(BUF_ADDR, BUF_LEN) \
>      (char *) (((char *) BUF_ADDR) + BUF_LEN)
> +
> +struct mbuf_tail {
> +    struct rte_mbuf *mbuf;
> +    uint16_t ofs;
> +};
>  #endif
> 
>  static inline void *dp_packet_data(const struct dp_packet *);
> --
> 2.7.4
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

On 28/05/2018 16:42, Loftus, Ciara wrote:
>>
>> The dp_packet_put*() function - dp_packet_put_uninit(), dp_packet_put()
>> and dp_packet_put_zeros() - are, in their current implementation,
>> operating on the data buffer of a dp_packet as if it were contiguous,
>> which in the case of multi-segment mbufs means they operate on the first
>> mbuf in the chain. However, in the case of dp_packet_put_uninit(), for
>> example, it is the data length of the last mbuf in the mbuf chain that
>> should be adjusted. These functions have thus been modified to support
>> multi-segment mbufs.
>>
>> Additionally, most of the core logic in dp_pcket_put_uninit() was moved
>> to a new helper function, dp_packet_put_uninit()_, to abstract the
>> implementation details from the API, since in the case of multi-seg
>> mbufs a new struct is returned that holds the mbuf and offset that
>> constitute the tail. For the single mbuf case a pointer to the byte that
>> constitute the tail still returned.
>>
>> Co-authored-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
>>
>> Signed-off-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
>> Signed-off-by: Tiago Lam <tiago.lam@intel.com>
>> ---
>>  lib/dp-packet.c | 97
>> +++++++++++++++++++++++++++++++++++++++++++++++++--------
>>  lib/dp-packet.h |  5 +++
>>  2 files changed, 89 insertions(+), 13 deletions(-)
>>
>> diff --git a/lib/dp-packet.c b/lib/dp-packet.c
>> index 782e7c2..1b39946 100644
>> --- a/lib/dp-packet.c
>> +++ b/lib/dp-packet.c
>> @@ -312,27 +312,66 @@ dp_packet_shift(struct dp_packet *b, int delta)
>>      }
>>  }
>>
>> -/* Appends 'size' bytes of data to the tail end of 'b', reallocating and
>> - * copying its data if necessary.  Returns a pointer to the first byte of the
>> - * new data, which is left uninitialized. */
> 
> Is the function rte_pktmbuf_append() (http://dpdk.org/doc/api/rte__mbuf_8h.html#ad5b0cd686ad3bcbb83416ca8395a080b) of any use here?
> 

Unfortunately most of the functions in rte_mbuf.h operate at the mbuf
level. In this case `rte_pktmbuf_append()` wouldn't allocate any more
data and would just return NULL if there's not enough tail room. To deal
with this why `dp_packet_resize__()` has been worked on to allocate more
mbufs when necessary.

>> -void *
>> -dp_packet_put_uninit(struct dp_packet *b, size_t size)
>> +static void *
>> +dp_packet_put_uninit_(struct dp_packet *b, size_t size)
>>  {
>>      void *p;
>>      dp_packet_prealloc_tailroom(b, size);
>> +#ifdef DPDK_NETDEV
>> +    p = dp_packet_mbuf_tail(b);
>> +    /* In the case of multi-segment mbufs, the data length of the last mbuf
>> +     * should be adjusted by 'size' bytes. The packet length of the entire
>> +     * mbuf chain (stored in the first mbuf of said chain) is adjusted in
>> +     * the normal execution path below.
>> +     */
>> +
>> +    struct rte_mbuf *tmbuf = (struct rte_mbuf *) p;
>> +    struct mbuf_tail *mbuf_tail = xmalloc(sizeof(*mbuf_tail));
>> +    size_t pkt_len = size;
>> +
>> +    mbuf_tail->mbuf = tmbuf;
>> +    mbuf_tail->ofs = tmbuf->data_len;
>> +
>> +    /* Adjust size of intermediate mbufs from current tail to end */
>> +    while (tmbuf && pkt_len > 0) {
>> +        tmbuf->data_len = MIN(pkt_len, tmbuf->buf_len - tmbuf->data_off);
>> +        pkt_len -= tmbuf->data_len;
>> +
>> +        tmbuf = tmbuf->next;
>> +    }
>> +
>> +    p = (void *) mbuf_tail;
>> +#else
>>      p = dp_packet_tail(b);
>> +#endif
>>      dp_packet_set_size(b, dp_packet_size(b) + size);
>> +
>>      return p;
>>  }
>>
>> -/* Appends 'size' zeroed bytes to the tail end of 'b'.  Data in 'b' is
>> - * reallocated and copied if necessary.  Returns a pointer to the first byte of
>> - * the data's location in the dp_packet. */
>> +static void *
>> +dp_packet_tail_to_addr(struct dp_packet *b OVS_UNUSED, void *p) {
>> +#ifdef DPDK_NETDEV
>> +    struct mbuf_tail *mbuf_tail = (struct mbuf_tail *) p;
>> +    p = (void *) rte_pktmbuf_mtod_offset(mbuf_tail->mbuf, void *,
>> +                                         mbuf_tail->ofs);
>> +#endif
>> +
>> +    return p;
>> +}
>> +
>> +/* Appends 'size' bytes of data to the tail end of 'b', reallocating and
>> + * copying its data if necessary.  Returns a pointer to the first byte of the
>> + * new data, which is left uninitialized. */
>>  void *
>> -dp_packet_put_zeros(struct dp_packet *b, size_t size)
>> +dp_packet_put_uninit(struct dp_packet *b, size_t size)
>>  {
>> -    void *dst = dp_packet_put_uninit(b, size);
>> -    memset(dst, 0, size);
>> +    void *tail = dp_packet_put_uninit_(b, size);
>> +
>> +    void *dst = dp_packet_tail_to_addr(b, tail);
>> +
>> +    free(tail);
> 
> For the !DPDK_NETDEV case, the code remains the same (as it should) except this call to free(). It could maybe cause a problem.
> 

This is a nice spot! It was indeed leading to memory corruption.

>> +
>>      return dst;
>>  }
>>
>> @@ -342,8 +381,40 @@ dp_packet_put_zeros(struct dp_packet *b, size_t
>> size)
>>  void *
>>  dp_packet_put(struct dp_packet *b, const void *p, size_t size)
>>  {
>> -    void *dst = dp_packet_put_uninit(b, size);
>> -    memcpy(dst, p, size);
>> +    void *tail = dp_packet_put_uninit_(b, size);
>> +#ifdef DPDK_NETDEV
>> +    struct mbuf_tail *mbuf_tail = (struct mbuf_tail *) tail;
>> +
>> +    dp_packet_mbuf_write(mbuf_tail->mbuf, mbuf_tail->ofs, size, p);
> 
> This function doesn't exist yet.
> 

It is included in the next patch in the series (07/13). But it was meant
to be here... I'll rebase to include it here for the next iteration.

Tiago.

>> +#else
>> +    memcpy(tail, p, size);
>> +#endif
>> +
>> +    void *dst = dp_packet_tail_to_addr(b, tail);
>> +
>> +    free(tail);
>> +
>> +    return dst;
>> +}
>> +
>> +/* Appends 'size' zeroed bytes to the tail end of 'b'.  Data in 'b' is
>> + * reallocated and copied if necessary.  Returns a pointer to the first byte of
>> + * the data's location in the dp_packet. */
>> +void *
>> +dp_packet_put_zeros(struct dp_packet *b, size_t size)
>> +{
>> +    void *dst;
>> +#ifdef DPDK_NETDEV
>> +    char zeros[size];
>> +    memset(zeros, 0, size);
>> +
>> +    dst = dp_packet_put(b, zeros, size);
>> +
>> +    return dst;
>> +#endif
>> +    dst = dp_packet_put_uninit(b, size);
>> +    memset(dst, 0, size);
>> +
>>      return dst;
>>  }
>>
>> diff --git a/lib/dp-packet.h b/lib/dp-packet.h
>> index 4d4b420..2fa6205 100644
>> --- a/lib/dp-packet.h
>> +++ b/lib/dp-packet.h
>> @@ -83,6 +83,11 @@ struct dp_packet {
>>  #ifdef DPDK_NETDEV
>>  #define MBUF_BUF_END(BUF_ADDR, BUF_LEN) \
>>      (char *) (((char *) BUF_ADDR) + BUF_LEN)
>> +
>> +struct mbuf_tail {
>> +    struct rte_mbuf *mbuf;
>> +    uint16_t ofs;
>> +};
>>  #endif
>>
>>  static inline void *dp_packet_data(const struct dp_packet *);
>> --
>> 2.7.4
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev