diff mbox series

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

Message ID 1538151315-117132-8-git-send-email-tiago.lam@intel.com
State Superseded
Headers show
Series Support multi-segment mbufs | expand

Commit Message

Lam, Tiago Sept. 28, 2018, 4:15 p.m. UTC
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>
Acked-by: Eelco Chaudron <echaudro@redhat.com>
---
 lib/dp-packet.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/dp-packet.h |  10 ++++++
 2 files changed, 110 insertions(+)

Comments

0-day Robot Sept. 28, 2018, 5:08 p.m. UTC | #1
Bleep bloop.  Greetings Tiago Lam, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Comment with 'xxx' marker
#40 FILE: lib/dp-packet.c:306:
 * XXX: This function is the counterpart of the `rte_pktmbuf_read()` function

Lines checked: 172, Warnings: 1, Errors: 0


Please check this out.  If you feel there has been an error, please email aconole@bytheb.org

Thanks,
0-day Robot
Flavio Leitner Oct. 3, 2018, 6:26 p.m. UTC | #2
On Fri, Sep 28, 2018 at 05:15:08PM +0100, Tiago Lam wrote:
> 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

typo..  mbufs
> 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>
> Acked-by: Eelco Chaudron <echaudro@redhat.com>
> ---
>  lib/dp-packet.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/dp-packet.h |  10 ++++++
>  2 files changed, 110 insertions(+)
> 
> diff --git a/lib/dp-packet.c b/lib/dp-packet.c
> index 2aaeaae..167bf43 100644
> --- a/lib/dp-packet.c
> +++ b/lib/dp-packet.c
> @@ -294,6 +294,100 @@ dp_packet_prealloc_headroom(struct dp_packet *b, size_t size)
>      }
>  }
>  
> +#ifdef DPDK_NETDEV
> +/* Write len data bytes in a mbuf at specified offset.
> + *
> + * 'mbuf', pointer to the destination mbuf where 'ofs' is, and the mbuf where
> + * the data will first be written.
> + * 'ofs', the offset within the provided 'mbuf' where 'data' is to be written.
> + * 'len', the size of the to be written 'data'.
> + * 'data', pointer to the to be written bytes.
> + *
> + * XXX: This function is the counterpart of the `rte_pktmbuf_read()` function
> + * available with DPDK, in the rte_mbuf.h */
> +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. Hence the use of memmove() here */
> +        memmove(dst_addr, data, len_copy);
> +
> +        data = ((char *) data) + len_copy;
> +        len -= len_copy;
> +        ofs = 0;
> +
> +        mbuf->data_len = len_copy;
> +        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 = xmalloc(sizeof(*rd) * len);
> +    const char *wd = rte_pktmbuf_read(sbuf, src_ofs, len, rd);
> +
> +    ovs_assert(wd);
> +
> +    dp_packet_mbuf_write(dbuf, dst_ofs, len, wd);
> +
> +    free(rd);
> +}
> +
> +/* Similarly to dp_packet_shift(), shifts the data within the mbufs of a
> + * dp_packet of DPBUF_DPDK source by 'delta' bytes.
> + * Caller must make sure of the following conditions:
> + * - When shifting left, delta can't be bigger than the data_len available in
> + *   the last mbuf;
> + * - When shifting right, delta can't be bigger than the space available in the
> + *   first mbuf (buf_len - data_off).
> + * Both these conditions guarantee that a shift operation doesn't fall outside
> + * the bounds of the existing mbufs, so that the first and last mbufs (when
> + * using multi-segment mbufs), remain the same. */
> +static void
> +dp_packet_mbuf_shift(struct dp_packet *b, int delta)
> +{
> +    uint16_t src_ofs;
> +    int16_t dst_ofs;
> +
> +    struct rte_mbuf *mbuf = CONST_CAST(struct rte_mbuf *, &b->mbuf);
> +    struct rte_mbuf *tmbuf = rte_pktmbuf_lastseg(mbuf);
> +
> +    if (delta < 0) {
> +        ovs_assert(-delta <= tmbuf->data_len);
> +    } else {
> +        ovs_assert(delta < (mbuf->buf_len - mbuf->data_off));
> +    }
> +
> +    /* Set the destination and source offsets to copy to */
> +    dst_ofs = delta;
> +    src_ofs = 0;
> +
> +    /* Shift data from src mbuf and offset to dst mbuf and offset */
> +    dp_packet_mbuf_shift_(mbuf, dst_ofs, mbuf, src_ofs,
> +                          rte_pktmbuf_pkt_len(mbuf));
> +
> +    /* Update mbufs' properties, and if using multi-segment mbufs, first and
> +     * last mbuf's data_len also needs to be adjusted */
> +    mbuf->data_off = mbuf->data_off + 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
> @@ -306,6 +400,12 @@ dp_packet_shift(struct dp_packet *b, int delta)
>                 : true);
>  
>      if (delta != 0) {
> +#ifdef DPDK_NETDEV
> +        if (b->source == DPBUF_DPDK) {
> +            dp_packet_mbuf_shift(b, delta);
> +            return;
> +        }
> +#endif
>          char *dst = (char *) dp_packet_data(b) + delta;
>          memmove(dst, dp_packet_data(b), dp_packet_size(b));
>          dp_packet_set_data(b, dst);
> diff --git a/lib/dp-packet.h b/lib/dp-packet.h
> index 259b470..aab8f62 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
> +

Since the above is internal to dp-packet.c, I suggest to leave it
there to not expose it to outside.

I think malloc() might introduce a preemption point, but this doesn't
appear to be used in the fast path, so we should be okay.

fbl

>  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,11 @@ 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
> +void
> +dp_packet_mbuf_write(struct rte_mbuf *mbuf, int16_t ofs, uint32_t len,
> +                     const void *data);
> +#endif
>  static inline void *dp_packet_tail(const struct dp_packet *);
>  static inline void *dp_packet_end(const struct dp_packet *);
>  
> -- 
> 2.7.4
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff mbox series

Patch

diff --git a/lib/dp-packet.c b/lib/dp-packet.c
index 2aaeaae..167bf43 100644
--- a/lib/dp-packet.c
+++ b/lib/dp-packet.c
@@ -294,6 +294,100 @@  dp_packet_prealloc_headroom(struct dp_packet *b, size_t size)
     }
 }
 
