Re: [ovs-dev] [PATCH v8 08/13] dp-packet: Handle multi-seg mubfs in shift() func.
On 26/06/2018 13:28, Eelco Chaudron wrote:
>
>
> On 22 Jun 2018, at 21:04, Lam, Tiago wrote:
>
>> On 18/06/2018 12:55, Eelco Chaudron wrote:
>>>
>>>
>>> On 11 Jun 2018, at 18:21, 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
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
---
lib/dp-packet.c | 102
lib/dp-packet.h | 8 +
2 files changed, 110 insertions(+)
diff --git a/lib/dp-packet.c b/lib/dp-packet.c
index 9f8503e..399fadb 100644
--- a/lib/dp-packet.c
+++ b/lib/dp-packet.c
@@ -294,6 +294,102 @@ 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 = 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);
+
+ovs_assert(wd);
+
>>>
>>> This implementation copies the entire packet to a temp buffer and
>>> then
>>> copies it back. Would it be "faster" to do a direct memmove, starting
>>> with the last/first segment depending on the direction?
>>>
>>
>> If I understand the question correctly, that would assume that memory
>> is
>> contiguous in memory, which isn't guaranteed with multi-segment mbufs.
>> Hence why we read to a temp buffer and write all at once (when
>> writing,
>> in dp_packet_prealloc_tailroom(), we do use memmove() within the same
>> mbuf, where memory is contiguous).
>
> Assuming the sbuf is continues, no memory is copied to rd, however if
> it’s not it’s copied, and we move memory twice.
> I’m suggesting to optimise for this case, skip the extra copy, but not
> sure if this is worth the effort.
>
OK, I understand your point now. rte_pktmbuf_read() does that internally
- if the data to copy is within the same mbuf (`sbuf` in this case), the
pointer returned by the function is to the data section in the `sbuf`
itself. Only otherwise it copies to `rd` and returns a pointer to `rd`.
Tiago.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v8 08/13] dp-packet: Handle multi-seg mubfs in shift() func.
On 22 Jun 2018, at 21:04, Lam, Tiago wrote:
On 18/06/2018 12:55, Eelco Chaudron wrote:
On 11 Jun 2018, at 18:21, 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
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
---
lib/dp-packet.c | 102
lib/dp-packet.h | 8 +
2 files changed, 110 insertions(+)
diff --git a/lib/dp-packet.c b/lib/dp-packet.c
index 9f8503e..399fadb 100644
--- a/lib/dp-packet.c
+++ b/lib/dp-packet.c
@@ -294,6 +294,102 @@ 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 = 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);
+
+ovs_assert(wd);
+
This implementation copies the entire packet to a temp buffer and
then
copies it back. Would it be "faster" to do a direct memmove, starting
with the last/first segment depending on the direction?
If I understand the question correctly, that would assume that memory
is
contiguous in memory, which isn't guaranteed with multi-segment mbufs.
Hence why we read to a temp buffer and write all at once (when
writing,
in dp_packet_prealloc_tailroom(), we do use memmove() within the same
mbuf, where memory is contiguous).
Assuming the sbuf is continues, no memory is copied to rd, however if
it’s not it’s copied, and we move memory twice.
I’m suggesting to optimise for this case, skip the extra copy, but not
sure if this is worth the effort.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v8 08/13] dp-packet: Handle multi-seg mubfs in shift() func.
On 18/06/2018 12:55, Eelco Chaudron wrote:
>
>
> On 11 Jun 2018, at 18:21, 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
>> 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
>> ---
>> lib/dp-packet.c | 102
>>
>> lib/dp-packet.h | 8 +
>> 2 files changed, 110 insertions(+)
>>
>> diff --git a/lib/dp-packet.c b/lib/dp-packet.c
>> index 9f8503e..399fadb 100644
>> --- a/lib/dp-packet.c
>> +++ b/lib/dp-packet.c
>> @@ -294,6 +294,102 @@ 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 = 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);
>> +
>> +ovs_assert(wd);
>> +
>
> This implementation copies the entire packet to a temp buffer and then
> copies it back. Would it be "faster" to do a direct memmove, starting
> with the last/first segment depending on the direction?
>
If I understand the question correctly, that would assume that memory is
contiguous in memory, which isn't guaranteed with multi-segment mbufs.
Hence why we read to a temp buffer and write all at once (when writing,
in dp_packet_prealloc_tailroom(), we do use memmove() within the same
mbuf, where memory is contiguous).
Tiago.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v8 08/13] dp-packet: Handle multi-seg mubfs in shift() func.
On 11 Jun 2018, at 18:21, 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
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
---
lib/dp-packet.c | 102
lib/dp-packet.h | 8 +
2 files changed, 110 insertions(+)
diff --git a/lib/dp-packet.c b/lib/dp-packet.c
index 9f8503e..399fadb 100644
--- a/lib/dp-packet.c
+++ b/lib/dp-packet.c
@@ -294,6 +294,102 @@ 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 = 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);
+
+ovs_assert(wd);
+
This implementation copies the entire packet to a temp buffer and then
copies it back. Would it be "faster" to do a direct memmove, starting
with the last/first segment depending on the direction?
+dp_packet_mbuf_write(dbuf, dst_ofs, len, wd);
+}
+
+/* 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 *, >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;
+
+if (mbuf != tmbuf) {
+tmbuf->data_len += delta;
+mbuf->data_len -= delta;
+}
+}
+#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 +402,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;
[ovs-dev] [PATCH v8 08/13] dp-packet: Handle multi-seg mubfs in shift() func.
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
---
lib/dp-packet.c | 102
lib/dp-packet.h | 8 +
2 files changed, 110 insertions(+)
diff --git a/lib/dp-packet.c b/lib/dp-packet.c
index 9f8503e..399fadb 100644
--- a/lib/dp-packet.c
+++ b/lib/dp-packet.c
@@ -294,6 +294,102 @@ 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 = 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);
+
+ovs_assert(wd);
+
+dp_packet_mbuf_write(dbuf, dst_ofs, len, wd);
+}
+
+/* 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 *, >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;
+
+if (mbuf != tmbuf) {
+tmbuf->data_len += delta;
+mbuf->data_len -= delta;
+}
+}
+#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 +402,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 272597f..4946fa3 100644
--- a/lib/dp-packet.h
+++ b/lib/dp-packet.h
@@ -80,6 +80,11 @@ struct dp_packet {
};
};
+#ifdef DPDK_NETDEV
