Re: [ovs-dev] [PATCH v7 2/4] nsh: add new flow key 'ttl'

[Yang, Yi]

On Tue, Jan 09, 2018 at 08:11:15AM +0800, Ben Pfaff wrote:
> On Sat, Jan 06, 2018 at 01:47:52PM +0800, Yi Yang wrote:
> > IETF NSH draft added a new filed ttl in NSH header, this patch
> > is to add new nsh key 'ttl' for it.
> > 
> > Signed-off-by: Yi Yang 
> 
> Thanks for v7!
> 
> The field assignments in meta-flow.h seem wrong to me:
> 
> - The TTL field is new in v2.9, so it shouldn't say v2.8.
> 
> - The existing fields should not be renumbered because that
>   breaks OpenFlow wire format compatibility with anything that
>   already knows how to talk to OVS 2.8.  Please keep the
>   existing numbering.
> 
> Why does nsh_16aligned_be32 exist?  Please use get_16aligned_be32, which
> is identical.

I want to keep include/openvswitch/nsh.h consistent as possible as nsh.h in
kernel, it is not good to depend on lib/unaligned.h because it is only used
in lib/*, I have fixed sparse warnings.

> 
> In meta-flow.xml, please properly document the NSH fields, following the
> pattern set by the other documentation in the file.
> 
> I see several uses of memcpy() for copying a struct.  Please use an
> assignment to copy structs.
> 
> This statement looks suspicious, since the target and the "sizeof" are
> different:
> memset(nsh, 0, sizeof(nsh->context));
> 
> I'm concerned about how this patch introduces different structs with
> identical layouts and then uses memcpy() to copy between them.  This is
> a trap for unsuspecting developers who change one structure or the other
> (even by reordering fields).  It would probably be better to figure out
> a way to either use the same struct in each case, or to do
> member-by-member copies.  Another way would be to use assertions to make
> sure that the structures really are identical.

Your concerns make sense, I use assignments for them. v8 has been
posted, please review new ones. Thanks a lot.

> 
> Thanks,
> 
> Ben.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v7 2/4] nsh: add new flow key 'ttl'

[Ben Pfaff]

On Sat, Jan 06, 2018 at 01:47:52PM +0800, Yi Yang wrote:
> IETF NSH draft added a new filed ttl in NSH header, this patch
> is to add new nsh key 'ttl' for it.
> 
> Signed-off-by: Yi Yang 

Thanks for v7!

The field assignments in meta-flow.h seem wrong to me:

- The TTL field is new in v2.9, so it shouldn't say v2.8.

- The existing fields should not be renumbered because that
  breaks OpenFlow wire format compatibility with anything that
  already knows how to talk to OVS 2.8.  Please keep the
  existing numbering.

Why does nsh_16aligned_be32 exist?  Please use get_16aligned_be32, which
is identical.

In meta-flow.xml, please properly document the NSH fields, following the
pattern set by the other documentation in the file.

I see several uses of memcpy() for copying a struct.  Please use an
assignment to copy structs.

This statement looks suspicious, since the target and the "sizeof" are
different:
memset(nsh, 0, sizeof(nsh->context));

I'm concerned about how this patch introduces different structs with
identical layouts and then uses memcpy() to copy between them.  This is
a trap for unsuspecting developers who change one structure or the other
(even by reordering fields).  It would probably be better to figure out
a way to either use the same struct in each case, or to do
member-by-member copies.  Another way would be to use assertions to make
sure that the structures really are identical.

Thanks,

Ben.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v7 2/4] nsh: add new flow key 'ttl'

[Yi Yang]

IETF NSH draft added a new filed ttl in NSH header, this patch
is to add new nsh key 'ttl' for it.

Signed-off-by: Yi Yang 
---
 datapath/linux/compat/include/linux/openvswitch.h |   2 +-
 include/openvswitch/flow.h|   6 +-
 include/openvswitch/meta-flow.h   |  31 +++--
 include/openvswitch/nsh.h |  96 ++
 include/openvswitch/packets.h |  12 +-
 lib/flow.c|  23 ++--
 lib/flow.h|   2 +-
 lib/match.c   |  12 +-
 lib/meta-flow.c   |  56 ++--
 lib/meta-flow.xml |   6 +-
 lib/nx-match.c|  16 ++-
 lib/odp-execute.c |  40 +++---
 lib/odp-util.c| 151 --
 lib/odp-util.h|   2 +-
 lib/packets.c |   1 +
 ofproto/ofproto-dpif-xlate.c  |   7 +-
 tests/nsh.at  |  41 +++---
 17 files changed, 308 insertions(+), 196 deletions(-)

diff --git a/datapath/linux/compat/include/linux/openvswitch.h 
b/datapath/linux/compat/include/linux/openvswitch.h
index 3ddb1c5..c7142b6 100644
--- a/datapath/linux/compat/include/linux/openvswitch.h
+++ b/datapath/linux/compat/include/linux/openvswitch.h
@@ -504,9 +504,9 @@ enum ovs_nsh_key_attr {
 
 struct ovs_nsh_key_base {
__u8 flags;
+   __u8 ttl;
__u8 mdtype;
__u8 np;
-   __u8 pad;
__be32 path_hdr;
 };
 
diff --git a/include/openvswitch/flow.h b/include/openvswitch/flow.h
index a658a58..cd61fff 100644
--- a/include/openvswitch/flow.h
+++ b/include/openvswitch/flow.h
@@ -146,7 +146,7 @@ struct flow {
 struct eth_addr arp_tha;/* ARP/ND target hardware address. */
 ovs_be16 tcp_flags; /* TCP flags. With L3 to avoid matching L4. */
 ovs_be16 pad2;  /* Pad to 64 bits. */
