I was wrong on handling v4/v6 tos. I will re-check man pages for -Q option, maybe it's using old TOS format? I will assume it's going be DSCP for IPv4 and TClass for IPv6 and copy lower 3 bits into MPLS EXP bits.
Thanks, Ravi -----Original Message----- From: dev-boun...@openvswitch.org [mailto:dev-boun...@openvswitch.org] On Behalf Of Ben Pfaff Sent: Thursday, May 24, 2012 4:02 PM To: Kerur, Ravi Cc: dev@openvswitch.org Subject: Re: [ovs-dev] MPLS and VLAN QinQ patch On Thu, May 24, 2012 at 11:58:53PM +0200, ravi.ke...@telekom.com wrote: > Ben Pfaff writes: > > get_l3_ttl_and_tos() > > -------------------- > > > > Why does get_l3_ttl_and_tos() consider an unknown IP version as > > success, with a default? This seems odd. > > To handle non-IP traffic i.e push/pop MPLS over non-IP traffic e.g. > ARP(practical?), VPLS. I had also mentioned in previous discussion > that MPLS ttl actions will not work for these type of traffic but > other actions lke push_mpls,pop_mpls, set_mpls_label/set_mpls_tc would > be. In this function I keep them to default for non-IP traffic. But that only comes up in one situation: copying the TTL inward or outward when there is exactly one MPLS label. In other situations (pushing an MPLS label or popping one) we know the correct inner Ethertype either because it's in the packet or because it's an argument to the action. When we know it, we should use it. > In get_l3_ttl_and_tos(), I'm pretty sure that "ih->tos & 0x07" is > wrong. By my reading, it extracts the two ECN bits and the > lowest-order DSCP bit. Is that really what you want? > > I will check again. I thought IP DSCP bits are 0 - 5 and 6,7 for ECN. > Here the goal is to copy lower 3 bits of DSCP/TOS or copy from mapping > table(based on Service Level Agreement) or configuration from the > controller. Copying from SLA mapping table can be added later, but > configuration can override EXP bits now using "set_mpls_tc" action. Please do check again. We have #define IP_ECN_MASK 0x03 #define IP_DSCP_MASK 0xfc in packets.h and I'm sure that's correct. > > In get_l3_ttl_and_tos(), I wonder about "ih6->ip6_vfc >> 4", too. > > By my reading, this will always equal 6, since we know that this is > > IPv6. I don't think the "6" in IPv6 means that it always has to > > have a TOS of 6. > > IPV6 tos bits are 8 bits long(starting from bit 21), I just take lower > 3 bits from there. Same argument as above. Please check again. We have #define IP_VER(ip_ihl_ver) ((ip_ihl_ver) >> 4) and #define IP6_VER(ip6_vfc) ((ip6_vfc) >> 4) in packets.h and I'm sure that's correct. > > In get_l3_ttl_and_tos(), I don't see any check that the payload is > > long enough to hold the entire IPv4 or IPV6 header. > > I don't see the need, but will fix it if needed. By omitting the check we could incur a segfault. Please check the length. > > lib/packets.c > > ------------- > > > > I see many uses of __be<N> in this file. Those are kernel types. > > In userspace, use ovs_be<N>. > > I double checked I don't see any __be* Hmm, I must have been looked elsewhere. Sorry. > > Why is it still not allowed to push an MPLS header onto non-IP data? > > I think that it should be. > > It does push MPLS onto non-IP data. I have tested it on ARP traffic. push_mpls() calls push_mpls_lse() in this case. push_mpls_lse() starts out this: /* neither ipv4 or ipv6 return; */ which implies that it does nothing to non-IP. Please remove the comment. Thanks, Ben. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
