[ovs-dev,v2,00/12] Packet type aware pipeline

Hi, guys

I'm working on NSH patches for kernel path, I think we can change it with new NSH kernel patches together. You ovs guys have confirmed several times userspace part can be merged before kernel part isn't there. So I propose we review and merge userspace part first, kernel part will be posted in next series, does it make sense?

-----Original Message-----
From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-bounces@openvswitch.org] On Behalf Of Zoltán Balogh
Sent: Monday, June 19, 2017 6:27 PM
To: Ben Pfaff <blp@ovn.org>; dev@openvswitch.org
Subject: Re: [ovs-dev] [PATCH v2 00/12] Packet type aware pipeline

> 
> A new concern came up while thinking about this series.  The 
> OVS_ATTR_PACKET_TYPE does not appear to be implemented in the kernel 
> module, and what's more, because of #ifdefs, OVS_ATTR_PACKET_TYPE will 
> actually have a different value in the kernel module than in userspace.
> What's the plan here?

Hi Ben, 

OVS_KEY_ATTR_PACKET_TYPE is represented with OVS_KEY_ATTR_ETHERNET and OVS_KEY_ATTR_ETHERTYPE in the kernel.
From Google doc:
"The presence of netlink key attribute OVS_KEY_ATTR_ETHERNET is used to indicate if it's about L2 or L3 packet on the netlink interface. The "L3" packet type is encoded in the OVS_KEY_ATTR_ETHERTYPE netlink attribute."

The plan was to do a conversion between OVS_KEY_ATTR_PACKET_TYPE and the pair of OVS_KEY_ATTR_ETHERNET + OVS_KEY_ATTR_ETHERTYPE attributes before/after transmission over netlink. Currently the PACKET_TYPE attribute is filtered out in the put_exclude_packet_type() function in lib/dpif-netlink.c.

put_exclude_packet_type(struct ofpbuf *buf, uint16_t type,
                        const struct nlattr *data, uint16_t data_len) {
    const struct nlattr *packet_type;

    packet_type = nl_attr_find__(data, data_len, OVS_KEY_ATTR_PACKET_TYPE);

    if (packet_type) {
        /* exclude PACKET_TYPE Netlink attribute. */
        ovs_assert(NLA_ALIGN(packet_type->nla_len) == NL_A_U32_SIZE);
        size_t packet_type_len = NL_A_U32_SIZE;
        size_t first_chunk_size = (uint8_t *)packet_type - (uint8_t *)data;
        size_t second_chunk_size = data_len - first_chunk_size
                                   - packet_type_len;
        uint8_t *first_attr = NULL;
        struct nlattr *next_attr = nl_attr_next(packet_type);

        first_attr = nl_msg_put_unspec_uninit(buf, type,
                                              data_len - packet_type_len);
        memcpy(first_attr, data, first_chunk_size);
        memcpy(first_attr + first_chunk_size, next_attr, second_chunk_size);
    } else {
        nl_msg_put_unspec(buf, type, data, data_len);
    }
}

This could be modified to do more verification and put OVS_KEY_ATTR_ETHERTYPE if needed:

---

     packet_type = nl_attr_find__(data, data_len, OVS_KEY_ATTR_PACKET_TYPE);
 
     if (packet_type) {
-        /* exclude PACKET_TYPE Netlink attribute. */
+        /* convert PACKET_TYPE Netlink attribute. */
         ovs_assert(NLA_ALIGN(packet_type->nla_len) == NL_A_U32_SIZE);
+        ovs_be32 value = nl_attr_get_be32(packet_type);
         size_t packet_type_len = NL_A_U32_SIZE;
         size_t first_chunk_size = (uint8_t *)packet_type - (uint8_t *)data;
         size_t second_chunk_size = data_len - first_chunk_size @@ -3455,10 +3456,31 @@ put_exclude_packet_type(struct ofpbuf *buf, uint16_t type,
         uint8_t *first_attr = NULL;
         struct nlattr *next_attr = nl_attr_next(packet_type);
 
+        bool ethernet_present = nl_attr_find__(data, data_len,
+                                               OVS_KEY_ATTR_ETHERNET);
+
         first_attr = nl_msg_put_unspec_uninit(buf, type,
                                               data_len - packet_type_len);
         memcpy(first_attr, data, first_chunk_size);
         memcpy(first_attr + first_chunk_size, next_attr, second_chunk_size);
+
+        /* Put OVS_KEY_ATTR_ETHERTYPE if needed. */
+        if (ntohl(value) == PT_ETH) {
+            ovs_assert(ethernet_present);
+        } else {
+            const struct nlattr *eth_type;
+            ovs_be16 ns_type = pt_ns_type_be(value);
+
+            ovs_assert(ethernet_present == false);
+
+            eth_type = nl_attr_find__(data, data_len, OVS_KEY_ATTR_ETHERTYPE);
+            if (eth_type) {
+                ovs_assert(nl_attr_get_be16(eth_type) == ns_type);
+            } else {
+                nl_msg_put_be16(buf, OVS_KEY_ATTR_ETHERTYPE, ns_type);
+            }
+        }
+
     } else {
         nl_msg_put_unspec(buf, type, data, data_len);
     }
@@ -3489,11 +3511,11 @@ dpif_netlink_flow_to_ofpbuf(const struct dpif_netlink_flow *flow,
     }
     if (!flow->ufid_terse || !flow->ufid_present) {
         if (flow->key_len) {
-            put_exclude_packet_type(buf, OVS_FLOW_ATTR_KEY, flow->key,
+            put_convert_packet_type(buf, OVS_FLOW_ATTR_KEY, flow->key,
                                            flow->key_len);
         }
         if (flow->mask_len) {
-            put_exclude_packet_type(buf, OVS_FLOW_ATTR_MASK, flow->mask,
+            put_convert_packet_type(buf, OVS_FLOW_ATTR_MASK, 
+ flow->mask,
                                            flow->mask_len);
         }
         if (flow->actions || flow->actions_len) {

---

On the Rx side, upcall->packet.packet_type is set in the parse_odp_packet() called by dpif_netlink_recv__().
I'm not sure, if this is enough. Could someone double check, please?

Best regards,
Zoltan