[ovs-dev] [PATCH v5 08/14] dp-packet: Handle multi-seg mbufs in resize__().

Lam, Tiago tiago.lam at intel.com
Tue Jul 24 07:06:35 UTC 2018


On 23/07/2018 23:55, Darrell Ball wrote:
> 
> 
> On Fri, Jul 20, 2018 at 10:09 AM, Lam, Tiago <tiago.lam at intel.com
> <mailto:tiago.lam at intel.com>> wrote:
> 
>     On 18/07/2018 23:53, Darrell Ball wrote:
>     > sorry, several distractions delayed response.
>     > 
>     > On Mon, Jul 16, 2018 at 1:37 AM, Lam, Tiago <tiago.lam at intel.com <mailto:tiago.lam at intel.com>
>     > <mailto:tiago.lam at intel.com <mailto:tiago.lam at intel.com>>> wrote:
>     > 
>     >     On 13/07/2018 18:54, Darrell Ball wrote:
>     >     > Thanks for the patch.
>     >     > 
>     >     > A few queries inline.
>     >     > 
>     > 
>     >     Hi Darrell,
>     > 
>     >     Thanks for your inputs. I've replied in-line as well.
>     > 
>     >     > On Wed, Jul 11, 2018 at 11:23 AM, Tiago Lam <tiago.lam at intel.com <mailto:tiago.lam at intel.com>
>     <mailto:tiago.lam at intel.com <mailto:tiago.lam at intel.com>>
>     >     > <mailto:tiago.lam at intel.com <mailto:tiago.lam at intel.com>
>     <mailto:tiago.lam at intel.com <mailto:tiago.lam at intel.com>>>> 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 <mailto:tiago.lam at intel.com>
>     <mailto:tiago.lam at intel.com <mailto:tiago.lam at intel.com>>
>     >     >     <mailto:tiago.lam at intel.com <mailto:tiago.lam at intel.com>
>     <mailto:tiago.lam at intel.com <mailto:tiago.lam at intel.com>>>>
>     >     >     Acked-by: Eelco Chaudron <echaudro at redhat.com <mailto:echaudro at redhat.com>
>     >     <mailto:echaudro at redhat.com <mailto:echaudro at redhat.com>>
>     >     >     <mailto:echaudro at redhat.com <mailto:echaudro at redhat.com>
>     <mailto:echaudro at redhat.com <mailto:echaudro at redhat.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 d6e19eb..87af459 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();
>     >     >
>     >     >
>     >     > Previously, it was a coding error to call this function for
>     a DPDK
>     >     mbuf
>     >     > case, which is pretty
>     >     > clear. But with this patch, presumably that is not longer
>     the case and
>     >     > the calling the API is
>     >     > now ok for DPDK mbufs.
>     >     >
>     >
>     >     As it stands, it will still be an error to call
>     dp_packet_resize__() for
>     >     any DPDK packet, or by extension any of the other functions
>     that call
>     >     it, such as dp_packet_prealloc_tailroom() and
>     >     dp_packet_prealloc_headroom().
>     >
>     >
>     >
>     > yep, the existing code fails in a very clear way; i.e. whenever it is
>     > called for a dpdk packet.
>     > So the user would need to handle in some other way, which is not being
>     > done today, I know.
>     >
>     >  
>     >
>     >     This patch only tries to alleviate that
>     >     by accommodating space from the headroom or tailroom, if
>     possible, and
>     >     create just enough space for the new data.
>     >
>     >
>     > The new code will fail is some yet undefined way, occasionally working
>     > and failing
>     > in the "other" cases.
>     >
>     >  
>     >
>     >     My preferred approach would
>     >     be to return an error if not possible, but since the API
>     doesn't deal
>     >     with errors as is, the previous behavior of manually asserting
>     was left
>     >     as is. As reported in [1] (I comment more on that below), the
>     behavior 
>     >
>     >     of manually asserting can lead to undesired behavior in some
>     use cases.
>     >
>     >
>     >
>     > I am familiar with the issue.
>     > As part of the change,  dp_packet_put_uninit()
>     > and dp_packet_push_uninit() could be modified to return NULL
>     > and that could be percolated and checked for.
>     >
>     > Those APIs could simply check (by calling a helper API) if they would
>     > fail a priori to trigger returning 
>     > NULL for dpdk buf cases.
>     >
>     >  
> 
>     Ok, I'm glad we're on the same page here. That's what [2] is doing. I'm
>     planning to bring that forward. If you could have a look as well, that
>     would be great.
> 
>     >
>     >
>     >     >  
>     >     >
>     >     >     +    {
>     >     >     +        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();
>     >     >
>     >     >
>     >     >
>     >     > This will not just drop the packet, it will fail the daemon,
>     because a
>     >     > packet cannot be resized.
>     >     > If the system is completely depleted of memory, that may be
>     ok, but in
>     >     > the case, no such
>     >     > assumption is implied.
>     >     >
>     >     > Also, why is XXX still left in the patch?
>     >     >
>     >
>     >     Because there's still work to do in that regard. The whole process
>     >     shouldn't be brought down if there's not enough space to put
>     some data
>     >     in one single packet. However, this was intentionally left out
>     of this
>     >     series or otherwise it would increase its complexity
>     considerably. 
>     >
>     >
>     > It seems unnecessary to add a bunch of code to a series that tries
>     to handle
>     > 'resize', but handles it partially in practical cases. It also seems
>     > undefined when
>     > it works and when it does not from a API caller POV.
> 
>     Granted that the dp_packet_resize__() supports more operations than what
>     this implementation provides, but dp_packet API callers won't notice a
>     difference since the only callers are
>     dp_packet_prealloc_tailroom() and dp_packet_prealloc_headroom(). And the
>     implementation covers for those functions, which only either modify the
>     tailroom or the headroom (and not both).
> 
>     > I think patch 7 is also there to only support this patch 8.
>     > 
> 
>     That's a wrong assumption. Patch 7/14 was added to support the
>     dp_packet_shift() operation.
> 
>     In this patch 8/14, because of the noncontiguous nature of chained
>     mbufs, there's no possibility of using general functions like `memmove`,
>     and re-using the shift operations was just easier.
> 
> 
> I guess we may be talking about 2 different things.
> My point is simply that greping for dp_packet_mbuf_shift() does not show
> any users with resize__()
> removed; I think it would be best to remove unused code from the patchset
> dp_packet_mbuf_shift() is defined in patch 7.
> 
> 

Again, it is used in `dp_packet_shift()` for DPDK packets.

> 
>     > Ideally, It would seem like a modified patch 7 and patch 8 would
>     belong
>     > with the rest of the 
>     > fix for the dpdk packet memory preallocation constraint issue.
>     >
>     > Also, ideally, 'XXX' is removed from patches.
>     >
>     >  
>     >
>     >
>     >     As others have pointed out in [1], this is not a simple
>     change, which
>     >     would have to be propagated to higher levels in other parts of
>     the code
>     >     base. I've proposed an alternative (vs refactoring the whole
>     dp_packet
>     >     API to handle and return errors) in [2], but that seems to
>     have gone
>     >     stale. Going forward I see that approach merging with this new
>     piece in
>     >     dp_packet_resize__(), where an error can be returned to the
>     caller if
>     >     there's not enough space.
>     >
>     >
>     > The full change is outside the scope of this series.
>     >
>     >  
>     >
>     >
>     >     > Also, the preexisting API handles two cases:
>     >     > 1/ Tailroom only adjustment
>     >     > 2/ headroom and/or tailroom adjustment
>     >     >
>     >     > meaning it handles all cases.
>     >     >
>     >     > The new DPDK addition (part of the same API) defines 2 cases
>     >     >
>     >     > 1/ tailroom only adjustment
>     >     > 2/ headroom only adjustment
>     >     >
>     >     > So, it looks like a different API, that also does not handle
>     all cases.
>     >     >
>     >     >  
>     >
>     >     You have a point there, support for point 2/ "headroom and
>     tailroom
>     >     adjustment" is missed. It doesn't seem to be used anywhere at the
>     >     moment, the only callers being dp_packet_prealloc_tailroom() and
>     >     dp_packet_prealloc_headroom(), but I'll submit an incremental
>     patch to
>     >     deal with this. Thanks for pointing it out.
>     >
>     >     Tiago.
>     >
>     >     [1]
>     >   
>      https://mail.openvswitch.org/pipermail/ovs-dev/2018-May/346649.html
>     <https://mail.openvswitch.org/pipermail/ovs-dev/2018-May/346649.html>
>     >   
>      <https://mail.openvswitch.org/pipermail/ovs-dev/2018-May/346649.html <https://mail.openvswitch.org/pipermail/ovs-dev/2018-May/346649.html>>
>     >     [2]
>     >   
>      https://mail.openvswitch.org/pipermail/ovs-dev/2018-July/348908.html <https://mail.openvswitch.org/pipermail/ovs-dev/2018-July/348908.html>
>     >   
>      <https://mail.openvswitch.org/pipermail/ovs-dev/2018-July/348908.html
>     <https://mail.openvswitch.org/pipermail/ovs-dev/2018-July/348908.html>>
>     >
>     >     >
>     >     >     +            }
>     >     >     +        } else {
>     >     >     +            /* Otherwise, this is a headroom
>     adjustment. Try to
>     >     >     shift data
>     >     >     +             * towards the tail to free up head space,
>     if there's
>     >     >     enough
>     >     >     +             * tailroom */
>     >     >     +
>     >     >     +            miss_len = new_headroom -
>     dp_packet_headroom(b);
>     >     >
>     >     >     +
>     >     >     +            if (miss_len <= new_tailroom) {
>     >     >     +                dp_packet_shift(b, miss_len);
>     >     >     +            } else {
>     >     >     +                /* XXX: Handle error case and report
>     error to
>     >     caller */
>     >     >     +                OVS_NOT_REACHED();
>     >     >
>     >     >
>     >     >
>     >     > same comments as above.
>     >     >
>     >     >  
>     >     >
>     >     >     +            }
>     >     >     +        }
>     >     >     +
>     >     >     +        new_base = dp_packet_base(b);
>     >     >     +
>     >     >     +        break;
>     >     >     +    }
>     >     >          case DPBUF_MALLOC:
>     >     >              if (new_headroom == dp_packet_headroom(b)) {
>     >     >                  new_base = xrealloc(dp_packet_base(b),
>     >     new_allocated);
>     >     >     @@ -263,7 +305,9 @@ dp_packet_resize__(struct dp_packet
>     *b, size_t
>     >     >     new_headroom, size_t new_tailroom
>     >     >              OVS_NOT_REACHED();
>     >     >          }
>     >     >
>     >     >     -    dp_packet_set_allocated(b, new_allocated);
>     >     >     +    if (b->source != DPBUF_DPDK) {
>     >     >     +        dp_packet_set_allocated(b, new_allocated);
>     >     >     +    }
>     >     >          dp_packet_set_base(b, new_base);
>     >     >
>     >     >          new_data = (char *) new_base + new_headroom;
>     >     >     --
>     >     >     2.7.4
>     >     >
>     >     >     _______________________________________________
>     >     >     dev mailing list
>     >     >     dev at openvswitch.org <mailto:dev at openvswitch.org>
>     <mailto:dev at openvswitch.org <mailto:dev at openvswitch.org>>
>     >     <mailto:dev at openvswitch.org <mailto:dev at openvswitch.org>
>     <mailto:dev at openvswitch.org <mailto:dev at openvswitch.org>>>
>     >     >     https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>     <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>
>     >     <https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>     <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>>
>     >     >     <https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>     <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>
>     >     <https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>     <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>>>
>     >     >
>     >     >
>     >
>     >
> 
> 


More information about the dev mailing list