Hi Olivier, > > Hi Konstantin, > > On 06/15/2016 04:08 PM, Ananyev, Konstantin wrote: > > > > Hi Olivier, > > > >> Hi Konstantin, > >> > >> On 04/29/2016 06:03 PM, Ananyev, Konstantin wrote: > >>>> The following commit introduces a function to list the supported > >>>> packet types of a device: > >>>> > >>>> http://dpdk.org/browse/dpdk/commit/?id=78a38edf66 > >>>> > >>>> I would like to know what does "supported" precisely mean. > >>>> Is it: > >>>> > >>>> 1/ - if a ptype is marked as supported, the driver MUST set > >>>> this ptype if the packet matches the format described in rte_mbuf.h > >>>> > >>>> -> if the ptype is not recognized, the application is sure > >>>> that the packet is not one of the supported ptype > >>>> -> but this is difficult to take advantage of this inside an > >>>> application that supports several different ports model > >>>> that do not support the same packet types > >>>> > >>>> 2/ - if a ptype is marked as supported, the driver CAN set > >>>> the ptype if the packet matches the format described in rte_mbuf.h > >>>> > >>>> -> if a ptype is not recognized, the application does a software > >>>> fallback > >>>> -> in this case, it would useless to have the get_supported_ptype() > >>>> > >>>> Can you confirm if the PMDs and l3fwd (the only user) expect 1/ > >>>> or 2/ ? > >>> > >>> 1) > >>> > >>>> > >>>> Can you elaborate on the advantages of having this API? > >>> > >>> Application can rely on information provided by PMD avoid parsing packet > >>> manually to recognise it's pytpe. > >>> > >>>> > >>>> And a supplementary question: if a ptype is not marked as supported, > >>>> is it forbidden for a driver to set this ptype anyway? > >>> > >>> I suppose it is not forbidden, but there is no guarantee from PMD that it > >>> would be able to recognise that ptype. > >>> > >>> Konstantin > >>> > >>>> Because we can > >>>> imagine a hardware that can only recognize packets in some conditions > >>>> (ex: can recognize IPv4 if there is no vlan). > >>>> > >>>> I think properly defining the meaning of "supported" is mandatory > >>>> to have an application beeing able to use this feature, and avoid > >>>> PMDs to behave differently because the API is unclear (like we've > >>>> already seen for other features). > >> > >> Back on this. I've made some tests with ixgbe, and I'm afraid it > >> will be difficult to ensure that when a ptype is advertised, it will > >> always be set in the mbuf, whatever the layers below. Here are some > >> examples: > >> > > > > 1) > > > >> - double vlans > >> > >> Ether(type=0x88A8)/Dot1Q(vlan=0x666)/Dot1Q(vlan=0x666)/IP()/UDP()/Raw("x"*32) > >> ixgbe advertises RTE_PTYPE_ETHER in supported ptypes > >> returned ptype: RTE_PTYPE_UNKNOWN > >> should be: L2_ETHER > >> (this works with any unknown ethtype) > > > > 2) > > > >> > >> - ip6 in ip6 tunnel > >> ixgbe advertises RTE_PTYPE_TUNNEL_IP in supported ptypes > >> Ether()/IPv6()/IPv6()/UDP()/Raw("x"*32) > >> returned ptype: L2_ETHER L3_IPV6 > >> should be: L2_ETHER L3_IPV6 TUNNEL_IP INNER_L3_IPV6 INNER_L4_UDP > > > > 3) > > > >> > >> - ip options > >> Ether()/IP(options=IPOption('\x83\x03\x10'))/UDP()/Raw("x"*32) > >> returned ptype: RTE_PTYPE_UNKNOWN > >> should be: L2_ETHER L3_IPV4_EXT L4_UDP > > > > 4) > > > >> > >> - ip inside ip with options > >> Ether()/IP(options=IPOption('\x83\x03\x10'))/IP()/UDP()/Raw("x"*32) > >> returned ptype: L2_ETHER L3_IPV4_EXT > >> should be: L2_ETHER L3_IPV4_EXT TUNNEL_IP INNER_L3_IPV4 INNER_L4_UDP > > > > > > I am marked your test-cases with numbers to simplify referencing to them. > > 1) & 3) - I believe the reason is a bug in our ptype code inside ixgbe PMD > > :( > > I submitted a patch: > > http://dpdk.org/dev/patchwork/patch/13820/ > > Could you give it a try? > > I think it should fix these issues. > > 1) works, I now have L2_ETHER > 3) works, I now have L2_ETHER L3_IPV4_EXT L4_UDP
Great, thanks for trying :) > > I'll answer to the other thread to so it appears on patchwork. > > > > > 2) & 4) - unfortunately ixgbe HW supports only ipv4->ipv6 tunneling. > > All other combinations are not supported. > > Right now I didn't decide what is a best way to address this problem. > > Two thoughts I have right now: > > - either remove tunnelling recognition (RTE_PTYPE_TUNNEL_IP and > > RTE_PTYPE_INNER_*) > > from supported ptypes in ixgbe_dev_supported_ptypes_get(). > > - or split RTE_PTYPE_TUNNEL_IP into RTE_PTYPE_TUNNEL_IPV4 and > > RTE_PTYPE_TUNNEL_IPV6. > > But that looks a bit ugly, again wuld probably be required for VXLAN/GRE > > and might be other > > tunnelling protocols in future. > > So don't really like any of them. > > If you have any better idea, please shout. > > No better idea. I'd say the first one is better because it does not > require to change the ptypes definition. Agree. > > Another idea (but probably worst) is to do a software fallback > in the PMD, but I don't believe the PMD is the proper place for that. Neither do I. > > >> I'm sure we can find more examples that do not return the expected > >> result, knowing that ixgbe is probably one of the most complete > >> driver in dpdk. I'm afraid of the behavior for other PMDs :) > > > > It is in general, but not for that particular feature :( > > > >> > >> That's why I think the get_supported_ptypes() function, as of today, > >> is useless for an application. > > > > Hmm for me right now it looks more like incorrect implementation. > > OK > > Do you mind if I send a patch to precise a bit what > supported ptype should return? I mean highlighting the fact > that it should return the ptypes that are _always_ recognized, > not the ptypes that _can_ be recognized. But it does not prevent > a PMD to set a ptype that is not in the supported list when > it knows it is correct. Yes, I think it would be a good thing. > > >> I suggest instead to set the ptype > >> in an opportunistic fashion instead: > >> - if the driver/hw knows the ptype, set it > >> - else, set it to unknown > > > > That's what PMD does now... and I don't think it can do much more - > > only interpret information provided by the HW in a proper way. > > Probably I misunderstood you here... > > My suggestion was to remove get_supported_ptypes an set the ptype in > mbuf when the pmd recognize a type. > > But we could also keep get_supported_ptypes() for ptypes that will > always be recognized, allowing a PMD to set any other ptype if it > is recognized. This is probably what we have today in some PMDs, I > think it just requires some clarification. Yes, +1 to the second option. Konstantin
