| Message ID | 1468306616-125783-17-git-send-email-jpettit@ovn.org |
|---|---|
| State | Changes Requested |
| Headers | show |
On Tue, Jul 12, 2016 at 2:56 PM, Justin Pettit <jpettit@ovn.org> wrote: > Rename "compose_nd" and "compose_na" to "compose_nd_sol" and > "compose_nd_adv", respecively, to be clearer about their functionality. > Also change the source and destination IPv6 addresses to take > "struct in6_addr" arguments, which are more common in the code base. > > Signed-off-by: Justin Pettit <jpettit@ovn.org> > --- > eth_compose(b, eth_dst, eth_src, ETH_TYPE_IPV6, IPV6_HEADER_LEN); > - na = compose_ipv6(b, IPPROTO_ICMPV6, ipv6_src, ipv6_dst, 0, 0, 255, > - ND_MSG_LEN + ND_OPT_LEN); > + na = compose_ipv6(b, IPPROTO_ICMPV6, > + ALIGNED_CAST(ovs_be32 *, ipv6_src->s6_addr), > + ALIGNED_CAST(ovs_be32 *, ipv6_dst->s6_addr), > + 0, 0, 255, ND_MSG_LEN + ND_OPT_LEN); > Hi, Justin. It's quite a long time to wait support for IPv6, but I think it worth. Thanks. :) About using ALIGNED_CAST, would you mind checking http://patchwork.ozlabs.org/patch/632026/ ? Ben has a comment said "Using ALIGNED_CAST does not solve a problem, it only suppresses a warning.". So I'm not sure will this be a good way to cleanup. Thanks. Zong Kai, LI
On Tue, Jul 12, 2016 at 07:26:29PM +0800, Zong Kai Li wrote: > On Tue, Jul 12, 2016 at 2:56 PM, Justin Pettit <jpettit@ovn.org> wrote: > > Rename "compose_nd" and "compose_na" to "compose_nd_sol" and > > "compose_nd_adv", respecively, to be clearer about their functionality. > > Also change the source and destination IPv6 addresses to take > > "struct in6_addr" arguments, which are more common in the code base. > > > > Signed-off-by: Justin Pettit <jpettit@ovn.org> > > --- > > > eth_compose(b, eth_dst, eth_src, ETH_TYPE_IPV6, IPV6_HEADER_LEN); > > - na = compose_ipv6(b, IPPROTO_ICMPV6, ipv6_src, ipv6_dst, 0, 0, 255, > > - ND_MSG_LEN + ND_OPT_LEN); > > + na = compose_ipv6(b, IPPROTO_ICMPV6, > > + ALIGNED_CAST(ovs_be32 *, ipv6_src->s6_addr), > > + ALIGNED_CAST(ovs_be32 *, ipv6_dst->s6_addr), > > + 0, 0, 255, ND_MSG_LEN + ND_OPT_LEN); > > > > Hi, Justin. It's quite a long time to wait support for IPv6, but I > think it worth. Thanks. :) > > About using ALIGNED_CAST, would you mind checking > http://patchwork.ozlabs.org/patch/632026/ ? > Ben has a comment said "Using ALIGNED_CAST does not solve a problem, > it only suppresses a warning.". > So I'm not sure will this be a good way to cleanup. Yes, I agree. Here is a more detailed explanation. I guess that most people reading this email know that RISC machines fault for misaligned accesses. In some cases, 32-bit objects inside packets will be aligned only to a 16-bit boundary. Thus, we typically use types like ovs_16aligned_in6_addr to make this obvious and harder to get wrong. GCC will warn for a cast from a type that requires less alignment (e.g. 16-bit for ovs_16aligned_in6_addr) to a type that requires more alignment (e.g. 32-bit for ovs_be32). We can use ALIGNED_CAST to suppress this if we know that the object is actually correctly aligned for the more strictly aligned type, but if this is not the case then it does not solve any problem, it only suppresses a warning and there is still a time bomb hidden in the code for RISC architectures.
On Mon, Jul 11, 2016 at 11:56:46PM -0700, Justin Pettit wrote: > Rename "compose_nd" and "compose_na" to "compose_nd_sol" and > "compose_nd_adv", respecively, to be clearer about their functionality. > Also change the source and destination IPv6 addresses to take > "struct in6_addr" arguments, which are more common in the code base. > > Signed-off-by: Justin Pettit <jpettit@ovn.org> In addition to the comments from elsethread: I was able to decode the names of the functions, with some thought, but it would be helpful to add a comment on each one explaining what it does.
> On Jul 13, 2016, at 12:36 PM, Ben Pfaff <blp@ovn.org> wrote: > > On Tue, Jul 12, 2016 at 07:26:29PM +0800, Zong Kai Li wrote: >> On Tue, Jul 12, 2016 at 2:56 PM, Justin Pettit <jpettit@ovn.org> wrote: >>> Rename "compose_nd" and "compose_na" to "compose_nd_sol" and >>> "compose_nd_adv", respecively, to be clearer about their functionality. >>> Also change the source and destination IPv6 addresses to take >>> "struct in6_addr" arguments, which are more common in the code base. >>> >>> Signed-off-by: Justin Pettit <jpettit@ovn.org> >>> --- >> >>> eth_compose(b, eth_dst, eth_src, ETH_TYPE_IPV6, IPV6_HEADER_LEN); >>> - na = compose_ipv6(b, IPPROTO_ICMPV6, ipv6_src, ipv6_dst, 0, 0, 255, >>> - ND_MSG_LEN + ND_OPT_LEN); >>> + na = compose_ipv6(b, IPPROTO_ICMPV6, >>> + ALIGNED_CAST(ovs_be32 *, ipv6_src->s6_addr), >>> + ALIGNED_CAST(ovs_be32 *, ipv6_dst->s6_addr), >>> + 0, 0, 255, ND_MSG_LEN + ND_OPT_LEN); >>> >> >> Hi, Justin. It's quite a long time to wait support for IPv6, but I >> think it worth. Thanks. :) >> >> About using ALIGNED_CAST, would you mind checking >> http://patchwork.ozlabs.org/patch/632026/ ? >> Ben has a comment said "Using ALIGNED_CAST does not solve a problem, >> it only suppresses a warning.". >> So I'm not sure will this be a good way to cleanup. > > Yes, I agree. > > Here is a more detailed explanation. I guess that most people reading > this email know that RISC machines fault for misaligned accesses. In > some cases, 32-bit objects inside packets will be aligned only to a > 16-bit boundary. Thus, we typically use types like > ovs_16aligned_in6_addr to make this obvious and harder to get wrong. > GCC will warn for a cast from a type that requires less alignment > (e.g. 16-bit for ovs_16aligned_in6_addr) to a type that requires more > alignment (e.g. 32-bit for ovs_be32). We can use ALIGNED_CAST to > suppress this if we know that the object is actually correctly aligned > for the more strictly aligned type, but if this is not the case then it > does not solve any problem, it only suppresses a warning and there is > still a time bomb hidden in the code for RISC architectures. Good point. I'll send out a v2. We'll see if I got it right. :-) --Justin
diff --git a/lib/packets.c b/lib/packets.c index a27264c..9e4d0e7 100644 --- a/lib/packets.c +++ b/lib/packets.c @@ -1320,8 +1320,8 @@ compose_ipv6(struct dp_packet *packet, uint8_t proto, const ovs_be32 src[4], } void -compose_nd(struct dp_packet *b, const struct eth_addr eth_src, - struct in6_addr *ipv6_src, struct in6_addr *ipv6_dst) +compose_nd_sol(struct dp_packet *b, const struct eth_addr eth_src, + const struct in6_addr *ipv6_src, const struct in6_addr *ipv6_dst) { struct in6_addr sn_addr; struct eth_addr eth_dst; @@ -1336,8 +1336,7 @@ compose_nd(struct dp_packet *b, const struct eth_addr eth_src, ns = compose_ipv6(b, IPPROTO_ICMPV6, ALIGNED_CAST(ovs_be32 *, ipv6_src->s6_addr), ALIGNED_CAST(ovs_be32 *, sn_addr.s6_addr), - 0, 0, 255, - ND_MSG_LEN + ND_OPT_LEN); + 0, 0, 255, ND_MSG_LEN + ND_OPT_LEN); ns->icmph.icmp6_type = ND_NEIGHBOR_SOLICIT; ns->icmph.icmp6_code = 0; @@ -1356,18 +1355,20 @@ compose_nd(struct dp_packet *b, const struct eth_addr eth_src, } void -compose_na(struct dp_packet *b, - const struct eth_addr eth_src, const struct eth_addr eth_dst, - const ovs_be32 ipv6_src[4], const ovs_be32 ipv6_dst[4], - ovs_be32 rco_flags) +compose_nd_adv(struct dp_packet *b, + const struct eth_addr eth_src, const struct eth_addr eth_dst, + const struct in6_addr *ipv6_src, const struct in6_addr *ipv6_dst, + ovs_be32 rco_flags) { struct ovs_nd_msg *na; struct ovs_nd_opt *nd_opt; uint32_t icmp_csum; eth_compose(b, eth_dst, eth_src, ETH_TYPE_IPV6, IPV6_HEADER_LEN); - na = compose_ipv6(b, IPPROTO_ICMPV6, ipv6_src, ipv6_dst, 0, 0, 255, - ND_MSG_LEN + ND_OPT_LEN); + na = compose_ipv6(b, IPPROTO_ICMPV6, + ALIGNED_CAST(ovs_be32 *, ipv6_src->s6_addr), + ALIGNED_CAST(ovs_be32 *, ipv6_dst->s6_addr), + 0, 0, 255, ND_MSG_LEN + ND_OPT_LEN); na->icmph.icmp6_type = ND_NEIGHBOR_ADVERT; na->icmph.icmp6_code = 0; @@ -1377,7 +1378,8 @@ compose_na(struct dp_packet *b, nd_opt->nd_opt_type = ND_OPT_TARGET_LINKADDR; nd_opt->nd_opt_len = 1; - packet_set_nd(b, ipv6_src, eth_addr_zero, eth_src); + packet_set_nd(b, ALIGNED_CAST(ovs_be32 *, ipv6_src->s6_addr), + eth_addr_zero, eth_src); na->icmph.icmp6_cksum = 0; icmp_csum = packet_csum_pseudoheader6(dp_packet_l3(b)); na->icmph.icmp6_cksum = csum_finish(csum_continue(icmp_csum, na, diff --git a/lib/packets.h b/lib/packets.h index 077ccfa..06166de 100644 --- a/lib/packets.h +++ b/lib/packets.h @@ -1067,12 +1067,14 @@ void compose_arp(struct dp_packet *, uint16_t arp_op, const struct eth_addr arp_sha, const struct eth_addr arp_tha, bool broadcast, ovs_be32 arp_spa, ovs_be32 arp_tpa); -void compose_nd(struct dp_packet *, const struct eth_addr eth_src, - struct in6_addr *, struct in6_addr *); -void compose_na(struct dp_packet *, - const struct eth_addr eth_src, const struct eth_addr eth_dst, - const ovs_be32 ipv6_src[4], const ovs_be32 ipv6_dst[4], - ovs_be32 rco_flags); +void compose_nd_sol(struct dp_packet *, const struct eth_addr eth_src, + const struct in6_addr *ipv6_src, + const struct in6_addr *ipv6_dst); +void compose_nd_adv(struct dp_packet *, const struct eth_addr eth_src, + const struct eth_addr eth_dst, + const struct in6_addr *ipv6_src, + const struct in6_addr *ipv6_dst, + ovs_be32 rco_flags); uint32_t packet_csum_pseudoheader(const struct ip_header *); void IP_ECN_set_ce(struct dp_packet *pkt, bool is_ipv6); diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 16fcea3..9aef5e6 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -2875,7 +2875,7 @@ tnl_send_nd_request(struct xlate_ctx *ctx, const struct xport *out_dev, struct dp_packet packet; dp_packet_init(&packet, 0); - compose_nd(&packet, eth_src, ipv6_src, ipv6_dst); + compose_nd_sol(&packet, eth_src, ipv6_src, ipv6_dst); compose_table_xlate(ctx, out_dev, &packet); dp_packet_uninit(&packet); } diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c index 1370301..37d3f41 100644 --- a/ovn/controller/pinctrl.c +++ b/ovn/controller/pinctrl.c @@ -964,15 +964,11 @@ pinctrl_handle_na(const struct flow *ip_flow, struct dp_packet packet; dp_packet_use_stub(&packet, packet_stub, sizeof packet_stub); - ovs_be32 ipv6_src[4], ipv6_dst[4]; - memcpy(ipv6_dst, &ip_flow->ipv6_src, sizeof ipv6_src); - memcpy(ipv6_src, &ip_flow->nd_target, sizeof ipv6_dst); - /* Frame the NA packet with RSO=011. */ - compose_na(&packet, - ip_flow->dl_dst, ip_flow->dl_src, - ipv6_src, ipv6_dst, - htonl(0x60000000)); + compose_nd_adv(&packet, + ip_flow->dl_dst, ip_flow->dl_src, + &ip_flow->nd_target, &ip_flow->ipv6_src, + htonl(0x60000000)); /* Reload previous packet metadata. */ uint64_t ofpacts_stub[4096 / 8];
Rename "compose_nd" and "compose_na" to "compose_nd_sol" and "compose_nd_adv", respecively, to be clearer about their functionality. Also change the source and destination IPv6 addresses to take "struct in6_addr" arguments, which are more common in the code base. Signed-off-by: Justin Pettit <jpettit@ovn.org> --- lib/packets.c | 24 +++++++++++++----------- lib/packets.h | 14 ++++++++------ ofproto/ofproto-dpif-xlate.c | 2 +- ovn/controller/pinctrl.c | 12 ++++-------- 4 files changed, 26 insertions(+), 26 deletions(-)