[ovs-dev] [PATCH 1/3] flow: Add packet_size option to flow_compose.
Ilya Maximets
i.maximets at samsung.com
Tue Jul 25 13:01:27 UTC 2017
On 24.07.2017 22:40, Andy Zhou wrote:
> On Mon, Jul 24, 2017 at 6:33 AM, Ilya Maximets <i.maximets at 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 at 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 at 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--
>
>
>
More information about the dev
mailing list