[ovs-dev,4/5] flow: Add FLOW_WC_SEQ assertions and improve comments.
diff mbox series

Message ID 20190408182206.15688-4-blp@ovn.org
State Accepted
Headers show
Series
  • [ovs-dev,1/5] compiler: Disable BUILD_MESSAGE() when processing with sparse.
Related show

Commit Message

Ben Pfaff April 8, 2019, 6:22 p.m. UTC
The assertions make it easier to find all the places that need to be
updated when adding protocol support.

Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 lib/flow.c     | 64 ++++++++++++++++++++++++++++----------------------
 lib/odp-util.c | 15 ++++++++++--
 2 files changed, 49 insertions(+), 30 deletions(-)

Comments

Numan Siddique April 10, 2019, 6:23 a.m. UTC | #1
On Mon, Apr 8, 2019 at 11:54 PM Ben Pfaff <blp@ovn.org> wrote:

> The assertions make it easier to find all the places that need to be
> updated when adding protocol support.
>
> Signed-off-by: Ben Pfaff <blp@ovn.org>
>

Acked-by: Numan Siddique <nusiddiq@redhat.com>


> ---
>  lib/flow.c     | 64 ++++++++++++++++++++++++++++----------------------
>  lib/odp-util.c | 15 ++++++++++--
>  2 files changed, 49 insertions(+), 30 deletions(-)
>
> diff --git a/lib/flow.c b/lib/flow.c
> index 7511b1877ca4..f39b57f5b9d9 100644
> --- a/lib/flow.c
> +++ b/lib/flow.c
> @@ -626,32 +626,8 @@ parse_nsh(const void **datap, size_t *sizep, struct
> ovs_key_nsh *key)
>      return true;
>  }
>
> -/* Initializes 'flow' members from 'packet' and 'md', taking the packet
> type
> - * into account.
> - *
> - * Initializes the layer offsets as follows:
> - *
> - *    - packet->l2_5_ofs to the
> - *          * the start of the MPLS shim header. Can be zero, if the
> - *            packet is of type (OFPHTN_ETHERTYPE, ETH_TYPE_MPLS).
> - *          * UINT16_MAX when there is no MPLS shim header.
> - *
> - *    - packet->l3_ofs is set to
> - *          * zero if the packet_type is in name space OFPHTN_ETHERTYPE
> - *            and there is no MPLS shim header.
> - *          * just past the Ethernet header, or just past the vlan_header
> if
> - *            one is present, to the first byte of the payload of the
> - *            Ethernet frame if the packet type is Ethernet and there is
> - *            no MPLS shim header.
> - *          * just past the MPLS label stack to the first byte of the MPLS
> - *            payload if there is at least one MPLS shim header.
> - *          * UINT16_MAX if the packet type is Ethernet and the frame is
> - *            too short to contain an Ethernet header.
> - *
> - *    - packet->l4_ofs is set to just past the IPv4 or IPv6 header, if
> one is
> - *      present and the packet has at least the content used for the
> fields
> - *      of interest for the flow, otherwise UINT16_MAX.
> - */
> +/* This does the same thing as miniflow_extract() with a full-size 'flow'
> as
> + * the destination. */
>  void
>  flow_extract(struct dp_packet *packet, struct flow *flow)
>  {
> @@ -730,11 +706,38 @@ ipv6_sanity_check(const struct ovs_16aligned_ip6_hdr
> *nh, size_t size)
>      return true;
>  }
>
> -/* Caller is responsible for initializing 'dst' with enough storage for
> - * FLOW_U64S * 8 bytes. */
> +/* Initializes 'dst' from 'packet' and 'md', taking the packet type into
> + * account.  'dst' must have enough space for FLOW_U64S * 8 bytes.
> + *
> + * Initializes the layer offsets as follows:
> + *
> + *    - packet->l2_5_ofs to the
> + *          * the start of the MPLS shim header. Can be zero, if the
> + *            packet is of type (OFPHTN_ETHERTYPE, ETH_TYPE_MPLS).
> + *          * UINT16_MAX when there is no MPLS shim header.
> + *
> + *    - packet->l3_ofs is set to
> + *          * zero if the packet_type is in name space OFPHTN_ETHERTYPE
> + *            and there is no MPLS shim header.
> + *          * just past the Ethernet header, or just past the vlan_header
> if
> + *            one is present, to the first byte of the payload of the
> + *            Ethernet frame if the packet type is Ethernet and there is
> + *            no MPLS shim header.
> + *          * just past the MPLS label stack to the first byte of the MPLS
> + *            payload if there is at least one MPLS shim header.
> + *          * UINT16_MAX if the packet type is Ethernet and the frame is
> + *            too short to contain an Ethernet header.
> + *
> + *    - packet->l4_ofs is set to just past the IPv4 or IPv6 header, if
> one is
> + *      present and the packet has at least the content used for the
> fields
> + *      of interest for the flow, otherwise UINT16_MAX.
> + */
>  void
>  miniflow_extract(struct dp_packet *packet, struct miniflow *dst)
>  {
> +    /* Add code to this function (or its callees) to extract new fields.
> */
> +    BUILD_ASSERT_DECL(FLOW_WC_SEQ == 41);
> +
>      const struct pkt_metadata *md = &packet->md;
>      const void *data = dp_packet_data(packet);
>      size_t size = dp_packet_size(packet);
> @@ -3182,6 +3185,11 @@ void
>  flow_compose(struct dp_packet *p, const struct flow *flow,
>               const void *l7, size_t l7_len)
>  {
> +    /* Add code to this function (or its callees) for emitting new fields
> or
> +     * protocols.  (This isn't essential, so it can be skipped for initial
> +     * testing.) */
> +    BUILD_ASSERT_DECL(FLOW_WC_SEQ == 41);
> +
>      uint32_t pseudo_hdr_csum;
>      size_t l4_len;
>
> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index b6552c5c208a..6e545b80ba6c 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> @@ -8230,14 +8230,25 @@ commit_encap_decap_action(const struct flow *flow,
>   * in addition to this function if needed.  Sets fields in 'wc' that are
>   * used as part of the action.
>   *
> - * Returns a reason to force processing the flow's packets into the
> userspace
> - * slow path, if there is one, otherwise 0. */
> + * In the common case, this function returns 0.  If the flow key
> modification
> + * requires the flow's packets to be forced into the userspace slow path,
> this
> + * function returns SLOW_ACTION.  This only happens when there is no ODP
> action
> + * to modify some field that was actually modified.  For example, there
> is no
> + * ODP action to modify any ARP field, so such a modification triggers
> + * SLOW_ACTION.  (When this happens, packets that need such modification
> get
> + * flushed to userspace and handled there, which works OK but much more
> slowly
> + * than if the datapath handled it directly.) */
>  enum slow_path_reason
>  commit_odp_actions(const struct flow *flow, struct flow *base,
>                     struct ofpbuf *odp_actions, struct flow_wildcards *wc,
>                     bool use_masked, bool pending_encap, bool
> pending_decap,
>                     struct ofpbuf *encap_data)
>  {
> +    /* If you add a field that OpenFlow actions can change, and that is
> visible
> +     * to the datapath (including all data fields), then you should also
> add
> +     * code here to commit changes to the field. */
> +    BUILD_ASSERT_DECL(FLOW_WC_SEQ == 41);
> +
>      enum slow_path_reason slow1, slow2;
>      bool mpls_done = false;
>
> --
> 2.20.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

Patch
diff mbox series

diff --git a/lib/flow.c b/lib/flow.c
index 7511b1877ca4..f39b57f5b9d9 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -626,32 +626,8 @@  parse_nsh(const void **datap, size_t *sizep, struct ovs_key_nsh *key)
     return true;
 }
 
-/* Initializes 'flow' members from 'packet' and 'md', taking the packet type
- * into account.
- *
- * Initializes the layer offsets as follows:
- *
- *    - packet->l2_5_ofs to the
- *          * the start of the MPLS shim header. Can be zero, if the
- *            packet is of type (OFPHTN_ETHERTYPE, ETH_TYPE_MPLS).
- *          * UINT16_MAX when there is no MPLS shim header.
- *
- *    - packet->l3_ofs is set to
- *          * zero if the packet_type is in name space OFPHTN_ETHERTYPE
- *            and there is no MPLS shim header.
- *          * just past the Ethernet header, or just past the vlan_header if
- *            one is present, to the first byte of the payload of the
- *            Ethernet frame if the packet type is Ethernet and there is
- *            no MPLS shim header.
- *          * just past the MPLS label stack to the first byte of the MPLS
- *            payload if there is at least one MPLS shim header.
- *          * UINT16_MAX if the packet type is Ethernet and the frame is
- *            too short to contain an Ethernet header.
- *
- *    - packet->l4_ofs is set to just past the IPv4 or IPv6 header, if one is
- *      present and the packet has at least the content used for the fields
- *      of interest for the flow, otherwise UINT16_MAX.
- */
+/* This does the same thing as miniflow_extract() with a full-size 'flow' as
+ * the destination. */
 void
 flow_extract(struct dp_packet *packet, struct flow *flow)
 {
@@ -730,11 +706,38 @@  ipv6_sanity_check(const struct ovs_16aligned_ip6_hdr *nh, size_t size)
     return true;
 }
 
-/* Caller is responsible for initializing 'dst' with enough storage for
- * FLOW_U64S * 8 bytes. */
+/* Initializes 'dst' from 'packet' and 'md', taking the packet type into
+ * account.  'dst' must have enough space for FLOW_U64S * 8 bytes.
+ *
+ * Initializes the layer offsets as follows:
+ *
+ *    - packet->l2_5_ofs to the
+ *          * the start of the MPLS shim header. Can be zero, if the
+ *            packet is of type (OFPHTN_ETHERTYPE, ETH_TYPE_MPLS).
+ *          * UINT16_MAX when there is no MPLS shim header.
+ *
+ *    - packet->l3_ofs is set to
+ *          * zero if the packet_type is in name space OFPHTN_ETHERTYPE
+ *            and there is no MPLS shim header.
+ *          * just past the Ethernet header, or just past the vlan_header if
+ *            one is present, to the first byte of the payload of the
+ *            Ethernet frame if the packet type is Ethernet and there is
+ *            no MPLS shim header.
+ *          * just past the MPLS label stack to the first byte of the MPLS
+ *            payload if there is at least one MPLS shim header.
+ *          * UINT16_MAX if the packet type is Ethernet and the frame is
+ *            too short to contain an Ethernet header.
+ *
+ *    - packet->l4_ofs is set to just past the IPv4 or IPv6 header, if one is
+ *      present and the packet has at least the content used for the fields
+ *      of interest for the flow, otherwise UINT16_MAX.
+ */
 void
 miniflow_extract(struct dp_packet *packet, struct miniflow *dst)
 {
+    /* Add code to this function (or its callees) to extract new fields. */
+    BUILD_ASSERT_DECL(FLOW_WC_SEQ == 41);
+
     const struct pkt_metadata *md = &packet->md;
     const void *data = dp_packet_data(packet);
     size_t size = dp_packet_size(packet);
@@ -3182,6 +3185,11 @@  void
 flow_compose(struct dp_packet *p, const struct flow *flow,
              const void *l7, size_t l7_len)
 {
+    /* Add code to this function (or its callees) for emitting new fields or
+     * protocols.  (This isn't essential, so it can be skipped for initial
+     * testing.) */
+    BUILD_ASSERT_DECL(FLOW_WC_SEQ == 41);
+
     uint32_t pseudo_hdr_csum;
     size_t l4_len;
 
diff --git a/lib/odp-util.c b/lib/odp-util.c
index b6552c5c208a..6e545b80ba6c 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -8230,14 +8230,25 @@  commit_encap_decap_action(const struct flow *flow,
  * in addition to this function if needed.  Sets fields in 'wc' that are
  * used as part of the action.
  *
- * Returns a reason to force processing the flow's packets into the userspace
- * slow path, if there is one, otherwise 0. */
+ * In the common case, this function returns 0.  If the flow key modification
+ * requires the flow's packets to be forced into the userspace slow path, this
+ * function returns SLOW_ACTION.  This only happens when there is no ODP action
+ * to modify some field that was actually modified.  For example, there is no
+ * ODP action to modify any ARP field, so such a modification triggers
+ * SLOW_ACTION.  (When this happens, packets that need such modification get
+ * flushed to userspace and handled there, which works OK but much more slowly
+ * than if the datapath handled it directly.) */
 enum slow_path_reason
 commit_odp_actions(const struct flow *flow, struct flow *base,
                    struct ofpbuf *odp_actions, struct flow_wildcards *wc,
                    bool use_masked, bool pending_encap, bool pending_decap,
                    struct ofpbuf *encap_data)
 {
+    /* If you add a field that OpenFlow actions can change, and that is visible
+     * to the datapath (including all data fields), then you should also add
+     * code here to commit changes to the field. */
+    BUILD_ASSERT_DECL(FLOW_WC_SEQ == 41);
+
     enum slow_path_reason slow1, slow2;
     bool mpls_done = false;