[dpdk-dev] [PATCH 09/18] mbuf: support Mpls in software packet type parser

[Liang, Cunming]

Hi Olivier,

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz at 6wind.com]
> Sent: Wednesday, July 06, 2016 4:00 PM
> To: Liang, Cunming <cunming.liang at intel.com>; dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 09/18] mbuf: support Mpls in software packet
> type parser
> 
> Hi Cunming,
> 
> On 07/06/2016 09:08 AM, Liang, Cunming wrote:
> > Hi Olivier,
> >
> > On 7/5/2016 11:41 PM, Olivier Matz wrote:
> >> Add a new RTE_PTYPE_L2_ETHER_MPLS packet type, and its support in
> >> rte_pktmbuf_get_ptype().
> >>
> >> Signed-off-by: Didier Pallard <didier.pallard at 6wind.com>
> >> Signed-off-by: Olivier Matz <olivier.matz at 6wind.com>
> >> ---
> >>   lib/librte_mbuf/rte_mbuf_ptype.c | 25 +++++++++++++++++++++++++
> >>   lib/librte_mbuf/rte_mbuf_ptype.h |  9 ++++++++-
> >>   lib/librte_net/Makefile          |  4 +++-
> >>   lib/librte_net/rte_ether.h       |  2 ++
> >>   4 files changed, 38 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/lib/librte_mbuf/rte_mbuf_ptype.c
> >> b/lib/librte_mbuf/rte_mbuf_ptype.c
> >> index 5d46608..0dea600 100644
> >> --- a/lib/librte_mbuf/rte_mbuf_ptype.c
> >> +++ b/lib/librte_mbuf/rte_mbuf_ptype.c
> >> @@ -41,6 +41,7 @@
> >>   #include <rte_tcp.h>
> >>   #include <rte_udp.h>
> >>   #include <rte_sctp.h>
> >> +#include <rte_mpls.h>
> >>     /* get l3 packet type from ip6 next protocol */
> >>   static uint32_t
> >> @@ -166,6 +167,9 @@ uint32_t rte_pktmbuf_get_ptype(const struct
> >> rte_mbuf *m,
> >>       off = sizeof(*eh);
> >>       hdr_lens->l2_len = off;
> >>   +    if (proto == rte_cpu_to_be_16(ETHER_TYPE_IPv4))
> >> +        goto l3; /* fast path if packet is IPv4 */
> >> +
> >>       if (proto == rte_cpu_to_be_16(ETHER_TYPE_VLAN)) {
> >>           const struct vlan_hdr *vh;
> >>           struct vlan_hdr vh_copy;
> >> @@ -189,8 +193,29 @@ uint32_t rte_pktmbuf_get_ptype(const struct
> >> rte_mbuf *m,
> >>           off += 2 * sizeof(*vh);
> >>           hdr_lens->l2_len += 2 * sizeof(*vh);
> >>           proto = vh->eth_proto;
> >> +    } else if ((proto == rte_cpu_to_be_16(ETHER_TYPE_MPLS)) ||
> >> +            (proto == rte_cpu_to_be_16(ETHER_TYPE_MPLSM))) {
> >> +        unsigned int i;
> >> +        const struct mpls_hdr *mh;
> >> +        struct mpls_hdr mh_copy;
> >> +
> >> +#define MAX_MPLS_HDR 5
> >> +        for (i = 0; i < MAX_MPLS_HDR; i++) {
> >> +            mh = rte_pktmbuf_read(m, off + (i * sizeof(*mh)),
> >> +                sizeof(*mh), &mh_copy);
> >> +            if (unlikely(mh == NULL))
> >> +                return pkt_type;
> >> +            if (mh->bs)
> >> +                break;
> >> +        }
> >> +        if (i == MAX_MPLS_HDR)
> >> +            return pkt_type;
> >> +        pkt_type = RTE_PTYPE_L2_ETHER_MPLS;
> >> +        hdr_lens->l2_len += (sizeof(*mh) * (i + 1));
> > [LC] l2_len includes Eth, Vlan(opt.), MPLS(opt.). For VLAN and MPLS, it
> > may include #n times overlay.
> > These layer recognition knowledge are lost after the detection logic.
> > Once the APP takes the ptype, for the layer(L2, L3, L4) which has more
> > shim-layer, the xxx_len can't help to avoid the re-parse cost.
> 
> This is linked with the definition of packet type. Each layer has a
> type, and here we associate it to a length (by the way the length is
> something we may consider integrate inside the packet type in the future).
[LC] Yes, I see. 
My point is in some case, the length can represent for different layer.
For who interests on L2 MPLS, the length layer scheme maybe can define as {L2/MPLS/inner_L2/inner_L3}.
The rte_mbuf_hdr_lens likes a meta data which associates with the specific parser(assuming customized runtime instance provided by rte_pktmbuf_get_ptype).
The provider understand the meaning and layout.
 
> 
> The packet_type model allows to describe many packets kinds. Some will
> be difficult to represent (ex: a packet with several different L2 or
> L3). But I think this is a good compromise that could help the
> application to get some information without looking inside the packet.
> 
> Changing the packet type structure to something more flexible/complex
> would probably imply to loose time filling it in drivers and parse it in
> the application. And we already have a structure that contains all the
> information needed by the application: the packet data ;)
[LC] Fully agree. Sometimes it's a tradeoff, if the offering meta data by parser is 
not enough for further processing, the duplication packet data walking through may happen.
It's hard to define a meta data format for all cases. Probably the raw META is a good choice, which is recognized by the parser provider.

> 
> In any case, this is not really the topic of the patchset, which just
> provide a helper to parse a packet by software and get a packet_type (as
> it is defined today).
[LC] Maybe the conversation is a little beyond. Hope you get my point.

Thanks.

> 
> Regards,
> Olivier