-struct flow_nsh nsh;/* Network Service Header keys */
+struct ovs_key_nsh nsh; /* Network Service Header keys */
 
 /* L4 (64-bit aligned) */
 ovs_be16 tp_src;/* TCP/UDP/SCTP source port/ICMP type. */
@@ -159,13 +159,13 @@ struct flow {
 };
 BUILD_ASSERT_DECL(sizeof(struct flow) % sizeof(uint64_t) == 0);
 BUILD_ASSERT_DECL(sizeof(struct flow_tnl) % sizeof(uint64_t) == 0);
-BUILD_ASSERT_DECL(sizeof(struct flow_nsh) % sizeof(uint64_t) == 0);
+BUILD_ASSERT_DECL(sizeof(struct ovs_key_nsh) % sizeof(uint64_t) == 0);
 
 #define FLOW_U64S (sizeof(struct flow) / sizeof(uint64_t))
 
 /* Remember to update FLOW_WC_SEQ when changing 'struct flow'. */
 BUILD_ASSERT_DECL(offsetof(struct flow, igmp_group_ip4) + sizeof(uint32_t)
-  == sizeof(struct flow_tnl) + sizeof(struct flow_nsh) + 300
+  == sizeof(struct flow_tnl) + sizeof(struct ovs_key_nsh) + 300
   && FLOW_WC_SEQ == 40);
 
 /* Incremental points at which flow classification may be performed in
diff --git a/include/openvswitch/meta-flow.h b/include/openvswitch/meta-flow.h
index 436501f..14e6b59 100644
--- a/include/openvswitch/meta-flow.h
+++ b/include/openvswitch/meta-flow.h
@@ -1757,6 +1757,21 @@ enum OVS_PACKED_ENUM mf_field_id {
  */
 MFF_NSH_FLAGS,
 
+/* "nsh_ttl".
+ *
+ * TTL field in NSH base header.
+ *
+ * Type: u8.
+ * Maskable: no.
+ * Formatting: decimal.
+ * Prerequisites: NSH.
+ * Access: read/write.
+ * NXM: none.
+ * OXM: NXOXM_NSH_TTL(2) since OF1.3 and v2.8.
+ */
+MFF_NSH_TTL,
+
+
 /* "nsh_mdtype".
  *
  * mdtype field in NSH base header.
@@ -1767,7 +1782,7 @@ enum OVS_PACKED_ENUM mf_field_id {
  * Prerequisites: NSH.
  * Access: read-only.
  * NXM: none.
- * OXM: NXOXM_NSH_MDTYPE(2) since OF1.3 and v2.8.
+ * OXM: NXOXM_NSH_MDTYPE(3) since OF1.3 and v2.8.
  */
 MFF_NSH_MDTYPE,
 
@@ -1781,7 +1796,7 @@ enum OVS_PACKED_ENUM mf_field_id {
  * Prerequisites: NSH.
  * Access: read-only.
  * NXM: none.
- * OXM: NXOXM_NSH_NP(3) since OF1.3 and v2.8.
+ * OXM: NXOXM_NSH_NP(4) since OF1.3 and v2.8.
  */
 MFF_NSH_NP,
 
@@ -1795,7 +1810,7 @@ enum OVS_PACKED_ENUM mf_field_id {
  * Prerequisites: NSH.
  * Access: read/write.
  * NXM: none.
- * OXM: NXOXM_NSH_SPI(4) since OF1.3 and v2.8.
+ * OXM: NXOXM_NSH_SPI(5) since OF1.3 and v2.8.
  */
 MFF_NSH_SPI,
 
@@ -1809,7 +1824,7 @@ enum OVS_PACKED_ENUM mf_field_id {
  * Prerequisites: NSH.
  * Access: read/write.
  * NXM: none.
- * OXM: NXOXM_NSH_SI(5) since OF1.3 and v2.8.
+ * OXM: NXOXM_NSH_SI(6) since OF1.3 and v2.8.
  */
 MFF_NSH_SI,
 
@@ -1823,10 +1838,10 @@ enum OVS_PACKED_ENUM mf_field_id {
  * Prerequisites: NSH.
  * Access: read/write.
  * NXM: none.
-