diff mbox series

[ovs-dev,v8,07/13] dp-packet: Handle multi-seg mbufs in put_uninit().

Message ID 1528734090-220990-9-git-send-email-tiago.lam@intel.com
State Superseded
Headers show
Series None | expand

Commit Message

Lam, Tiago June 11, 2018, 4:21 p.m. UTC 
The dp_packet_put_uninit() function is, in its 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, when making use of multi-segment mbufs, it
is the data length of the last mbuf in the mbuf chain that should be
adjusted. This function has thus been modified to support multi-segment
mbufs.

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 | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Eelco Chaudron June 18, 2018, 11:52 a.m. UTC | #1 
On 11 Jun 2018, at 18:21, Tiago Lam wrote:

> The dp_packet_put_uninit() function is, in its 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, when making use of multi-segment mbufs, it
> is the data length of the last mbuf in the mbuf chain that should be
> adjusted. This function has thus been modified to support 
> multi-segment
> mbufs.
>
> 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 | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/lib/dp-packet.c b/lib/dp-packet.c
> index 2aaeaae..9f8503e 100644
> --- a/lib/dp-packet.c
> +++ b/lib/dp-packet.c
> @@ -321,7 +321,19 @@ dp_packet_put_uninit(struct dp_packet *b, size_t 
> size)
>      void *p;
>      dp_packet_prealloc_tailroom(b, size);
>      p = dp_packet_tail(b);
> +#ifdef DPDK_NETDEV
> +    struct rte_mbuf *mbuf;
> +
> +    if (b->source == DPBUF_DPDK) {
> +        mbuf = rte_pktmbuf_lastseg(&b->mbuf);
> +    } else {
> +        mbuf = CONST_CAST(struct rte_mbuf *, &b->mbuf);
> +    }
> +
> +    mbuf->data_len += size;

How can we be sure there is room in the (last) mbuf->data? The 
dp_packet_set_size() is not allocating more room (but it's truncating).
The mbuf->data_len should be handled in dp_packet_set_size(), and not 
here so it will work for all these cases.

> +#endif
>      dp_packet_set_size(b, dp_packet_size(b) + size);
> +
>      return p;
>  }
>
> -- 
> 2.7.4
Lam, Tiago June 22, 2018, 7:04 p.m. UTC | #2 
On 18/06/2018 12:52, Eelco Chaudron wrote:
> 
> 
> On 11 Jun 2018, at 18:21, Tiago Lam wrote:
> 
>> The dp_packet_put_uninit() function is, in its 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, when making use of multi-segment mbufs, it
>> is the data length of the last mbuf in the mbuf chain that should be
>> adjusted. This function has thus been modified to support 
>> multi-segment
>> mbufs.
>>
>> 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 | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/lib/dp-packet.c b/lib/dp-packet.c
>> index 2aaeaae..9f8503e 100644
>> --- a/lib/dp-packet.c
>> +++ b/lib/dp-packet.c
>> @@ -321,7 +321,19 @@ dp_packet_put_uninit(struct dp_packet *b, size_t 
>> size)
>>      void *p;
>>      dp_packet_prealloc_tailroom(b, size);
>>      p = dp_packet_tail(b);
>> +#ifdef DPDK_NETDEV
>> +    struct rte_mbuf *mbuf;
>> +
>> +    if (b->source == DPBUF_DPDK) {
>> +        mbuf = rte_pktmbuf_lastseg(&b->mbuf);
>> +    } else {
>> +        mbuf = CONST_CAST(struct rte_mbuf *, &b->mbuf);
>> +    }
>> +
>> +    mbuf->data_len += size;
> 
> How can we be sure there is room in the (last) mbuf->data? The 
> dp_packet_set_size() is not allocating more room (but it's truncating).

dp_packet_prealloc_tailroom() called at the top is responsible for
arranging memory and make sure it returns with enough memory. Otherwise,
it keeps the same behavior of hard asserting.

> The mbuf->data_len should be handled in dp_packet_set_size(), and not 
> here so it will work for all these cases.
> 
This was a leftover, thanks for spotting it! I've fixed it and will
include with the next iteration.

Tiago.
Eelco Chaudron June 26, 2018, 10:35 a.m. UTC | #3 
On 22 Jun 2018, at 21:04, Lam, Tiago wrote:

> On 18/06/2018 12:52, Eelco Chaudron wrote:
>>
>>
>> On 11 Jun 2018, at 18:21, Tiago Lam wrote:
>>
>>> The dp_packet_put_uninit() function is, in its 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, when making use of multi-segment mbufs, 
>>> it
>>> is the data length of the last mbuf in the mbuf chain that should be
>>> adjusted. This function has thus been modified to support
>>> multi-segment
>>> mbufs.
>>>
>>> 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 | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>>
>>> diff --git a/lib/dp-packet.c b/lib/dp-packet.c
>>> index 2aaeaae..9f8503e 100644
>>> --- a/lib/dp-packet.c
>>> +++ b/lib/dp-packet.c
>>> @@ -321,7 +321,19 @@ dp_packet_put_uninit(struct dp_packet *b, 
>>> size_t
>>> size)
>>>      void *p;
>>>      dp_packet_prealloc_tailroom(b, size);
>>>      p = dp_packet_tail(b);
>>> +#ifdef DPDK_NETDEV
>>> +    struct rte_mbuf *mbuf;
>>> +
>>> +    if (b->source == DPBUF_DPDK) {
>>> +        mbuf = rte_pktmbuf_lastseg(&b->mbuf);
>>> +    } else {
>>> +        mbuf = CONST_CAST(struct rte_mbuf *, &b->mbuf);
>>> +    }
>>> +
>>> +    mbuf->data_len += size;
>>
>> How can we be sure there is room in the (last) mbuf->data? The
>> dp_packet_set_size() is not allocating more room (but it's 
>> truncating).
>
> dp_packet_prealloc_tailroom() called at the top is responsible for
> arranging memory and make sure it returns with enough memory. 
> Otherwise,
> it keeps the same behavior of hard asserting.

Yes I feel it’s a bad design in the first place to assert() if there 
is not enough memory to handle a packet. But nothing your patch can do 
about this, Ack.

>> The mbuf->data_len should be handled in dp_packet_set_size(), and not
>> here so it will work for all these cases.
>>
> This was a leftover, thanks for spotting it! I've fixed it and will
> include with the next iteration.
>
> Tiago.
diff mbox series

Patch

diff --git a/lib/dp-packet.c b/lib/dp-packet.c
index 2aaeaae..9f8503e 100644
--- a/lib/dp-packet.c
+++ b/lib/dp-packet.c
@@ -321,7 +321,19 @@  dp_packet_put_uninit(struct dp_packet *b, size_t size)
     void *p;
     dp_packet_prealloc_tailroom(b, size);
     p = dp_packet_tail(b);
+#ifdef DPDK_NETDEV
+    struct rte_mbuf *mbuf;
+
+    if (b->source == DPBUF_DPDK) {
+        mbuf = rte_pktmbuf_lastseg(&b->mbuf);
+    } else {
+        mbuf = CONST_CAST(struct rte_mbuf *, &b->mbuf);
+    }
+
+    mbuf->data_len += size;
+#endif
     dp_packet_set_size(b, dp_packet_size(b) + size);
+
     return p;
 }