Re: [dpdk-dev] [PATCH v2 1/6] mbuf: add buffer offset field for flexible indirection

[Olivier Matz <olivier.matz@6wind.com>]

Hi Yongseok,

On Tue, Apr 03, 2018 at 05:12:06PM -0700, Yongseok Koh wrote:
> On Tue, Apr 03, 2018 at 10:26:15AM +0200, Olivier Matz wrote:
> > Hi,
> > 
> > On Mon, Apr 02, 2018 at 11:50:03AM -0700, Yongseok Koh wrote:
> > > When attaching a mbuf, indirect mbuf has to point to start of buffer of
> > > direct mbuf. By adding buf_off field to rte_mbuf, this becomes more
> > > flexible. Indirect mbuf can point to any part of direct mbuf by calling
> > > rte_pktmbuf_attach_at().
> > > 
> > > Possible use-cases could be:
> > > - If a packet has multiple layers of encapsulation, multiple indirect
> > >   buffers can reference different layers of the encapsulated packet.
> > > - A large direct mbuf can even contain multiple packets in series and
> > >   each packet can be referenced by multiple mbuf indirections.
> > > 
> > > Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
> > 
> > I think the current API is already able to do what you want.
> > 
> > 1/ Here is a mbuf m with its data
> > 
> >                off
> >                <-->
> >                       len
> >           +----+   <---------->
> >           |    |
> >         +-|----v----------------------+
> >         | |    -----------------------|
> > m       | buf  |    XXXXXXXXXXX      ||
> >         |      -----------------------|
> >         +-----------------------------+
> > 
> > 
> > 2/ clone m:
> > 
> >   c = rte_pktmbuf_alloc(pool);
> >   rte_pktmbuf_attach(c, m);
> > 
> >   Note that c has its own offset and length fields.
> > 
> > 
> >                off
> >                <-->
> >                       len
> >           +----+   <---------->
> >           |    |
> >         +-|----v----------------------+
> >         | |    -----------------------|
> > m       | buf  |    XXXXXXXXXXX      ||
> >         |      -----------------------|
> >         +------^----------------------+
> >                |
> >           +----+
> > indirect  |
> >         +-|---------------------------+
> >         | |    -----------------------|
> > c       | buf  |                     ||
> >         |      -----------------------|
> >         +-----------------------------+
> > 
> >                 off    len
> >                 <--><---------->
> > 
> > 
> > 3/ remove some data from c without changing m
> > 
> >    rte_pktmbuf_adj(c, 10)   // at head
> >    rte_pktmbuf_trim(c, 10)  // at tail
> > 
> > 
> > Please let me know if it fits your needs.
> 
> No, it doesn't.
> 
> Trimming head and tail with the current APIs removes data and make the space
> available. Adjusting packet head means giving more headroom, not shifting the
> buffer itself. If m has two indirect mbufs (c1 and c2) and those are pointing to
> difference offsets in m,
> 
> rte_pktmbuf_adj(c1, 10);
> rte_pktmbuf_adj(c2, 20);
> 
> then the owner of c2 regard the first (off+20)B as available headroom. If it
> wants to attach outer header, it will overwrite the headroom even though the
> owner of c1 is still accessing it. Instead, another mbuf (h1) for the outer
> header should be linked by h1->next = c2.

Yes, after these operations c1, c2 and m should become read-only. So, to
prepend headers, another mbuf has to be inserted before as you suggest. It
is possible to wrap this in a function rte_pktmbuf_clone_area(m, offset,
length) that will:
  - alloc and attach indirect mbuf for each segment of m that is
    in the range [offset : length+offset].
  - prepend an empty and writable mbuf for the headers

> If c1 and c2 are attached with shifting buffer address by adjusting buf_off,
> which actually shrink the headroom, this case can be properly handled.

What do you mean by properly handled?

Yes, prepending data or adding data in the indirect mbuf won't override
the direct mbuf. But prepending data or adding data in the direct mbuf m
won't be protected.

>From an application point of view, indirect mbufs, or direct mbufs that
have refcnt != 1, should be both considered as read-only because they
may share their data. How an application can know if the data is shared
or not?

Maybe we need a flag to differentiate mbufs that are read-only
(something like SHARED_DATA, or simply READONLY). In your case, if my
understanding is correct, you want to have indirect mbufs with RW data.


> And another use-case (this is my actual use-case) is to make a large mbuf have
> multiple packets in series. AFAIK, this will also be helpful for some FPGA NICs
> because it transfers multiple packets to a single large buffer to reduce PCIe
> overhead for small packet traffic like the Multi-Packet Rx of mlx5 does.
> Otherwise, packets should be memcpy'd to regular mbufs one by one instead of
> indirect referencing.
> 
> Does this make sense?

I understand the need.

Another option would be to make the mbuf->buffer point to an external
buffer (not inside the direct mbuf). This would require to add a
mbuf->free_cb. See "Mbuf with external data buffer" (page 19) in [1] for
a quick overview.

[1] https://dpdksummit.com/Archive/pdf/2016Userspace/Day01-Session05-OlivierMatz-Userspace2016.pdf

The advantage is that it does not require the large data to be inside a
mbuf (requiring a mbuf structure before the buffer, and requiring to be
allocated from a mempool). On the other hand, it is maybe more complex
to implement compared to your solution.


Regards,
Olivier