+#ifdef DPDK_NETDEV
+/* Write len data bytes in a mbuf at specified offset.
+ *
+ * 'mbuf', pointer to the destination mbuf where 'ofs' is, and the mbuf where
+ * the data will first be written.
+ * 'ofs', the offset within the provided 'mbuf' where 'data' is to be written.
+ * 'len', the size of the to be written 'data'.
+ * 'data', pointer to the to be written bytes.
+ *
+ * XXX: This function is the counterpart of the `rte_pktmbuf_read()` function
+ * available with DPDK, in the rte_mbuf.h */
+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. Hence the use of memmove() here */
+        memmove(dst_addr, data, len_copy);
+
+        data = ((char *) data) + len_copy;
+        len -= len_copy;
+        ofs = 0;
+
+        mbuf->data_len = len_copy;
+        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 = xmalloc(sizeof(*rd) * len);
+    const char *wd = rte_pktmbuf_read(sbuf, src_ofs, len, rd);
+
+    ovs_assert(wd);
+
+    dp_packet_mbuf_write(dbuf, dst_ofs, len, wd);
+
+    free(rd);
+}
+
+/* Similarly to dp_packet_shift(), shifts the data within the mbufs of a
+ * dp_packet of DPBUF_DPDK source by 'delta' bytes.
+ * Caller must make sure of the following conditions:
+ * - When shifting left, delta can't be bigger than the data_len available in
+ *   the last mbuf;
+ * - When shifting right, delta can't be bigger than the space available in the
+ *   first mbuf (buf_len - data_off).
+ * Both these conditions guarantee that a shift operation doesn't fall outside
+ * the bounds of the existing mbufs, so that the first and last mbufs (when
+ * using multi-segment mbufs), remain the same. */
+static void
+dp_packet_mbuf_shift(struct dp_packet *b, int delta)
+{
+    uint16_t src_ofs;
+    int16_t dst_ofs;
+
+    struct rte_mbuf *mbuf = CONST_CAST(struct rte_mbuf *, &b->mbuf);
+    struct rte_mbuf *tmbuf = rte_pktmbuf_lastseg(mbuf);
+
+    if (delta < 0) {
+        ovs_assert(-delta <= tmbuf->data_len);
+    } else {
+        ovs_assert(delta < (mbuf->buf_len - mbuf->data_off));
+    }
+
+    /* Set the destination and source offsets to copy to */
+    dst_ofs = delta;
+    src_ofs = 0;
+
+    /* Shift data from src mbuf and offset to dst mbuf and offset */
+    dp_packet_mbuf_shift_(mbuf, dst_ofs, mbuf, src_ofs,
+                          rte_pktmbuf_pkt_len(mbuf));
+
+    /* Update mbufs' properties, and if using multi-segment mbufs, first and
+     * last mbuf's data_len also needs to be adjusted */
+    mbuf->data_off = mbuf->data_off + 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
@@ -306,6 +400,12 @@  dp_packet_shift(struct dp_packet *b, int delta)
                : true);
 
     if (delta != 0) {
+#ifdef DPDK_NETDEV
+        if (b->source == DPBUF_DPDK) {
+            dp_packet_mbuf_shift(b, delta);
+            return;
+        }
+#endif
         char *dst = (char *) dp_packet_data(b) + delta;
         memmove(dst, dp_packet_data(b), dp_packet_size(b));
         dp_packet_set_data(b, dst);
diff --git a/lib/dp-packet.h b/lib/dp-packet.h
index 259b470..aab8f62 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,11 @@  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
+void
+dp_packet_mbuf_write(struct rte_mbuf *mbuf, int16_t ofs, uint32_t len,
+                     const void *data);
+#endif
 static inline void *dp_packet_tail(const struct dp_packet *);
 static inline void *dp_packet_end(const struct dp_packet *);