| Message ID | d21a4352-ff65-b17b-bc67-ba3d495614e8@samsung.com |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, Jul 24, 2017 at 6:33 AM, Ilya Maximets <i.maximets@samsung.com> wrote: > On 22.07.2017 01:38, Andy Zhou wrote: >> On Wed, Jul 19, 2017 at 7:51 AM, Ilya Maximets <i.maximets@samsung.com> wrote: >>> This allows to compose packets with different real lenghts from >>> odp flows i.e. memory will be allocated for requested packet >>> size and all required headers like ip->tot_len filled correctly. >>> >>> Will be used in netdev-dummy to properly handle '--len' option. >>> >>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> >> >> Thank you for working on this. Although those functions are mainly >> for testing, it is still good that we improve them. >> >> I am wondering about a slightly different approach. Instead of adding >> 'packet_size' to the flow_compose() interface, would it make >> sense to come up with a new function whose task is to >> expand a packet to the final length, (similar to flow_compose_l4_csum()) >> >> We would first create the necessary headers for all layers based on >> flow, without fill in the actual size related field or compute checksums. >> >> Then the fix size function will take over, fill in data, and >> update various headers. >> >> Then checksums can be computed and filled in. >> >> I think the logics will be easier to follow with this approach. What >> do you think? > > > I thought about this. I just tried to avoid double packet parsing, > but such approach could be interesting. This approach looks fine to me. > > Below is the possible implementation. > If you think that it's better than the modification of flow_compose(), > I can send v2 with below implementation: I don't think that eth_from_flow() adds much value. Would it be less clear if we just use flow_compose_xxx() APIs ? Please go ahead with V2. Looking forward to it. > > --8<----------------------------------------------------------------------->8-- > diff --git a/lib/flow.c b/lib/flow.c > index e1597fa..ce99c06 100644 > --- a/lib/flow.c > +++ b/lib/flow.c > @@ -2706,40 +2706,87 @@ flow_compose_l4_csum(struct dp_packet *p, const struct flow *flow, > if (flow->nw_proto == IPPROTO_TCP) { > struct tcp_header *tcp = dp_packet_l4(p); > > - /* Checksum has already been zeroed by put_zeros call in > - * flow_compose_l4(). */ > + tcp->tcp_csum = 0; > tcp->tcp_csum = csum_finish(csum_continue(pseudo_hdr_csum, > tcp, l4_len)); > } else if (flow->nw_proto == IPPROTO_UDP) { > struct udp_header *udp = dp_packet_l4(p); > > - /* Checksum has already been zeroed by put_zeros call in > - * flow_compose_l4(). */ > + udp->udp_csum = 0; > udp->udp_csum = csum_finish(csum_continue(pseudo_hdr_csum, > udp, l4_len)); > } else if (flow->nw_proto == IPPROTO_ICMP) { > struct icmp_header *icmp = dp_packet_l4(p); > > - /* Checksum has already been zeroed by put_zeros call in > - * flow_compose_l4(). */ > + icmp->icmp_csum = 0; > icmp->icmp_csum = csum(icmp, l4_len); > } else if (flow->nw_proto == IPPROTO_IGMP) { > struct igmp_header *igmp = dp_packet_l4(p); > > - /* Checksum has already been zeroed by put_zeros call in > - * flow_compose_l4(). */ > + igmp->igmp_csum = 0; > igmp->igmp_csum = csum(igmp, l4_len); > } else if (flow->nw_proto == IPPROTO_ICMPV6) { > struct icmp6_hdr *icmp = dp_packet_l4(p); > > - /* Checksum has already been zeroed by put_zeros call in > - * flow_compose_l4(). */ > + icmp->icmp6_cksum = 0; > icmp->icmp6_cksum = (OVS_FORCE uint16_t) > csum_finish(csum_continue(pseudo_hdr_csum, icmp, l4_len)); > } > } > } > > +/* Tries to increase the size of packet composed by 'flow_compose' up to > + * 'size' bytes. Fixes all the required packet headers like ip/udp lengths > + * and l3/l4 checksums. */ > +void > +flow_compose_size(struct dp_packet *p, const struct flow *flow, size_t size) > +{ > + size_t extra_size, l4_len; > + uint32_t pseudo_hdr_csum; > + > + if (size <= dp_packet_size(p)) { > + return; > + } > + > + extra_size = size - dp_packet_size(p); > + dp_packet_put_zeros(p, extra_size); > + > + l4_len = (char *) dp_packet_tail(p) - (char *) dp_packet_l4(p); > + > + if (flow->dl_type == htons(FLOW_DL_TYPE_NONE)) { > + struct eth_header *eth = dp_packet_eth(p); > + > + eth->eth_type = htons(dp_packet_size(p)); > + return; > + } > + > + if (flow->dl_type == htons(ETH_TYPE_IP)) { > + struct ip_header *ip = dp_packet_l3(p); > + > + ip->ip_tot_len = htons(p->l4_ofs - p->l3_ofs + l4_len); > + ip->ip_csum = 0; > + ip->ip_csum = csum(ip, sizeof *ip); > + > + pseudo_hdr_csum = packet_csum_pseudoheader(ip); > + } else if (flow->dl_type == htons(ETH_TYPE_IPV6)) { > + struct ovs_16aligned_ip6_hdr *nh = dp_packet_l3(p); > + > + nh->ip6_plen = htons(l4_len); > + pseudo_hdr_csum = packet_csum_pseudoheader6(nh); > + } > + > + if (dl_type_is_ip_any(flow->dl_type)) { > + if ((!(flow->nw_frag & FLOW_NW_FRAG_ANY) > + || !(flow->nw_frag & FLOW_NW_FRAG_LATER)) > + && flow->nw_proto == IPPROTO_UDP) { > + struct udp_header *udp = dp_packet_l4(p); > + > + udp->udp_len = htons(l4_len + extra_size); > + } > + flow_compose_l4_csum(p, flow, pseudo_hdr_csum); > + } > +} > + > /* Puts into 'p' a packet that flow_extract() would parse as having the given > * 'flow'. > * > diff --git a/lib/flow.h b/lib/flow.h > index 9297842..1bbbe41 100644 > --- a/lib/flow.h > +++ b/lib/flow.h > @@ -125,6 +125,7 @@ void flow_set_mpls_bos(struct flow *, int idx, uint8_t stack); > void flow_set_mpls_lse(struct flow *, int idx, ovs_be32 lse); > > void flow_compose(struct dp_packet *, const struct flow *); > +void flow_compose_size(struct dp_packet *, const struct flow *, size_t size); > > bool parse_ipv6_ext_hdrs(const void **datap, size_t *sizep, uint8_t *nw_proto, > uint8_t *nw_frag); > diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c > index 51d29d5..83e30b3 100644 > --- a/lib/netdev-dummy.c > +++ b/lib/netdev-dummy.c > @@ -1449,7 +1449,7 @@ eth_from_packet(const char *s) > } > > static struct dp_packet * > -eth_from_flow(const char *s) > +eth_from_flow(const char *s, size_t packet_size) > { > enum odp_key_fitness fitness; > struct dp_packet *packet; > @@ -1479,6 +1479,7 @@ eth_from_flow(const char *s) > > packet = dp_packet_new(0); > flow_compose(packet, &flow); > + flow_compose_size(packet, &flow, packet_size); > > ofpbuf_uninit(&odp_key); > return packet; > @@ -1556,20 +1557,26 @@ netdev_dummy_receive(struct unixctl_conn *conn, > packet = eth_from_packet(argv[i]); > > if (!packet) { > + int packet_size = 0; > + const char *flow_str = argv[i]; > + > + /* Parse optional --len argument immediately follows a 'flow'. */ > + if (argc >= i + 2 && !strcmp(argv[i + 1], "--len")) { > + packet_size = strtol(argv[i + 2], NULL, 10); > + > + if (packet_size < ETH_TOTAL_MIN) { > + unixctl_command_reply_error(conn, "too small packet len"); > + goto exit; > + } > + i+=2; > + } > /* Try parse 'argv[i]' as odp flow. */ > - packet = eth_from_flow(argv[i]); > + packet = eth_from_flow(flow_str, packet_size); > > if (!packet) { > unixctl_command_reply_error(conn, "bad packet or flow syntax"); > goto exit; > } > - > - /* Parse optional --len argument immediately follows a 'flow'. */ > - if (argc >= i + 2 && !strcmp(argv[i + 1], "--len")) { > - int packet_size = strtol(argv[i + 2], NULL, 10); > - dp_packet_set_size(packet, packet_size); > - i+=2; > - } > } > > netdev_dummy_queue_packet(dummy_dev, packet, rx_qid); > --8<----------------------------------------------------------------------->8--
On 24.07.2017 22:40, Andy Zhou wrote: > On Mon, Jul 24, 2017 at 6:33 AM, Ilya Maximets <i.maximets@samsung.com> wrote: >> On 22.07.2017 01:38, Andy Zhou wrote: >>> On Wed, Jul 19, 2017 at 7:51 AM, Ilya Maximets <i.maximets@samsung.com> wrote: >>>> This allows to compose packets with different real lenghts from >>>> odp flows i.e. memory will be allocated for requested packet >>>> size and all required headers like ip->tot_len filled correctly. >>>> >>>> Will be used in netdev-dummy to properly handle '--len' option. >>>> >>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> >>> >>> Thank you for working on this. Although those functions are mainly >>> for testing, it is still good that we improve them. >>> >>> I am wondering about a slightly different approach. Instead of adding >>> 'packet_size' to the flow_compose() interface, would it make >>> sense to come up with a new function whose task is to >>> expand a packet to the final length, (similar to flow_compose_l4_csum()) >>> >>> We would first create the necessary headers for all layers based on >>> flow, without fill in the actual size related field or compute checksums. >>> >>> Then the fix size function will take over, fill in data, and >>> update various headers. >>> >>> Then checksums can be computed and filled in. >>> >>> I think the logics will be easier to follow with this approach. What >>> do you think? >> >> >> I thought about this. I just tried to avoid double packet parsing, >> but such approach could be interesting. > > This approach looks fine to me. >> >> Below is the possible implementation. >> If you think that it's better than the modification of flow_compose(), >> I can send v2 with below implementation: > > I don't think that eth_from_flow() adds much value. Would it > be less clear if we just use flow_compose_xxx() APIs ? String to flow parsing code complicates receive function. I don't see a beautiful solution right now. I'll send v2 with netdev-dummy changes as below. > Please go ahead with V2. Looking forward to it. > >> >> --8<----------------------------------------------------------------------->8-- >> diff --git a/lib/flow.c b/lib/flow.c >> index e1597fa..ce99c06 100644 >> --- a/lib/flow.c >> +++ b/lib/flow.c >> @@ -2706,40 +2706,87 @@ flow_compose_l4_csum(struct dp_packet *p, const struct flow *flow, >> if (flow->nw_proto == IPPROTO_TCP) { >> struct tcp_header *tcp = dp_packet_l4(p); >> >> - /* Checksum has already been zeroed by put_zeros call in >> - * flow_compose_l4(). */ >> + tcp->tcp_csum = 0; >> tcp->tcp_csum = csum_finish(csum_continue(pseudo_hdr_csum, >> tcp, l4_len)); >> } else if (flow->nw_proto == IPPROTO_UDP) { >> struct udp_header *udp = dp_packet_l4(p); >> >> - /* Checksum has already been zeroed by put_zeros call in >> - * flow_compose_l4(). */ >> + udp->udp_csum = 0; >> udp->udp_csum = csum_finish(csum_continue(pseudo_hdr_csum, >> udp, l4_len)); >> } else if (flow->nw_proto == IPPROTO_ICMP) { >> struct icmp_header *icmp = dp_packet_l4(p); >> >> - /* Checksum has already been zeroed by put_zeros call in >> - * flow_compose_l4(). */ >> + icmp->icmp_csum = 0; >> icmp->icmp_csum = csum(icmp, l4_len); >> } else if (flow->nw_proto == IPPROTO_IGMP) { >> struct igmp_header *igmp = dp_packet_l4(p); >> >> - /* Checksum has already been zeroed by put_zeros call in >> - * flow_compose_l4(). */ >> + igmp->igmp_csum = 0; >> igmp->igmp_csum = csum(igmp, l4_len); >> } else if (flow->nw_proto == IPPROTO_ICMPV6) { >> struct icmp6_hdr *icmp = dp_packet_l4(p); >> >> - /* Checksum has already been zeroed by put_zeros call in >> - * flow_compose_l4(). */ >> + icmp->icmp6_cksum = 0; >> icmp->icmp6_cksum = (OVS_FORCE uint16_t) >> csum_finish(csum_continue(pseudo_hdr_csum, icmp, l4_len)); >> } >> } >> } >> >> +/* Tries to increase the size of packet composed by 'flow_compose' up to >> + * 'size' bytes. Fixes all the required packet headers like ip/udp lengths >> + * and l3/l4 checksums. */ >> +void >> +flow_compose_size(struct dp_packet *p, const struct flow *flow, size_t size) >> +{ >> + size_t extra_size, l4_len; >> + uint32_t pseudo_hdr_csum; >> + >> + if (size <= dp_packet_size(p)) { >> + return; >> + } >> + >> + extra_size = size - dp_packet_size(p); >> + dp_packet_put_zeros(p, extra_size); >> + >> + l4_len = (char *) dp_packet_tail(p) - (char *) dp_packet_l4(p); >> + >> + if (flow->dl_type == htons(FLOW_DL_TYPE_NONE)) { >> + struct eth_header *eth = dp_packet_eth(p); >> + >> + eth->eth_type = htons(dp_packet_size(p)); >> + return; >> + } >> + >> + if (flow->dl_type == htons(ETH_TYPE_IP)) { >> + struct ip_header *ip = dp_packet_l3(p); >> + >> + ip->ip_tot_len = htons(p->l4_ofs - p->l3_ofs + l4_len); >> + ip->ip_csum = 0; >> + ip->ip_csum = csum(ip, sizeof *ip); >> + >> + pseudo_hdr_csum = packet_csum_pseudoheader(ip); >> + } else if (flow->dl_type == htons(ETH_TYPE_IPV6)) { >> + struct ovs_16aligned_ip6_hdr *nh = dp_packet_l3(p); >> + >> + nh->ip6_plen = htons(l4_len); >> + pseudo_hdr_csum = packet_csum_pseudoheader6(nh); >> + } >> + >> + if (dl_type_is_ip_any(flow->dl_type)) { >> + if ((!(flow->nw_frag & FLOW_NW_FRAG_ANY) >> + || !(flow->nw_frag & FLOW_NW_FRAG_LATER)) >> + && flow->nw_proto == IPPROTO_UDP) { >> + struct udp_header *udp = dp_packet_l4(p); >> + >> + udp->udp_len = htons(l4_len + extra_size); >> + } >> + flow_compose_l4_csum(p, flow, pseudo_hdr_csum); >> + } >> +} >> + >> /* Puts into 'p' a packet that flow_extract() would parse as having the given >> * 'flow'. >> * >> diff --git a/lib/flow.h b/lib/flow.h >> index 9297842..1bbbe41 100644 >> --- a/lib/flow.h >> +++ b/lib/flow.h >> @@ -125,6 +125,7 @@ void flow_set_mpls_bos(struct flow *, int idx, uint8_t stack); >> void flow_set_mpls_lse(struct flow *, int idx, ovs_be32 lse); >> >> void flow_compose(struct dp_packet *, const struct flow *); >> +void flow_compose_size(struct dp_packet *, const struct flow *, size_t size); >> >> bool parse_ipv6_ext_hdrs(const void **datap, size_t *sizep, uint8_t *nw_proto, >> uint8_t *nw_frag); >> diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c >> index 51d29d5..83e30b3 100644 >> --- a/lib/netdev-dummy.c >> +++ b/lib/netdev-dummy.c >> @@ -1449,7 +1449,7 @@ eth_from_packet(const char *s) >> } >> >> static struct dp_packet * >> -eth_from_flow(const char *s) >> +eth_from_flow(const char *s, size_t packet_size) >> { >> enum odp_key_fitness fitness; >> struct dp_packet *packet; >> @@ -1479,6 +1479,7 @@ eth_from_flow(const char *s) >> >> packet = dp_packet_new(0); >> flow_compose(packet, &flow); >> + flow_compose_size(packet, &flow, packet_size); >> >> ofpbuf_uninit(&odp_key); >> return packet; >> @@ -1556,20 +1557,26 @@ netdev_dummy_receive(struct unixctl_conn *conn, >> packet = eth_from_packet(argv[i]); >> >> if (!packet) { >> + int packet_size = 0; >> + const char *flow_str = argv[i]; >> + >> + /* Parse optional --len argument immediately follows a 'flow'. */ >> + if (argc >= i + 2 && !strcmp(argv[i + 1], "--len")) { >> + packet_size = strtol(argv[i + 2], NULL, 10); >> + >> + if (packet_size < ETH_TOTAL_MIN) { >> + unixctl_command_reply_error(conn, "too small packet len"); >> + goto exit; >> + } >> + i+=2; >> + } >> /* Try parse 'argv[i]' as odp flow. */ >> - packet = eth_from_flow(argv[i]); >> + packet = eth_from_flow(flow_str, packet_size); >> >> if (!packet) { >> unixctl_command_reply_error(conn, "bad packet or flow syntax"); >> goto exit; >> } >> - >> - /* Parse optional --len argument immediately follows a 'flow'. */ >> - if (argc >= i + 2 && !strcmp(argv[i + 1], "--len")) { >> - int packet_size = strtol(argv[i + 2], NULL, 10); >> - dp_packet_set_size(packet, packet_size); >> - i+=2; >> - } >> } >> >> netdev_dummy_queue_packet(dummy_dev, packet, rx_qid); >> --8<----------------------------------------------------------------------->8-- > > >
diff --git a/lib/flow.c b/lib/flow.c index e1597fa..ce99c06 100644 --- a/lib/flow.c +++ b/lib/flow.c @@ -2706,40 +2706,87 @@ flow_compose_l4_csum(struct dp_packet *p, const struct flow *flow, if (flow->nw_proto == IPPROTO_TCP) { struct tcp_header *tcp = dp_packet_l4(p); - /* Checksum has already been zeroed by put_zeros call in - * flow_compose_l4(). */ + tcp->tcp_csum = 0; tcp->tcp_csum = csum_finish(csum_continue(pseudo_hdr_csum, tcp, l4_len)); } else if (flow->nw_proto == IPPROTO_UDP) { struct udp_header *udp = dp_packet_l4(p); - /* Checksum has already been zeroed by put_zeros call in - * flow_compose_l4(). */ + udp->udp_csum = 0; udp->udp_csum = csum_finish(csum_continue(pseudo_hdr_csum, udp, l4_len)); } else if (flow->nw_proto == IPPROTO_ICMP) { struct icmp_header *icmp = dp_packet_l4(p); - /* Checksum has already been zeroed by put_zeros call in - * flow_compose_l4(). */ + icmp->icmp_csum = 0; icmp->icmp_csum = csum(icmp, l4_len); } else if (flow->nw_proto == IPPROTO_IGMP) { struct igmp_header *igmp = dp_packet_l4(p); - /* Checksum has already been zeroed by put_zeros call in - * flow_compose_l4(). */ + igmp->igmp_csum = 0; igmp->igmp_csum = csum(igmp, l4_len); } else if (flow->nw_proto == IPPROTO_ICMPV6) { struct icmp6_hdr *icmp = dp_packet_l4(p); - /* Checksum has already been zeroed by put_zeros call in - * flow_compose_l4(). */ + icmp->icmp6_cksum = 0; icmp->icmp6_cksum = (OVS_FORCE uint16_t) csum_finish(csum_continue(pseudo_hdr_csum, icmp, l4_len)); } } } +/* Tries to increase the size of packet composed by 'flow_compose' up to + * 'size' bytes. Fixes all the required packet headers like ip/udp lengths + * and l3/l4 checksums. */ +void +flow_compose_size(struct dp_packet *p, const struct flow *flow, size_t size) +{ + size_t extra_size, l4_len; + uint32_t pseudo_hdr_csum; + + if (size <= dp_packet_size(p)) { + return; + } + + extra_size = size - dp_packet_size(p); + dp_packet_put_zeros(p, extra_size); + + l4_len = (char *) dp_packet_tail(p) - (char *) dp_packet_l4(p); + + if (flow->dl_type == htons(FLOW_DL_TYPE_NONE)) { + struct eth_header *eth = dp_packet_eth(p); + + eth->eth_type = htons(dp_packet_size(p)); + return; + } + + if (flow->dl_type == htons(ETH_TYPE_IP)) { + struct ip_header *ip = dp_packet_l3(p); + + ip->ip_tot_len = htons(p->l4_ofs - p->l3_ofs + l4_len); + ip->ip_csum = 0; + ip->ip_csum = csum(ip, sizeof *ip); + + pseudo_hdr_csum = packet_csum_pseudoheader(ip); + } else if (flow->dl_type == htons(ETH_TYPE_IPV6)) { + struct ovs_16aligned_ip6_hdr *nh = dp_packet_l3(p); + + nh->ip6_plen = htons(l4_len); + pseudo_hdr_csum = packet_csum_pseudoheader6(nh); + } + + if (dl_type_is_ip_any(flow->dl_type)) { + if ((!(flow->nw_frag & FLOW_NW_FRAG_ANY) + || !(flow->nw_frag & FLOW_NW_FRAG_LATER)) + && flow->nw_proto == IPPROTO_UDP) { + struct udp_header *udp = dp_packet_l4(p); + + udp->udp_len = htons(l4_len + extra_size); + } + flow_compose_l4_csum(p, flow, pseudo_hdr_csum); + } +} + /* Puts into 'p' a packet that flow_extract() would parse as having the given * 'flow'. * diff --git a/lib/flow.h b/lib/flow.h index 9297842..1bbbe41 100644 --- a/lib/flow.h +++ b/lib/flow.h @@ -125,6 +125,7 @@ void flow_set_mpls_bos(struct flow *, int idx, uint8_t stack); void flow_set_mpls_lse(struct flow *, int idx, ovs_be32 lse); void flow_compose(struct dp_packet *, const struct flow *); +void flow_compose_size(struct dp_packet *, const struct flow *, size_t size); bool parse_ipv6_ext_hdrs(const void **datap, size_t *sizep, uint8_t *nw_proto, uint8_t *nw_frag); diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c index 51d29d5..83e30b3 100644 --- a/lib/netdev-dummy.c +++ b/lib/netdev-dummy.c @@ -1449,7 +1449,7 @@ eth_from_packet(const char *s) } static struct dp_packet * -eth_from_flow(const char *s) +eth_from_flow(const char *s, size_t packet_size) { enum odp_key_fitness fitness; struct dp_packet *packet; @@ -1479,6 +1479,7 @@ eth_from_flow(const char *s) packet = dp_packet_new(0); flow_compose(packet, &flow); + flow_compose_size(packet, &flow, packet_size); ofpbuf_uninit(&odp_key); return packet; @@ -1556,20 +1557,26 @@ netdev_dummy_receive(struct unixctl_conn *conn, packet = eth_from_packet(argv[i]); if (!packet) { + int packet_size = 0; + const char *flow_str = argv[i]; + + /* Parse optional --len argument immediately follows a 'flow'. */ + if (argc >= i + 2 && !strcmp(argv[i + 1], "--len")) { + packet_size = strtol(argv[i + 2], NULL, 10); + + if (packet_size < ETH_TOTAL_MIN) { + unixctl_command_reply_error(conn, "too small packet len"); + goto exit; + } + i+=2; + } /* Try parse 'argv[i]' as odp flow. */ - packet = eth_from_flow(argv[i]); + packet = eth_from_flow(flow_str, packet_size); if (!packet) { unixctl_command_reply_error(conn, "bad packet or flow syntax"); goto exit; } - - /* Parse optional --len argument immediately follows a 'flow'. */ - if (argc >= i + 2 && !strcmp(argv[i + 1], "--len")) { - int packet_size = strtol(argv[i + 2], NULL, 10); - dp_packet_set_size(packet, packet_size); - i+=2; - } } netdev_dummy_queue_packet(dummy_dev, packet, rx_qid);