| Message ID | 1452496694-50996-3-git-send-email-pshelar@nicira.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Sun, Jan 10, 2016 at 11:18 PM, Pravin B Shelar <pshelar@nicira.com> wrote: > diff --git a/lib/packets.c b/lib/packets.c > index d82341d..a910f50 100644 > --- a/lib/packets.c > +++ b/lib/packets.c > +void > +IP_ECN_set_ce(struct dp_packet *pkt, bool is_ipv6) > +{ > + if (is_ipv6) { > + struct ovs_16aligned_ip6_hdr *ip6 = dp_packet_l3(pkt); > + > + put_16aligned_be32((ovs_16aligned_be32 *)ip6, htonl(IP_ECN_CE << 20)); Isn't this going to blow away the rest of the things that are already in that word? > + } else { > + struct ip_header *nh = dp_packet_l4(pkt); This should be the L3 header, right? > + uint8_t tos = nh->ip_tos; > + > + tos |= IP_ECN_CE; > + if (nh->ip_tos != tos) { > + uint8_t *field = &nh->ip_tos; > + > + nh->ip_csum = recalc_csum16(nh->ip_csum, htons((uint16_t) *field), > + htons((uint16_t) tos)); > + *field = tos; Can we just pass the original nh->ip_tos in instead of using the field pointer? It seems simpler. What about checking to see if the packet is originally ECT or even IP? It doesn't inherently have to be done in this function but skipping ahead to the STT patch I don't see that we check when it is called. (I see the check for IPv6 but it seems to me that it will interpret all non-IP traffic as IPv4.)
On Thu, Jan 21, 2016 at 1:29 PM, Jesse Gross <jesse@kernel.org> wrote: > On Sun, Jan 10, 2016 at 11:18 PM, Pravin B Shelar <pshelar@nicira.com> wrote: >> diff --git a/lib/packets.c b/lib/packets.c >> index d82341d..a910f50 100644 >> --- a/lib/packets.c >> +++ b/lib/packets.c >> +void >> +IP_ECN_set_ce(struct dp_packet *pkt, bool is_ipv6) >> +{ >> + if (is_ipv6) { >> + struct ovs_16aligned_ip6_hdr *ip6 = dp_packet_l3(pkt); >> + >> + put_16aligned_be32((ovs_16aligned_be32 *)ip6, htonl(IP_ECN_CE << 20)); > > Isn't this going to blow away the rest of the things that are already > in that word? > right, I will fix it. >> + } else { >> + struct ip_header *nh = dp_packet_l4(pkt); > > This should be the L3 header, right? > yes. >> + uint8_t tos = nh->ip_tos; >> + >> + tos |= IP_ECN_CE; >> + if (nh->ip_tos != tos) { >> + uint8_t *field = &nh->ip_tos; >> + >> + nh->ip_csum = recalc_csum16(nh->ip_csum, htons((uint16_t) *field), >> + htons((uint16_t) tos)); >> + *field = tos; > > Can we just pass the original nh->ip_tos in instead of using the field > pointer? It seems simpler. > ok. > What about checking to see if the packet is originally ECT or even IP? > It doesn't inherently have to be done in this function but skipping > ahead to the STT patch I don't see that we check when it is called. (I > see the check for IPv6 but it seems to me that it will interpret all > non-IP traffic as IPv4.) > I do not think stt pop can receive any other protocol packet than IPv6 or IPv4. That is how the packet filter is set in tnl-port.c. _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev
On Thu, Jan 21, 2016 at 1:50 PM, pravin shelar <pshelar@ovn.org> wrote: > On Thu, Jan 21, 2016 at 1:29 PM, Jesse Gross <jesse@kernel.org> wrote: >> What about checking to see if the packet is originally ECT or even IP? >> It doesn't inherently have to be done in this function but skipping >> ahead to the STT patch I don't see that we check when it is called. (I >> see the check for IPv6 but it seems to me that it will interpret all >> non-IP traffic as IPv4.) >> > I do not think stt pop can receive any other protocol packet than IPv6 > or IPv4. That is how the packet filter is set in tnl-port.c. OK, I see it is actually being called on the outer header, not the inner, and we'll move this to the inner header in the tunnel flow setup code.
diff --git a/lib/packets.c b/lib/packets.c index d82341d..a910f50 100644 --- a/lib/packets.c +++ b/lib/packets.c @@ -1292,3 +1292,25 @@ packet_csum_pseudoheader6(const struct ovs_16aligned_ip6_hdr *ip6) return partial; } #endif + +void +IP_ECN_set_ce(struct dp_packet *pkt, bool is_ipv6) +{ + if (is_ipv6) { + struct ovs_16aligned_ip6_hdr *ip6 = dp_packet_l3(pkt); + + put_16aligned_be32((ovs_16aligned_be32 *)ip6, htonl(IP_ECN_CE << 20)); + } else { + struct ip_header *nh = dp_packet_l4(pkt); + uint8_t tos = nh->ip_tos; + + tos |= IP_ECN_CE; + if (nh->ip_tos != tos) { + uint8_t *field = &nh->ip_tos; + + nh->ip_csum = recalc_csum16(nh->ip_csum, htons((uint16_t) *field), + htons((uint16_t) tos)); + *field = tos; + } + } +} diff --git a/lib/packets.h b/lib/packets.h index 834e8a4..2157657 100644 --- a/lib/packets.h +++ b/lib/packets.h @@ -603,6 +603,12 @@ char *ip_parse_cidr(const char *s, ovs_be32 *ip, unsigned int *plen) #define IP_ECN_MASK 0x03 #define IP_DSCP_MASK 0xfc +static inline int +IP_ECN_is_ce(uint8_t dsfield) +{ + return (dsfield & IP_ECN_MASK) == IP_ECN_CE; +} + #define IP_VERSION 4 #define IP_DONT_FRAGMENT 0x4000 /* Don't fragment. */ @@ -1061,5 +1067,6 @@ void compose_arp(struct dp_packet *, uint16_t arp_op, void compose_nd(struct dp_packet *, const struct eth_addr eth_src, struct in6_addr *, struct in6_addr *); uint32_t packet_csum_pseudoheader(const struct ip_header *); +void IP_ECN_set_ce(struct dp_packet *pkt, bool is_ipv6); #endif /* packets.h */ diff --git a/ofproto/tunnel.c b/ofproto/tunnel.c index 5cf6c75..7ac8841 100644 --- a/ofproto/tunnel.c +++ b/ofproto/tunnel.c @@ -342,7 +342,7 @@ tnl_process_ecn(struct flow *flow) return true; } - if (is_ip_any(flow) && (flow->tunnel.ip_tos & IP_ECN_MASK) == IP_ECN_CE) { + if (is_ip_any(flow) && IP_ECN_is_ce(flow->tunnel.ip_tos)) { if ((flow->nw_tos & IP_ECN_MASK) == IP_ECN_NOT_ECT) { VLOG_WARN_RL(&rl, "dropping tunnel packet marked ECN CE" " but is not ECN capable"); @@ -382,7 +382,7 @@ tnl_wc_init(struct flow *flow, struct flow_wildcards *wc) memset(&wc->masks.pkt_mark, 0xff, sizeof wc->masks.pkt_mark); if (is_ip_any(flow) - && (flow->tunnel.ip_tos & IP_ECN_MASK) == IP_ECN_CE) { + && IP_ECN_is_ce(flow->tunnel.ip_tos)) { wc->masks.nw_tos |= IP_ECN_MASK; } } @@ -451,7 +451,7 @@ tnl_port_send(const struct ofport_dpif *ofport, struct flow *flow, if (is_ip_any(flow)) { wc->masks.nw_tos |= IP_ECN_MASK; - if ((flow->nw_tos & IP_ECN_MASK) == IP_ECN_CE) { + if (IP_ECN_is_ce(flow->nw_tos)) { flow->tunnel.ip_tos |= IP_ECN_ECT_0; } else { flow->tunnel.ip_tos |= flow->nw_tos & IP_ECN_MASK;
This function will be used by STT tunnel. Signed-off-by: Pravin B Shelar <pshelar@nicira.com> --- lib/packets.c | 22 ++++++++++++++++++++++ lib/packets.h | 7 +++++++ ofproto/tunnel.c | 6 +++--- 3 files changed, 32 insertions(+), 3 deletions(-)