[ovs-dev] flow: Refactor flow_compose() API.

Ilya Maximets i.maximets at samsung.com
Thu Jul 27 09:09:50 UTC 2017


On 26.07.2017 00:44, andy zhou wrote:
> Currently, flow_compose_size() is only supposed to be called after
> flow_compose(). I find this API to be unintuitive.
> 
> Change flow_compose() API to take the 'size' argument, and
> returns 'true' if the packet can be created, 'false' otherwise.
> 
> This change also improves error detection and reporting when
> 'size' is unreasonably small.
> 
> Signed-off-by: Andy Zhou <azhou at ovn.org>
> ---

Thanks for the refactoring. I wanted to combine flow_compose_size()
and flow_compose() somehow but didn't thought about the wrapper.

Looks good to me in general. One comment inline.

>  lib/flow.c                   | 53 ++++++++++++++++++++++++++++++++++----------
>  lib/flow.h                   |  3 +--
>  lib/netdev-dummy.c           |  7 ++++--
>  ofproto/ofproto-dpif-trace.c |  2 +-
>  ofproto/ofproto-dpif.c       |  2 +-
>  ovn/controller/ofctrl.c      |  2 +-
>  tests/test-ovn.c             |  2 +-
>  7 files changed, 51 insertions(+), 20 deletions(-)
> 
> diff --git a/lib/flow.c b/lib/flow.c
> index 8da9f3235d0a..39aed51837a6 100644
> --- a/lib/flow.c
> +++ b/lib/flow.c
> @@ -2735,17 +2735,17 @@ flow_compose_l4_csum(struct dp_packet *p, const struct flow *flow,
>      }
>  }
>  
> -/* 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)
> +/* Increase the size of packet composed by 'flow_compose_minimal'
> + * up to 'size' bytes.  Fixes all the required packet headers like
> + * ip/udp lengths and l3/l4 checksums.
> + *
> + * 'size' needs to be larger then the current packet size.  */
> +static void
> +packet_expand(struct dp_packet *p, const struct flow *flow, size_t size)
>  {
>      size_t extra_size;
>  
> -    if (size <= dp_packet_size(p)) {
> -        return;
> -    }
> +    ovs_assert(size > dp_packet_size(p));
>  
>      extra_size = size - dp_packet_size(p);
>      dp_packet_put_zeros(p, extra_size);
> @@ -2754,7 +2754,6 @@ flow_compose_size(struct dp_packet *p, const struct flow *flow, size_t size)
>          struct eth_header *eth = dp_packet_eth(p);
>  
>          eth->eth_type = htons(dp_packet_size(p));
> -
>      } else if (dl_type_is_ip_any(flow->dl_type)) {
>          uint32_t pseudo_hdr_csum;
>          size_t l4_len = (char *) dp_packet_tail(p) - (char *) dp_packet_l4(p);
> @@ -2789,9 +2788,12 @@ flow_compose_size(struct dp_packet *p, const struct flow *flow, size_t size)
>   * 'flow'.
>   *
>   * (This is useful only for testing, obviously, and the packet isn't really
> - * valid.  Lots of fields are just zeroed.) */
> -void
> -flow_compose(struct dp_packet *p, const struct flow *flow)
> + * valid.  Lots of fields are just zeroed.)
> + *
> + * The created packet will have minimal packet length, just large
> + * enough to hold the packet header fields.  */
> +static void
> +flow_compose_minimal(struct dp_packet *p, const struct flow *flow)
>  {
>      uint32_t pseudo_hdr_csum;
>      size_t l4_len;
> @@ -2896,6 +2898,33 @@ flow_compose(struct dp_packet *p, const struct flow *flow)
>          }
>      }
>  }
> +
> +/* Puts into 'p' a Ethernet frame of size 'size' that flow_extract() would
> + * parse as having the given 'flow'.
> + *
> + * When 'size' is zero, 'p' is a minimal size packet that only big enough
> + * to contains all packet headers.
> + *
> + * When 'size' is larger than the minimal packet size, the packet will
> + * be expended to 'size' with the payload set to zero.
> + *
> + * Return 'true' if the packet is successfully created. 'false' otherwise.
> + * Note, when 'size' is set to zero, this function always returns true.  */
> +bool
> +flow_compose(struct dp_packet *p, const struct flow *flow, size_t size)
> +{
> +    flow_compose_minimal(p, flow);
> +
> +    if (size && size < dp_packet_size(p)) {
> +        return false;
> +    }
> +
> +    if (size > dp_packet_size(p)) {
> +        packet_expand(p, flow, size);
> +    }
> +
> +    return true;
> +}
>  
>  /* Compressed flow. */
>  
> diff --git a/lib/flow.h b/lib/flow.h
> index 1bbbe410c6d2..0c6069c66c74 100644
> --- a/lib/flow.h
> +++ b/lib/flow.h
> @@ -124,8 +124,7 @@ void flow_set_mpls_tc(struct flow *, int idx, uint8_t tc);
>  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 flow_compose(struct dp_packet *, const struct flow *, size_t);
>  
>  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 83e30b37cbc3..6e110b1c7376 100644
> --- a/lib/netdev-dummy.c
> +++ b/lib/netdev-dummy.c
> @@ -1478,8 +1478,11 @@ eth_from_flow(const char *s, size_t packet_size)
>      }
>  
>      packet = dp_packet_new(0);
> -    flow_compose(packet, &flow);
> -    flow_compose_size(packet, &flow, packet_size);
> +    if (!flow_compose(packet, &flow, packet_size)) {
> +        dp_packet_uninit(packet);
> +        free(packet);

I think, it's better to use 'dp_packet_delete(packet)' instead of two
lines above. 'delete' is the better pair for 'new'. 'uninit' is mostly
for packets allocated statically and initialized by 'dp_packet_init()'.

Additionally this will help to avoid issues if 'dp_packet_new' will
be able to allocate DPDK memory someday.

With this change:
Acked-by: Ilya Maximets <i.maximets at samsung.com>

> +        packet = NULL;
> +    };
>  
>      ofpbuf_uninit(&odp_key);
>      return packet;
> diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c
> index b3f3cbc6a4ef..38d11002f290 100644
> --- a/ofproto/ofproto-dpif-trace.c
> +++ b/ofproto/ofproto-dpif-trace.c
> @@ -376,7 +376,7 @@ parse_flow_and_packet(int argc, const char *argv[],
>      /* Generate a packet, if requested. */
>      if (packet) {
>          if (!dp_packet_size(packet)) {
> -            flow_compose(packet, flow);
> +            flow_compose(packet, flow, 0);
>          } else {
>              /* Use the metadata from the flow and the packet argument
>               * to reconstruct the flow. */
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 50f440fd8964..823e506f1f7c 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -1287,7 +1287,7 @@ check_ct_eventmask(struct dpif_backer *backer)
>  
>      /* Compose a dummy UDP packet. */
>      dp_packet_init(&packet, 0);
> -    flow_compose(&packet, &flow);
> +    flow_compose(&packet, &flow, 0);
>  
>      /* Execute the actions.  On older datapaths this fails with EINVAL, on
>       * newer datapaths it succeeds. */
> diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
> index 5aff2302a346..7164ff061b64 100644
> --- a/ovn/controller/ofctrl.c
> +++ b/ovn/controller/ofctrl.c
> @@ -1150,7 +1150,7 @@ ofctrl_inject_pkt(const struct ovsrec_bridge *br_int, const char *flow_s,
>      uint64_t packet_stub[128 / 8];
>      struct dp_packet packet;
>      dp_packet_use_stub(&packet, packet_stub, sizeof packet_stub);
> -    flow_compose(&packet, &uflow);
> +    flow_compose(&packet, &uflow, 0);
>  
>      uint64_t ofpacts_stub[1024 / 8];
>      struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(ofpacts_stub);
> diff --git a/tests/test-ovn.c b/tests/test-ovn.c
> index ca27a0f5a1a2..a216b820a02c 100644
> --- a/tests/test-ovn.c
> +++ b/tests/test-ovn.c
> @@ -1164,7 +1164,7 @@ test_expr_to_packets(struct ovs_cmdl_context *ctx OVS_UNUSED)
>          uint64_t packet_stub[128 / 8];
>          struct dp_packet packet;
>          dp_packet_use_stub(&packet, packet_stub, sizeof packet_stub);
> -        flow_compose(&packet, &uflow);
> +        flow_compose(&packet, &uflow, 0);
>  
>          struct ds output = DS_EMPTY_INITIALIZER;
>          const uint8_t *buf = dp_packet_data(&packet);
> 


More information about the dev mailing list