[ovs-dev,RFC,v7,07/13] dp-packet: Handle multi-seg mubfs in shift() func.

Message ID 1527094048-105084-8-git-send-email-tiago.lam@intel.com
State Superseded
Headers show
Series
  • Support multi-segment mbufs
Related show

Commit Message

Tiago Lam May 23, 2018, 4:47 p.m.
In its current implementation dp_packet_shift() is also unaware of
multi-seg mbufs (that holds data in memory non-contiguously) and assumes
that data exists contiguously in memory, memmove'ing data to perform the
shift.

To add support for multi-seg mbuds a new set of functions was
introduced, dp_packet_mbuf_shift() and dp_packet_mbuf_write(). These
functions are used by dp_packet_shift(), when handling multi-seg mbufs,
to shift and write data within a chain of mbufs.

Signed-off-by: Tiago Lam <tiago.lam@intel.com>
---
 lib/dp-packet.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 101 insertions(+), 1 deletion(-)

Comments

Loftus, Ciara May 28, 2018, 3:42 p.m. | #1
> 
> In its current implementation dp_packet_shift() is also unaware of
> multi-seg mbufs (that holds data in memory non-contiguously) and assumes
> that data exists contiguously in memory, memmove'ing data to perform the
> shift.
> 
> To add support for multi-seg mbuds a new set of functions was
> introduced, dp_packet_mbuf_shift() and dp_packet_mbuf_write(). These
> functions are used by dp_packet_shift(), when handling multi-seg mbufs,
> to shift and write data within a chain of mbufs.
> 
> Signed-off-by: Tiago Lam <tiago.lam@intel.com>
> ---
>  lib/dp-packet.c | 102
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 101 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/dp-packet.c b/lib/dp-packet.c
> index 1b39946..7b603c7 100644
> --- a/lib/dp-packet.c
> +++ b/lib/dp-packet.c
> @@ -294,13 +294,113 @@ dp_packet_prealloc_headroom(struct dp_packet
> *b, size_t size)
>      }
>  }
> 
> +#ifdef DPDK_NETDEV
> +/* XXX: Caller must provide the correct mbuf where the offset is located. It
> +   must also guarantee that 'ofs' is within bounds */
> +static void
> +dp_packet_mbuf_write(struct rte_mbuf *mbuf, int16_t ofs, uint32_t len,
> +                     const void *data)
> +{
> +    char *dst_addr;
> +    uint16_t data_len;
> +    int len_copy;
> +    while (mbuf) {
> +        if (len == 0) {
> +            break;
> +        }
> +
> +        dst_addr = rte_pktmbuf_mtod_offset(mbuf, char *, ofs);
> +        data_len = MBUF_BUF_END(mbuf->buf_addr, mbuf->buf_len) -
> dst_addr;
> +
> +        len_copy = MIN(len, data_len);
> +        /* We don't know if 'data' is the result of a rte_pktmbuf_read call, in
> +         * which case we may end up writing to the same region of memory we
> are
> +         * reading from and overlapping. Thus, the use of memmove */
> +        memmove(dst_addr, data, len_copy);
> +
> +        data = ((char *) data) + len_copy;
> +        len -= len_copy;
> +        ofs = 0;
> +
> +        mbuf = mbuf->next;
> +    }
> +}
> +
> +static void
> +dp_packet_mbuf_shift_(struct rte_mbuf *dbuf, int16_t dst_ofs,
> +                      const struct rte_mbuf *sbuf, uint16_t src_ofs, int len)
> +{
> +    char rd[len];
> +
> +    const char *wd = rte_pktmbuf_read(sbuf, src_ofs, len, rd);
> +    if (!wd) {
> +        /* XXX: Log WARN here. */
> +        return;
> +    }
> +
> +    dp_packet_mbuf_write(dbuf, dst_ofs, len, wd);
> +}
> +
> +/* XXX: Delta must fall within the bounds of an mbuf */
> +static void
> +dp_packet_mbuf_shift(struct dp_packet *b, int delta)
> +{
> +    struct rte_mbuf *sbuf, *dbuf;
> +    uint16_t src_ofs;
> +    int16_t dst_ofs;
> +
> +    struct rte_mbuf *mbuf = CONST_CAST(struct rte_mbuf *, &b->mbuf);
> +    dbuf = mbuf;
> +    sbuf = mbuf;
> +
> +    /* Set the destination address to copy to */
> +    if (delta < 0) {
> +        dst_ofs = delta;
> +        src_ofs = 0;
> +    } else {
> +        dst_ofs = delta;
> +        src_ofs = 0;
> +    }

The outcome of the if and else is the same.

> +
> +    /* Shift data from src mbuf and offset to dst mbuf and offset */
> +    dp_packet_mbuf_shift_(dbuf, dst_ofs, sbuf, src_ofs,
> +                          rte_pktmbuf_pkt_len(sbuf));
> +
> +    /* Update mbufs' properties, depending if there's one mbuf or more */
> +    mbuf->data_off = mbuf->data_off + dst_ofs;
> +
> +    struct rte_mbuf *tmbuf = rte_pktmbuf_lastseg(mbuf);
> +    if (dbuf != tmbuf) {
> +        tmbuf->data_len += delta;
> +        mbuf->data_len -= dst_ofs;
> +    }
> +}
> +#endif
> +
>  /* Shifts all of the data within the allocated space in 'b' by 'delta' bytes.
>   * For example, a 'delta' of 1 would cause each byte of data to move one
> byte
>   * forward (from address 'p' to 'p+1'), and a 'delta' of -1 would cause each
> - * byte to move one byte backward (from 'p' to 'p-1'). */
> + * byte to move one byte backward (from 'p' to 'p-1').
> + * Note for DPBUF_DPDK(XXX): The shift can only move within the bounds
> of an
> + * mbuf since it assumes the source and destination offsets will be on the
> + * same mbuf. */
>  void
>  dp_packet_shift(struct dp_packet *b, int delta)
>  {
> +#ifdef DPDK_NETDEV
> +     if (b->source == DPBUF_DPDK) {
> +        ovs_assert(delta > 0 ? delta <= dp_packet_tailroom(b)
> +                   : delta < 0 ? -delta <= dp_packet_headroom(b)
> +                   : true);
> +
> +
> +        if (delta != 0) {

Could remove some code duplication here. The assert & check for delta !=0 is common for both DPDK_NETDEV and !DPDK_NETDEV

> +            dp_packet_mbuf_shift(b, delta);
> +        }
> +
> +        return;
> +    }
> +#endif
>      ovs_assert(delta > 0 ? delta <= dp_packet_tailroom(b)
>                 : delta < 0 ? -delta <= dp_packet_headroom(b)
>                 : true);
> --
> 2.7.4
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Patch

diff --git a/lib/dp-packet.c b/lib/dp-packet.c
index 1b39946..7b603c7 100644
--- a/lib/dp-packet.c
+++ b/lib/dp-packet.c
@@ -294,13 +294,113 @@  dp_packet_prealloc_headroom(struct dp_packet *b, size_t size)
     }
 }
 
