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
