[ovs-dev] [PATCH v5 08/14] dp-packet: Handle multi-seg mbufs in resize__().
Darrell Ball
dlu998 at gmail.com
Mon Jul 23 22:55:36 UTC 2018
On Fri, Jul 20, 2018 at 10:09 AM, Lam, Tiago <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>> 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>>> 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>>>
> > > Acked-by: Eelco Chaudron <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.
> > 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
> >
> > [2]
> > 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>>
> > > 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