[ovs-dev] [PATCH v8 09/13] dp-packet: Handle multi-seg mbufs in resize__().

Eelco Chaudron echaudro at redhat.com
Tue Jun 26 12:42:53 UTC 2018



On 22 Jun 2018, at 21:04, Lam, Tiago wrote:

> On 18/06/2018 14:06, Eelco Chaudron wrote:
>>
>>
>> On 11 Jun 2018, at 18:21, Tiago Lam wrote:
>>
>>> When enabled with DPDK OvS relies on mbufs allocated by mempools to
>>> receive and output data on DPDK ports. Until now, each OvS dp_packet
>>> has
>>> had only one mbuf associated, which is allocated with the maximum
>>> possible size, taking the MTU into account. This approach, however,
>>> doesn't allow us to increase the allocated size in an mbuf, if 
>>> needed,
>>> since an mbuf is allocated and initialised upon mempool creation.
>>> Thus,
>>> in the current implementatin this is dealt with by calling
>>> OVS_NOT_REACHED() and terminating OvS.
>>>
>>> To avoid this, and allow the (already) allocated space to be better
>>> used, dp_packet_resize__() now tries to use the available room, both
>>> the
>>> tailroom and the headroom, to make enough space for the new data.
>>> Since
>>> this happens for packets of source DPBUF_DPDK, the single-segment 
>>> mbuf
>>> case mentioned above is also covered by this new aproach in
>>> resize__().
>>>
>>> Signed-off-by: Tiago Lam <tiago.lam at intel.com>
>>> ---
>>>  lib/dp-packet.c | 48 
>>> ++++++++++++++++++++++++++++++++++++++++++++++--
>>>  1 file changed, 46 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/lib/dp-packet.c b/lib/dp-packet.c
>>> index 399fadb..d0fab94 100644
>>> --- a/lib/dp-packet.c
>>> +++ b/lib/dp-packet.c
>>> @@ -237,9 +237,51 @@ dp_packet_resize__(struct dp_packet *b, size_t
>>> new_headroom, size_t new_tailroom
>>>      new_allocated = new_headroom + dp_packet_size(b) + 
>>> new_tailroom;
>>>
>>>      switch (b->source) {
>>> +    /* When resizing mbufs, both a single mbuf and multi-segment
>>> mbufs (where
>>> +     * data is not contigously held in memory), both the headroom 
>>> and
>>> the
>>> +     * tailroom available will be used to make more space for where
>>> data needs
>>> +     * to be inserted. I.e if there's not enough headroom, data may
>>> be shifted
>>> +     * right if there's enough tailroom.
>>> +     * However, this is not bulletproof and in some cases the space
>>> available
>>> +     * won't be enough - in those cases, an error should be 
>>> returned
>>> and the
>>> +     * packet dropped. */
>>>      case DPBUF_DPDK:
>>> -        OVS_NOT_REACHED();
>>> +    {
>>> +        size_t miss_len;
>>> +
>>> +        if (new_headroom == dp_packet_headroom(b)) {
>>> +            /* This is a tailroom adjustment. Since there's no
>>> tailroom space
>>> +             * left, try and shift data towards the head to free up
>>> tail space,
>>> +             * if there's enough headroom */
>>> +
>>> +            miss_len = new_tailroom - dp_packet_tailroom(b);
>>> +
>>> +            if (miss_len <= new_headroom) {
>>> +                dp_packet_shift(b, -miss_len);
>>> +            } else {
>>> +                /* XXX: Handle error case and report error to 
>>> caller
>>> */
>>> +                OVS_NOT_REACHED();
>> Should we add another fragment here, asking as dp_packet_set_size() 
>> can
>> free buffers?
>
> I think I've covered this in my reply to the cover letter and to patch
> 06/13, we can continue the discussion there.
>
>>> +            }
>>> +        } else {
>>
>> Can it also be possible that we need to adjust both tail and head 
>> room?
>
> My understanding is that dp_packet_resize__() should be called
> internally only, by either dp_packet_prealloc_tailroom() or
> dp_packet_prealloc_headroom(), thus it should only change one of those
> at a time. The case that handles `DPBUF_MALLOC` makes the same 
> assumption.

I re-read the code and you and DPBUF_MALLOC are handling the case where 
both change.
So ignore my comment :)



More information about the dev mailing list