Hi Cunming,
On 07/07/2016 10:48 AM, Liang, Cunming wrote:
> 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.
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
