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 <[email protected]>
>> ---
>> 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.
Tiago.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
