[dpdk-dev] [PATCH v4 12/12] examples/l3fwd: add option to parse ptype

[Tan, Jianfeng]

Hi Konstantin,

On 2/26/2016 9:14 PM, Ananyev, Konstantin wrote:
> Hi Jianfeng,
>
>> +static int
...
>> +			if (hdr_len == sizeof(struct ipv4_hdr) &&
>> +			    (hdr->next_proto_id == 6 ||
>> +			     hdr->next_proto_id == 17))
> Use IPPORTO_UDP, IPPORTO_TCP instead of hardcoded values.

Yes, will fix it.

>> +				packet_type |= RTE_PTYPE_L3_IPV4;
>> +		}
> Actually it is a good point:
> for EM case should l3fwd process only TCP/UDP packets?
> If yes, then it needs to check not only L3, but also L4 type too
> Which means that for EM and LPM check_packet_type_ok() should also be different.
> Or we can leave it as it is - in that case EM even for non UDP/TCP packet would still
> do route  lookup using first 4B of L3 payload.

I'd like to follow the first approach, (if nobody strongly objects to 
it), because it's EM's real intention to use 5 tuples.

> If you choose first approach, then there is another thing to consider -
> there are 2 patches in flight for l3fwd:
> http://dpdk.org/dev/patchwork/patch/10800/
> http://dpdk.org/dev/patchwork/patch/10782/
>
> Which makes LPM/EM choice for l3fwd a runtime decision.
> So  APP_LOOKUP_METHOD macro would not be available after it.
> Probably need to take that into account for your changes.
> Might be exclude l3fwd from this patch series, then rebase it
> on these patches and submit as a separate one?

Thanks for reminding me of this. And that sounds a good idea to me. This 
commit will be excluded and submitted as a separate one.

>> +#elif (APP_LOOKUP_METHOD == APP_LOOKUP_LPM)
>> +		packet_type |= RTE_PTYPE_L3_IPV4_EXT_UNKNOWN;
>> +#endif
>> +		break;
>> +	case ETHER_TYPE_IPv6:
>> +#if (APP_LOOKUP_METHOD == APP_LOOKUP_EXACT_MATCH)
>> +		{
>> +			struct ipv6_hdr *hdr;
>> +
>> +			hdr = (struct ipv6_hdr *)((uint8_t *)eth_hdr +
>> +						  sizeof(struct ether_hdr));
>> +			if (hdr->proto == 6 || hdr->proto == 17)
>> +				packet_type |= RTE_PTYPE_L3_IPV4;
> s/ RTE_PTYPE_L3_IPV4/RTE_PTYPE_L3_IPV6/
> ?

Oops, nice catch. Will fix it.

Thanks,
Jianfeng

> Apart from that the series looks good to me.
> Konstantin
>