Hi Cunming,

On 07/07/2016 10:48 AM, Liang, Cunming wrote:

Hi Olivier,

quoted

-----Original Message-----
From: Olivier MATZ [mailto:olivier.matz@6wind.com]
Sent: Wednesday, July 06, 2016 4:00 PM
To: Liang, Cunming <redacted>; 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:

quoted

Hi Olivier,

On 7/5/2016 11:41 PM, Olivier Matz wrote:

quoted

Add a new RTE_PTYPE_L2_ETHER_MPLS packet type, and its support in
rte_pktmbuf_get_ptype().

Signed-off-by: Didier Pallard <redacted>
Signed-off-by: Olivier Matz <redacted>
---
  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

quoted

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.

quoted

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.

quoted

Regards,
Olivier