+#ifdef DPDK_NETDEV
+/* XXX: Caller must provide the correct mbuf where the offset is located. It
+   must also guarantee that 'ofs' is within bounds */
+static void
+dp_packet_mbuf_write(struct rte_mbuf *mbuf, int16_t ofs, uint32_t len,
+                     const void *data)
+{
+    char *dst_addr;
+    uint16_t data_len;
+    int len_copy;
+    while (mbuf) {
+        if (len == 0) {
+            break;
+        }
+
+        dst_addr = rte_pktmbuf_mtod_offset(mbuf, char *, ofs);
+        data_len = MBUF_BUF_END(mbuf->buf_addr, mbuf->buf_len) - dst_addr;
+
+        len_copy = MIN(len, data_len);
+        /* We don't know if 'data' is the result of a rte_pktmbuf_read call, in
+         * which case we may end up writing to the same region of memory we are
+         * reading from and overlapping. Thus, the use of memmove */
+        memmove(dst_addr, data, len_copy);
+
+        data = ((char *) data) + len_copy;
+        len -= len_copy;
+        ofs = 0;
+
+        mbuf = mbuf->next;
+    }
+}
+
+static void
+dp_packet_mbuf_shift_(struct rte_mbuf *dbuf, int16_t dst_ofs,
+                      const struct rte_mbuf *sbuf, uint16_t src_ofs, int len)
+{
+    char rd[len];
+
+    const char *wd = rte_pktmbuf_read(sbuf, src_ofs, len, rd);
+    if (!wd) {
+        /* XXX: Log WARN here. */
+        return;
+    }
+
+    dp_packet_mbuf_write(dbuf, dst_ofs, len, wd);
+}
+
+/* XXX: Delta must fall within the bounds of an mbuf */
+static void
+dp_packet_mbuf_shift(struct dp_packet *b, int delta)
+{
+    struct rte_mbuf *sbuf, *dbuf;
+    uint16_t src_ofs;
+    int16_t dst_ofs;
+
+    struct rte_mbuf *mbuf = CONST_CAST(struct rte_mbuf *, &b->mbuf);
+    dbuf = mbuf;
+    sbuf = mbuf;
+
+    /* Set the destination address to copy to */
+    if (delta < 0) {
+        dst_ofs = delta;
+        src_ofs = 0;
+    } else {
+        dst_ofs = delta;
+        src_ofs = 0;
+    }
+
+    /* Shift data from src mbuf and offset to dst mbuf and offset */
+    dp_packet_mbuf_shift_(dbuf, dst_ofs, sbuf, src_ofs,
+                          rte_pktmbuf_pkt_len(sbuf));
+
+    /* Update mbufs' properties, depending if there's one mbuf or more */
+    mbuf->data_off = mbuf->data_off + dst_ofs;
+
+    struct rte_mbuf *tmbuf = rte_pktmbuf_lastseg(mbuf);
+    if (dbuf != tmbuf) {
+        tmbuf->data_len += delta;
+        mbuf->data_len -= dst_ofs;
+    }
+}
+#endif
+
 /* Shifts all of the data within the allocated space in 'b' by 'delta' bytes.
  * For example, a 'delta' of 1 would cause each byte of data to move one byte
  * forward (from address 'p' to 'p+1'), and a 'delta' of -1 would cause each
- * byte to move one byte backward (from 'p' to 'p-1'). */
+ * byte to move one byte backward (from 'p' to 'p-1').
+ * Note for DPBUF_DPDK(XXX): The shift can only move within the bounds of an
+ * mbuf since it assumes the source and destination offsets will be on the
+ * same mbuf. */
 void
 dp_packet_shift(struct dp_packet *b, int delta)
 {
+#ifdef DPDK_NETDEV
+     if (b->source == DPBUF_DPDK) {
+        ovs_assert(delta > 0 ? delta <= dp_packet_tailroom(b)
+                   : delta < 0 ? -delta <= dp_packet_headroom(b)
+                   : true);
+
+
+        if (delta != 0) {
+            dp_packet_mbuf_shift(b, delta);
+        }
+
+        return;
+    }
+#endif
     ovs_assert(delta > 0 ? delta <= dp_packet_tailroom(b)
                : delta < 0 ? -delta <= dp_packet_headroom(b)
                : true);