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

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

Hi Cunming,

On 07/07/2016 10:48 AM, Liang, Cunming wrote:
> Hi Olivier,
> 
>> -----Original Message-----
>> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
>> Sent: Wednesday, July 06, 2016 4:00 PM
>> To: Liang, Cunming <cunming.liang@intel.com>; dev@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@6wind.com>
>>>> Signed-off-by: Olivier Matz <olivier.matz@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.

OK, I see.

For VLAN or QinQ, we could consider that it is the same L2 than the
Ethernet header.
But maybe MPLS should not be part of this patchset, because it's
actually a bit different. The choice I've made was to represent MPLS in
packet_type like this:

  Ether - MPLS - IP - TCP
  \         /     |    |
      L2         L3    L4

Another way to represent it would be:

  Ether - MPLS - IP - TCP
    |      |      |    |
   L2   INNER_L2    INNER_L4
              INNER_L3

If it's too confusing, we may remove MPLS from this patchset.


Regards,
Olivier



>  
>>
>> 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