[ovs-dev,v5,2/2] ofproto-dpif-ipfix: add interface Information Elements to flow key

Message ID 1506955769-115602-2-git-send-email-michalx.weglicki@intel.com
State New
Headers show
Series
  • [ovs-dev,v5,1/2] netdev-dpdk: extend netdev_dpdk_get_status to include if_type and if_descr
Related show

Commit Message

Weglicki, MichalX Oct. 2, 2017, 2:49 p.m.
Extend flow key part of data record to include following Information Elements:
- ingressInterface
- ingressInterfaceType
- egressInterface
- egressInterfaceType
- interfaceName
- interfaceDescription

In case of input sampling we don't have information about egress port.
Define templates depending not only on protocol types, but also on flow
direction. Only egress flow will include egress information elements.

With this change, dpif_ipfix_exporter stores every port in hmap rather
than only tunnel ports. It allows to easily retrieve required
information about interfaces during sampling upcalls.

v1->v2
* Add interfaceType and interfaceDescription
* Rework ipfix_get_iface_data_record function
v2->v3: Code rebase.
v3->v4: Minor comments applied.
v4->v5: Clang complation problem fix.

Co-authored-by: Michal Weglicki <michalx.weglicki@intel.com>
Signed-off-by: Michal Weglicki <michalx.weglicki@intel.com>
Signed-off-by: Przemyslaw Szczerbik <przemyslawx.szczerbik@intel.com>
---
 ofproto/ofproto-dpif-ipfix.c | 358 +++++++++++++++++++++++++++++++------------
 ofproto/ofproto-dpif-ipfix.h |   6 +-
 ofproto/ofproto-dpif-xlate.c |   4 +-
 ofproto/ofproto-dpif.c       |  19 +--
 4 files changed, 277 insertions(+), 110 deletions(-)

Comments

Greg Rose Oct. 3, 2017, 10:21 p.m. | #1
On 10/02/2017 07:49 AM, Michal Weglicki wrote:
> Extend flow key part of data record to include following Information Elements:
> - ingressInterface
> - ingressInterfaceType
> - egressInterface
> - egressInterfaceType
> - interfaceName
> - interfaceDescription
> 
> In case of input sampling we don't have information about egress port.
> Define templates depending not only on protocol types, but also on flow
> direction. Only egress flow will include egress information elements.
> 
> With this change, dpif_ipfix_exporter stores every port in hmap rather
> than only tunnel ports. It allows to easily retrieve required
> information about interfaces during sampling upcalls.
> 
> v1->v2
> * Add interfaceType and interfaceDescription
> * Rework ipfix_get_iface_data_record function
> v2->v3: Code rebase.
> v3->v4: Minor comments applied.
> v4->v5: Clang complation problem fix.
> 
> Co-authored-by: Michal Weglicki <michalx.weglicki@intel.com>
> Signed-off-by: Michal Weglicki <michalx.weglicki@intel.com>
> Signed-off-by: Przemyslaw Szczerbik <przemyslawx.szczerbik@intel.com>

Michal and Przemyslaw,

There is one more small nit pick that I came across noted below.

Other than that....

Tested-by: Greg Rose <gvrose8192@gmail.com>
Reviewed-by: Greg Rose <gvrose8192@gmail.com>

> ---
>   ofproto/ofproto-dpif-ipfix.c | 358 +++++++++++++++++++++++++++++++------------
>   ofproto/ofproto-dpif-ipfix.h |   6 +-
>   ofproto/ofproto-dpif-xlate.c |   4 +-
>   ofproto/ofproto-dpif.c       |  19 +--
>   4 files changed, 277 insertions(+), 110 deletions(-)
> 
> diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
> index 472c272..138c325 100644
> --- a/ofproto/ofproto-dpif-ipfix.c
> +++ b/ofproto/ofproto-dpif-ipfix.c
> @@ -115,11 +115,12 @@ struct dpif_ipfix_global_stats {
>   };
>   
>   struct dpif_ipfix_port {
> -    struct hmap_node hmap_node; /* In struct dpif_ipfix's "tunnel_ports" hmap. */
> +    struct hmap_node hmap_node; /* In struct dpif_ipfix's "ports" hmap. */
>       struct ofport *ofport;      /* To retrieve port stats. */
>       odp_port_t odp_port;
>       enum dpif_ipfix_tunnel_type tunnel_type;
>       uint8_t tunnel_key_length;
> +    uint32_t ifindex;
>   };
>   
>   struct dpif_ipfix_exporter {
> @@ -157,9 +158,9 @@ struct dpif_ipfix_flow_exporter_map_node {
>   struct dpif_ipfix {
>       struct dpif_ipfix_bridge_exporter bridge_exporter;
>       struct hmap flow_exporter_map;  /* dpif_ipfix_flow_exporter_map_node. */
> -    struct hmap tunnel_ports;       /* Contains "struct dpif_ipfix_port"s.
> -                                     * It makes tunnel port lookups faster in
> -                                     * sampling upcalls. */
> +    struct hmap ports;              /* Contains "struct dpif_ipfix_port"s.
> +                                     * It makes port lookups faster in sampling
> +                                     * upcalls. */
>       struct ovs_refcount ref_cnt;
>   };
>   
> @@ -293,7 +294,8 @@ BUILD_ASSERT_DECL(sizeof(struct ipfix_template_field_specifier) == 8);
>   /* Cf. IETF RFC 5102 Section 5.11.6. */
>   enum ipfix_flow_direction {
>       INGRESS_FLOW = 0x00,
> -    EGRESS_FLOW = 0x01
> +    EGRESS_FLOW = 0x01,
> +    NUM_IPFIX_FLOW_DIRECTION
>   };
>   
>   /* Part of data record flow key for common metadata and Ethernet entities. */
> @@ -308,6 +310,18 @@ struct ipfix_data_record_flow_key_common {
>   });
>   BUILD_ASSERT_DECL(sizeof(struct ipfix_data_record_flow_key_common) == 20);
>   
> +/* Part of data record flow key for interface information. Since some of the
> + * elements have variable length, members of this structure should be appended
> + * to the 'struct dp_packet' one by one. */
> +struct ipfix_data_record_flow_key_iface {
> +    ovs_be32 if_index;     /* (INGRESS | EGRESS)_INTERFACE */
> +    ovs_be32 if_type;     /* (INGRESS | EGRESS)_INTERFACE_TYPE */
> +    uint8_t if_name_len;   /* Variable length element: INTERFACE_NAME */
> +    char *if_name;
> +    uint8_t if_descr_len; /* Variable length element: INTERFACE_DESCRIPTION */
> +    char *if_descr;
> +};
> +

I think we can close some holes in this structure by placing the two uint8_t members
beside each other.

Like this maybe?

 > +struct ipfix_data_record_flow_key_iface {
 > +    ovs_be32 if_index;     /* (INGRESS | EGRESS)_INTERFACE */
 > +    ovs_be32 if_type;     /* (INGRESS | EGRESS)_INTERFACE_TYPE */
 > +    char *if_name;
 > +    char *if_descr;
 > +    uint8_t if_name_len;   /* Variable length element: INTERFACE_NAME */
 > +    uint8_t if_descr_len; /* Variable length element: INTERFACE_DESCRIPTION */
 > +};

>   /* Part of data record flow key for VLAN entities. */
>   OVS_PACKED(
>   struct ipfix_data_record_flow_key_vlan {
> @@ -745,7 +759,7 @@ dpif_ipfix_find_port(const struct dpif_ipfix *di,
>       struct dpif_ipfix_port *dip;
>   
>       HMAP_FOR_EACH_IN_BUCKET (dip, hmap_node, hash_odp_port(odp_port),
> -                             &di->tunnel_ports) {
> +                             &di->ports) {
>           if (dip->odp_port == odp_port) {
>               return dip;
>           }
> @@ -754,82 +768,116 @@ dpif_ipfix_find_port(const struct dpif_ipfix *di,
>   }
>   
>   static void
> -dpif_ipfix_del_port(struct dpif_ipfix *di,
> +dpif_ipfix_del_port__(struct dpif_ipfix *di,
>                         struct dpif_ipfix_port *dip)
>       OVS_REQUIRES(mutex)
>   {
> -    hmap_remove(&di->tunnel_ports, &dip->hmap_node);
> +    hmap_remove(&di->ports, &dip->hmap_node);
>       free(dip);
>   }
>   
> +static enum dpif_ipfix_tunnel_type
> +dpif_ipfix_tunnel_type(const struct ofport *ofport)
> +{
> +    const char *type = netdev_get_type(ofport->netdev);
> +
> +    if (type == NULL) {
> +        return DPIF_IPFIX_TUNNEL_UNKNOWN;
> +    }
> +    if (strcmp(type, "gre") == 0) {
> +        return DPIF_IPFIX_TUNNEL_GRE;
> +    } else if (strcmp(type, "vxlan") == 0) {
> +        return DPIF_IPFIX_TUNNEL_VXLAN;
> +    } else if (strcmp(type, "lisp") == 0) {
> +        return DPIF_IPFIX_TUNNEL_LISP;
> +    } else if (strcmp(type, "geneve") == 0) {
> +        return DPIF_IPFIX_TUNNEL_GENEVE;
> +    } else if (strcmp(type, "stt") == 0) {
> +        return DPIF_IPFIX_TUNNEL_STT;
> +    }
> +
> +    return DPIF_IPFIX_TUNNEL_UNKNOWN;
> +}
> +
> +static uint8_t
> +dpif_ipfix_tunnel_key_length(enum dpif_ipfix_tunnel_type tunnel_type)
> +{
> +
> +    switch (tunnel_type) {
> +        case DPIF_IPFIX_TUNNEL_GRE:
> +            /* 32-bit key gre */
> +            return 4;
> +        case DPIF_IPFIX_TUNNEL_VXLAN:
> +        case DPIF_IPFIX_TUNNEL_LISP:
> +        case DPIF_IPFIX_TUNNEL_GENEVE:
> +            return 3;
> +        case DPIF_IPFIX_TUNNEL_STT:
> +            return 8;
> +        case DPIF_IPFIX_TUNNEL_UNKNOWN:
> +        case NUM_DPIF_IPFIX_TUNNEL:
> +        default:
> +            return 0;
> +    }
> +}
> +
>   void
> -dpif_ipfix_add_tunnel_port(struct dpif_ipfix *di, struct ofport *ofport,
> -                           odp_port_t odp_port) OVS_EXCLUDED(mutex)
> +dpif_ipfix_add_port(struct dpif_ipfix *di, struct ofport *ofport,
> +                    odp_port_t odp_port) OVS_EXCLUDED(mutex)
>   {
>       struct dpif_ipfix_port *dip;
> -    const char *type;
> +    int64_t ifindex;
>   
>       ovs_mutex_lock(&mutex);
>       dip = dpif_ipfix_find_port(di, odp_port);
>       if (dip) {
> -        dpif_ipfix_del_port(di, dip);
> +        dpif_ipfix_del_port__(di, dip);
>       }
>   
> -    type = netdev_get_type(ofport->netdev);
> -    if (type == NULL) {
> -        goto out;
> +    ifindex = netdev_get_ifindex(ofport->netdev);
> +    if (ifindex < 0) {
> +        ifindex = 0;
>       }
>   
> -    /* Add to table of tunnel ports. */
> +    /* Add to table of ports. */
>       dip = xmalloc(sizeof *dip);
>       dip->ofport = ofport;
>       dip->odp_port = odp_port;
> -    if (strcmp(type, "gre") == 0) {
> -        /* 32-bit key gre */
> -        dip->tunnel_type = DPIF_IPFIX_TUNNEL_GRE;
> -        dip->tunnel_key_length = 4;
> -    } else if (strcmp(type, "vxlan") == 0) {
> -        dip->tunnel_type = DPIF_IPFIX_TUNNEL_VXLAN;
> -        dip->tunnel_key_length = 3;
> -    } else if (strcmp(type, "lisp") == 0) {
> -        dip->tunnel_type = DPIF_IPFIX_TUNNEL_LISP;
> -        dip->tunnel_key_length = 3;
> -    } else if (strcmp(type, "geneve") == 0) {
> -        dip->tunnel_type = DPIF_IPFIX_TUNNEL_GENEVE;
> -        dip->tunnel_key_length = 3;
> -    } else if (strcmp(type, "stt") == 0) {
> -        dip->tunnel_type = DPIF_IPFIX_TUNNEL_STT;
> -        dip->tunnel_key_length = 8;
> -    } else {
> -        free(dip);
> -        goto out;
> -    }
> -    hmap_insert(&di->tunnel_ports, &dip->hmap_node, hash_odp_port(odp_port));
> +    dip->tunnel_type = dpif_ipfix_tunnel_type(ofport);
> +    dip->tunnel_key_length = dpif_ipfix_tunnel_key_length(dip->tunnel_type);
> +    dip->ifindex = ifindex;
> +    hmap_insert(&di->ports, &dip->hmap_node, hash_odp_port(odp_port));
>   
> -out:
>       ovs_mutex_unlock(&mutex);
>   }
>   
>   void
> -dpif_ipfix_del_tunnel_port(struct dpif_ipfix *di, odp_port_t odp_port)
> +dpif_ipfix_del_port(struct dpif_ipfix *di, odp_port_t odp_port)
>       OVS_EXCLUDED(mutex)
>   {
>       struct dpif_ipfix_port *dip;
>       ovs_mutex_lock(&mutex);
>       dip = dpif_ipfix_find_port(di, odp_port);
>       if (dip) {
> -        dpif_ipfix_del_port(di, dip);
> +        dpif_ipfix_del_port__(di, dip);
>       }
>       ovs_mutex_unlock(&mutex);
>   }
>   
> +static struct dpif_ipfix_port *
> +dpif_ipfix_find_tunnel_port(const struct dpif_ipfix *di, odp_port_t odp_port)
> +    OVS_REQUIRES(mutex)
> +{
> +    struct dpif_ipfix_port *dip = dpif_ipfix_find_port(di, odp_port);
> +    return (dip && dip->tunnel_type != DPIF_IPFIX_TUNNEL_UNKNOWN) ? dip : NULL;
> +}
> +
>   bool
> -dpif_ipfix_get_tunnel_port(const struct dpif_ipfix *di, odp_port_t odp_port)
> +dpif_ipfix_is_tunnel_port(const struct dpif_ipfix *di, odp_port_t odp_port)
>       OVS_EXCLUDED(mutex)
>   {
>       struct dpif_ipfix_port *dip;
>       ovs_mutex_lock(&mutex);
> -    dip = dpif_ipfix_find_port(di, odp_port);
> +    dip = dpif_ipfix_find_tunnel_port(di, odp_port);
>       ovs_mutex_unlock(&mutex);
>       return dip != NULL;
>   }
> @@ -1065,7 +1113,7 @@ dpif_ipfix_create(void)
>       di = xzalloc(sizeof *di);
>       dpif_ipfix_bridge_exporter_init(&di->bridge_exporter);
>       hmap_init(&di->flow_exporter_map);
> -    hmap_init(&di->tunnel_ports);
> +    hmap_init(&di->ports);
>       ovs_refcount_init(&di->ref_cnt);
>       return di;
>   }
> @@ -1159,8 +1207,8 @@ dpif_ipfix_clear(struct dpif_ipfix *di) OVS_REQUIRES(mutex)
>           free(exp_node);
>       }
>   
> -    HMAP_FOR_EACH_SAFE (dip, next, hmap_node, &di->tunnel_ports) {
> -        dpif_ipfix_del_port(di, dip);
> +    HMAP_FOR_EACH_SAFE (dip, next, hmap_node, &di->ports) {
> +        dpif_ipfix_del_port__(di, dip);
>       }
>   }
>   
> @@ -1172,7 +1220,7 @@ dpif_ipfix_unref(struct dpif_ipfix *di) OVS_EXCLUDED(mutex)
>           dpif_ipfix_clear(di);
>           dpif_ipfix_bridge_exporter_destroy(&di->bridge_exporter);
>           hmap_destroy(&di->flow_exporter_map);
> -        hmap_destroy(&di->tunnel_ports);
> +        hmap_destroy(&di->ports);
>           free(di);
>           ovs_mutex_unlock(&mutex);
>       }
> @@ -1211,13 +1259,15 @@ ipfix_send_msg(const struct collectors *collectors, struct dp_packet *msg)
>   
>   static uint16_t
>   ipfix_get_template_id(enum ipfix_proto_l2 l2, enum ipfix_proto_l3 l3,
> -                      enum ipfix_proto_l4 l4, enum ipfix_proto_tunnel tunnel)
> +                      enum ipfix_proto_l4 l4, enum ipfix_proto_tunnel tunnel,
> +                      enum ipfix_flow_direction flow_direction)
>   {
>       uint16_t template_id;
>       template_id = l2;
>       template_id = template_id * NUM_IPFIX_PROTO_L3 + l3;
>       template_id = template_id * NUM_IPFIX_PROTO_L4 + l4;
>       template_id = template_id * NUM_IPFIX_PROTO_TUNNEL + tunnel;
> +    template_id = template_id * NUM_IPFIX_FLOW_DIRECTION + flow_direction;
>       return IPFIX_TEMPLATE_ID_MIN + template_id;
>   }
>   
> @@ -1229,7 +1279,8 @@ ipfix_get_options_template_id(enum ipfix_options_template opt_tmpl_type)
>       uint16_t max_tmpl_id = ipfix_get_template_id(NUM_IPFIX_PROTO_L2,
>                                                    NUM_IPFIX_PROTO_L3,
>                                                    NUM_IPFIX_PROTO_L4,
> -                                                 NUM_IPFIX_PROTO_TUNNEL);
> +                                                 NUM_IPFIX_PROTO_TUNNEL,
> +                                                 NUM_IPFIX_FLOW_DIRECTION);
>   
>       return max_tmpl_id + opt_tmpl_type;
>   }
> @@ -1325,7 +1376,9 @@ ipfix_def_options_template_fields(enum ipfix_options_template opt_tmpl_type,
>   static uint16_t
>   ipfix_define_template_fields(enum ipfix_proto_l2 l2, enum ipfix_proto_l3 l3,
>                                enum ipfix_proto_l4 l4, enum ipfix_proto_tunnel tunnel,
> -                             bool virtual_obs_id_set, size_t tmpl_hdr_offset,
> +                             enum ipfix_flow_direction flow_direction,
> +                             bool virtual_obs_id_set,
> +                             size_t tmpl_hdr_offset,
>                                struct dp_packet *msg)
>   {
>   
> @@ -1343,6 +1396,19 @@ ipfix_define_template_fields(enum ipfix_proto_l2 l2, enum ipfix_proto_l3 l3,
>       DEF(ETHERNET_TYPE);
>       DEF(ETHERNET_HEADER_LENGTH);
>   
> +    /* Interface Information Elements */
> +    DEF(INGRESS_INTERFACE);
> +    DEF(INGRESS_INTERFACE_TYPE);
> +    DEF(INTERFACE_NAME);
> +    DEF(INTERFACE_DESCRIPTION);
> +
> +    if (flow_direction == EGRESS_FLOW) {
> +        DEF(EGRESS_INTERFACE);
> +        DEF(EGRESS_INTERFACE_TYPE);
> +        DEF(INTERFACE_NAME);
> +        DEF(INTERFACE_DESCRIPTION);
> +    }
> +
>       if (l2 == IPFIX_PROTO_L2_VLAN) {
>           DEF(VLAN_ID);
>           DEF(DOT1Q_VLAN_ID);
> @@ -1544,6 +1610,24 @@ ipfix_send_options_template_msgs(struct dpif_ipfix_exporter *exporter,
>   }
>   
>   static void
> +ipfix_add_template_record(enum ipfix_proto_l2 l2, enum ipfix_proto_l3 l3,
> +                          enum ipfix_proto_l4 l4,
> +                          enum ipfix_proto_tunnel tunnel,
> +                          enum ipfix_flow_direction flow_direction,
> +                          bool virtual_obs_id_set,
> +                          struct dp_packet *msg)
> +{
> +    struct ipfix_template_record_header *tmpl_hdr;
> +    size_t tmpl_hdr_offset = dp_packet_size(msg);
> +
> +    tmpl_hdr = dp_packet_put_zeros(msg, sizeof *tmpl_hdr);
> +    tmpl_hdr->template_id =
> +        htons(ipfix_get_template_id(l2, l3, l4, tunnel, flow_direction));
> +    ipfix_define_template_fields(l2, l3, l4, tunnel, flow_direction,
> +                                 virtual_obs_id_set, tmpl_hdr_offset, msg);
> +}
> +
> +static void
>   ipfix_send_template_msgs(struct dpif_ipfix_exporter *exporter,
>                            uint32_t export_time_sec, uint32_t obs_domain_id)
>   {
> @@ -1551,14 +1635,14 @@ ipfix_send_template_msgs(struct dpif_ipfix_exporter *exporter,
>       struct dp_packet msg;
>       dp_packet_use_stub(&msg, msg_stub, sizeof msg_stub);
>   
> -    size_t set_hdr_offset, tmpl_hdr_offset, error_pkts;
> -    struct ipfix_template_record_header *tmpl_hdr;
> +    size_t set_hdr_offset, error_pkts;
>       size_t tx_packets = 0;
>       size_t tx_errors = 0;
>       enum ipfix_proto_l2 l2;
>       enum ipfix_proto_l3 l3;
>       enum ipfix_proto_l4 l4;
>       enum ipfix_proto_tunnel tunnel;
> +    enum ipfix_flow_direction flow_direction;
>   
>       ipfix_init_template_msg(export_time_sec, exporter->seq_number,
>                               obs_domain_id, IPFIX_SET_ID_TEMPLATE, &msg,
> @@ -1573,41 +1657,44 @@ ipfix_send_template_msgs(struct dpif_ipfix_exporter *exporter,
>                       continue;
>                   }
>                   for (tunnel = 0; tunnel < NUM_IPFIX_PROTO_TUNNEL; tunnel++) {
> -                    /* When the size of the template packet reaches
> -                     * MAX_MESSAGE_LEN(1024), send it out.
> -                     * And then reinitialize the msg to construct a new
> -                     * packet for the following templates.
> -                     */
> -                    if (dp_packet_size(&msg) >= MAX_MESSAGE_LEN) {
> -                        /* Send template message. */
> -                        error_pkts = ipfix_send_template_msg(exporter->collectors,
> -                                                             &msg, set_hdr_offset);
> -                        tx_errors += error_pkts;
> -                        tx_packets += collectors_count(exporter->collectors) - error_pkts;
> -
> -                        /* Reinitialize the template msg. */
> -                        ipfix_init_template_msg(export_time_sec,
> -                                                exporter->seq_number,
> -                                                obs_domain_id,
> -                                                IPFIX_SET_ID_TEMPLATE,
> -                                                &msg,
> -                                                &set_hdr_offset);
> +                    for (flow_direction = 0;
> +                         flow_direction < NUM_IPFIX_FLOW_DIRECTION;
> +                         flow_direction++) {
> +                        /* When the size of the template packet reaches
> +                         * MAX_MESSAGE_LEN(1024), send it out.
> +                         * And then reinitialize the msg to construct a new
> +                         * packet for the following templates.
> +                         */
> +                        if (dp_packet_size(&msg) >= MAX_MESSAGE_LEN) {
> +                            /* Send template message. */
> +                            error_pkts =
> +                                ipfix_send_template_msg(exporter->collectors,
> +                                                        &msg, set_hdr_offset);
> +                            tx_errors += error_pkts;
> +                            tx_packets +=
> +                                collectors_count(exporter->collectors)
> +                                - error_pkts;
> +
> +                            /* Reinitialize the template msg. */
> +                            ipfix_init_template_msg(export_time_sec,
> +                                                    exporter->seq_number,
> +                                                    obs_domain_id,
> +                                                    IPFIX_SET_ID_TEMPLATE,
> +                                                    &msg, &set_hdr_offset);
> +                        }
> +
> +                        ipfix_add_template_record(l2, l3, l4, tunnel,
> +                                flow_direction,
> +                                exporter->virtual_obs_id != NULL, &msg);
>                       }
> -
> -                    tmpl_hdr_offset = dp_packet_size(&msg);
> -                    tmpl_hdr = dp_packet_put_zeros(&msg, sizeof *tmpl_hdr);
> -                    tmpl_hdr->template_id = htons(
> -                        ipfix_get_template_id(l2, l3, l4, tunnel));
> -                    ipfix_define_template_fields(
> -                        l2, l3, l4, tunnel, exporter->virtual_obs_id != NULL,
> -                        tmpl_hdr_offset, &msg);
>                   }
>               }
>           }
>       }
>   
>       /* Send template message. */
> -    error_pkts = ipfix_send_template_msg(exporter->collectors, &msg, set_hdr_offset);
> +    error_pkts = ipfix_send_template_msg(exporter->collectors, &msg,
> +                                         set_hdr_offset);
>       tx_errors += error_pkts;
>       tx_packets += collectors_count(exporter->collectors) - error_pkts;
>   
> @@ -1909,8 +1996,80 @@ ipfix_cache_update(struct dpif_ipfix_exporter *exporter,
>       }
>   }
>   
> +static void
> +ipfix_destroy_iface_data_record(struct ipfix_data_record_flow_key_iface *data)
> +{
> +    free(data->if_descr);
> +    free(data->if_name);
> +}
> +
> +/* Fills '*data' structure based on port number 'port_no'.  Caller must destroy
> + * 'data' with ipfix_destroy_iface_data_record(). */
> +static int
> +ipfix_get_iface_data_record(const struct dpif_ipfix *di, odp_port_t port_no,
> +                            struct ipfix_data_record_flow_key_iface *data)
> +    OVS_REQUIRES(mutex)
> +{
> +    struct dpif_ipfix_port *port;
> +    struct smap netdev_status;
> +
> +    port = dpif_ipfix_find_port(di, port_no);
> +    if (!port) {
> +        return -1;
> +    }
> +
> +    smap_init(&netdev_status);
> +    if (!netdev_get_status(port->ofport->netdev, &netdev_status)) {
> +        data->if_type = htonl(smap_get_ullong(&netdev_status, "if_type", 0));
> +        data->if_descr = nullable_xstrdup(smap_get(&netdev_status,
> +                                                   "if_descr"));
> +    } else {
> +        data->if_type = 0;
> +        data->if_descr = NULL;
> +    }
> +
> +    smap_destroy(&netdev_status);
> +    data->if_index = htonl(port->ifindex);
> +    data->if_descr_len = data->if_descr ? strlen(data->if_descr) : 0;
> +    data->if_name = nullable_xstrdup(netdev_get_name(port->ofport->netdev));
> +    data->if_name_len = data->if_name ? strlen(data->if_name) : 0;
> +
> +    return 0;
> +}
> +
> +static void
> +ipfix_put_iface_data_record(const struct dpif_ipfix *di, odp_port_t port_no,
> +                            struct dp_packet *msg)
> +    OVS_REQUIRES(mutex)
> +{
> +    struct ipfix_data_record_flow_key_iface data;
> +    int err;
> +
> +    memset(&data, 0, sizeof(struct ipfix_data_record_flow_key_iface));
> +    err = ipfix_get_iface_data_record(di, port_no, &data);
> +    if (err == 0) {
> +        dp_packet_put(msg, &data.if_index, sizeof data.if_index);
> +        dp_packet_put(msg, &data.if_type, sizeof data.if_type);
> +        dp_packet_put(msg, &data.if_name_len, sizeof data.if_name_len);
> +        if (data.if_name_len) {
> +            dp_packet_put(msg, data.if_name, data.if_name_len);
> +        }
> +        dp_packet_put(msg, &data.if_descr_len, sizeof data.if_descr_len);
> +        if (data.if_descr_len) {
> +            dp_packet_put(msg, data.if_descr, data.if_descr_len);
> +        }
> +        ipfix_destroy_iface_data_record(&data);
> +    } else {
> +        dp_packet_put_zeros(msg, sizeof data.if_index);
> +        dp_packet_put_zeros(msg, sizeof data.if_type);
> +        dp_packet_put_zeros(msg, sizeof data.if_name_len);
> +        dp_packet_put_zeros(msg, sizeof data.if_descr_len);
> +    }
> +}
> +
>   static enum ipfix_sampled_packet_type
> -ipfix_cache_entry_init(struct ipfix_flow_cache_entry *entry,
> +ipfix_cache_entry_init(const struct dpif_ipfix *di,
> +                       struct ipfix_flow_cache_entry *entry,
>                          const struct dp_packet *packet, const struct flow *flow,
>                          uint64_t packet_delta_count, uint32_t obs_domain_id,
>                          uint32_t obs_point_id, odp_port_t output_odp_port,
> @@ -1919,6 +2078,7 @@ ipfix_cache_entry_init(struct ipfix_flow_cache_entry *entry,
>                          const struct flow_tnl *tunnel_key,
>                          struct dpif_ipfix_global_stats *stats,
>                          const struct dpif_ipfix_actions *ipfix_actions)
> +    OVS_REQUIRES(mutex)
>   {
>       struct ipfix_flow_key *flow_key;
>       struct dp_packet msg;
> @@ -1993,8 +2153,14 @@ ipfix_cache_entry_init(struct ipfix_flow_cache_entry *entry,
>          tunnel = IPFIX_PROTO_TUNNELED;
>       }
>   
> +    uint8_t flow_direction =
> +        (direction == NX_ACTION_SAMPLE_INGRESS ? INGRESS_FLOW
> +         : direction == NX_ACTION_SAMPLE_EGRESS ? EGRESS_FLOW
> +         : output_odp_port == ODPP_NONE ? INGRESS_FLOW : EGRESS_FLOW);
> +
>       flow_key->obs_domain_id = obs_domain_id;
> -    flow_key->template_id = ipfix_get_template_id(l2, l3, l4, tunnel);
> +    flow_key->template_id = ipfix_get_template_id(l2, l3, l4, tunnel,
> +                                                  flow_direction);
>   
>       /* The fields defined in the ipfix_data_record_* structs and sent
>        * below must match exactly the templates defined in
> @@ -2004,11 +2170,6 @@ ipfix_cache_entry_init(struct ipfix_flow_cache_entry *entry,
>           ? VLAN_ETH_HEADER_LEN : ETH_HEADER_LEN;
>       ethernet_total_length = dp_packet_size(packet);
>   
> -    uint8_t flow_direction =
> -        (direction == NX_ACTION_SAMPLE_INGRESS ? INGRESS_FLOW
> -         : direction == NX_ACTION_SAMPLE_EGRESS ? EGRESS_FLOW
> -         : output_odp_port == ODPP_NONE ? INGRESS_FLOW : EGRESS_FLOW);
> -
>       /* Common Ethernet entities. */
>       {
>           struct ipfix_data_record_flow_key_common *data_common;
> @@ -2022,6 +2183,13 @@ ipfix_cache_entry_init(struct ipfix_flow_cache_entry *entry,
>           data_common->ethernet_header_length = ethernet_header_length;
>       }
>   
> +    /* Interface Information Elements */
> +    ipfix_put_iface_data_record(di, flow->in_port.odp_port, &msg);
> +
> +    if (flow_direction == EGRESS_FLOW) {
> +        ipfix_put_iface_data_record(di, output_odp_port, &msg);
> +    }
> +
>       if (l2 == IPFIX_PROTO_L2_VLAN) {
>           struct ipfix_data_record_flow_key_vlan *data_vlan;
>           uint16_t vlan_id = vlan_tci_to_vid(flow->vlans[0].tci);
> @@ -2469,7 +2637,8 @@ ipfix_send_data_msg(struct dpif_ipfix_exporter *exporter,
>   }
>   
>   static void
> -dpif_ipfix_sample(struct dpif_ipfix_exporter *exporter,
> +dpif_ipfix_sample(const struct dpif_ipfix *di,
> +                  struct dpif_ipfix_exporter *exporter,
>                     const struct dp_packet *packet, const struct flow *flow,
>                     uint64_t packet_delta_count, uint32_t obs_domain_id,
>                     uint32_t obs_point_id, odp_port_t output_odp_port,
> @@ -2477,6 +2646,7 @@ dpif_ipfix_sample(struct dpif_ipfix_exporter *exporter,
>                     const struct dpif_ipfix_port *tunnel_port,
>                     const struct flow_tnl *tunnel_key,
>                     const struct dpif_ipfix_actions *ipfix_actions)
> +    OVS_REQUIRES(mutex)
>   {
>       struct ipfix_flow_cache_entry *entry;
>       enum ipfix_sampled_packet_type sampled_packet_type;
> @@ -2484,7 +2654,7 @@ dpif_ipfix_sample(struct dpif_ipfix_exporter *exporter,
>       /* Create a flow cache entry from the sample. */
>       entry = xmalloc(sizeof *entry);
>       sampled_packet_type =
> -            ipfix_cache_entry_init(entry, packet,
> +            ipfix_cache_entry_init(di, entry, packet,
>                                      flow, packet_delta_count,
>                                      obs_domain_id, obs_point_id,
>                                      output_odp_port, direction,
> @@ -2542,16 +2712,16 @@ dpif_ipfix_bridge_sample(struct dpif_ipfix *di, const struct dp_packet *packet,
>           if (output_odp_port == ODPP_NONE && flow->tunnel.ip_dst) {
>               /* Input tunnel. */
>               tunnel_key = &flow->tunnel;
> -            tunnel_port = dpif_ipfix_find_port(di, input_odp_port);
> +            tunnel_port = dpif_ipfix_find_tunnel_port(di, input_odp_port);
>           }
>           if (output_odp_port != ODPP_NONE && output_tunnel_key) {
>               /* Output tunnel, output_tunnel_key must be valid. */
>               tunnel_key = output_tunnel_key;
> -            tunnel_port = dpif_ipfix_find_port(di, output_odp_port);
> +            tunnel_port = dpif_ipfix_find_tunnel_port(di, output_odp_port);
>           }
>       }
>   
> -    dpif_ipfix_sample(&di->bridge_exporter.exporter, packet, flow,
> +    dpif_ipfix_sample(di, &di->bridge_exporter.exporter, packet, flow,
>                         packet_delta_count,
>                         di->bridge_exporter.options->obs_domain_id,
>                         di->bridge_exporter.options->obs_point_id,
> @@ -2587,16 +2757,16 @@ dpif_ipfix_flow_sample(struct dpif_ipfix *di, const struct dp_packet *packet,
>               if (output_odp_port == ODPP_NONE && flow->tunnel.ip_dst) {
>                   /* Input tunnel. */
>                   tunnel_key = &flow->tunnel;
> -                tunnel_port = dpif_ipfix_find_port(di, input_odp_port);
> +                tunnel_port = dpif_ipfix_find_tunnel_port(di, input_odp_port);
>               }
>               if (output_odp_port != ODPP_NONE && output_tunnel_key) {
>                   /* Output tunnel, output_tunnel_key must be valid. */
>                   tunnel_key = output_tunnel_key;
> -                tunnel_port = dpif_ipfix_find_port(di, output_odp_port);
> +                tunnel_port = dpif_ipfix_find_tunnel_port(di, output_odp_port);
>               }
>           }
>   
> -        dpif_ipfix_sample(&node->exporter.exporter, packet, flow,
> +        dpif_ipfix_sample(di, &node->exporter.exporter, packet, flow,
>                             packet_delta_count,
>                             cookie->flow_sample.obs_domain_id,
>                             cookie->flow_sample.obs_point_id,
> diff --git a/ofproto/ofproto-dpif-ipfix.h b/ofproto/ofproto-dpif-ipfix.h
> index f91d041..1309da1 100644
> --- a/ofproto/ofproto-dpif-ipfix.h
> +++ b/ofproto/ofproto-dpif-ipfix.h
> @@ -38,8 +38,8 @@ struct dpif_ipfix *dpif_ipfix_create(void);
>   struct dpif_ipfix *dpif_ipfix_ref(const struct dpif_ipfix *);
>   void dpif_ipfix_unref(struct dpif_ipfix *);
>   
> -void dpif_ipfix_add_tunnel_port(struct dpif_ipfix *, struct ofport *, odp_port_t);
> -void dpif_ipfix_del_tunnel_port(struct dpif_ipfix *, odp_port_t);
> +void dpif_ipfix_add_port(struct dpif_ipfix *, struct ofport *, odp_port_t);
> +void dpif_ipfix_del_port(struct dpif_ipfix *, odp_port_t);
>   
>   uint32_t dpif_ipfix_get_bridge_exporter_probability(const struct dpif_ipfix *);
>   bool dpif_ipfix_get_bridge_exporter_tunnel_sampling(const struct dpif_ipfix *);
> @@ -47,7 +47,7 @@ bool dpif_ipfix_get_bridge_exporter_input_sampling(const struct dpif_ipfix *);
>   bool dpif_ipfix_get_bridge_exporter_output_sampling(const struct dpif_ipfix *);
>   bool dpif_ipfix_get_flow_exporter_tunnel_sampling(const struct dpif_ipfix *,
>                                                     const uint32_t);
> -bool dpif_ipfix_get_tunnel_port(const struct dpif_ipfix *, odp_port_t);
> +bool dpif_ipfix_is_tunnel_port(const struct dpif_ipfix *, odp_port_t);
>   void dpif_ipfix_set_options(
>       struct dpif_ipfix *,
>       const struct ofproto_ipfix_bridge_exporter_options *,
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 9e1f837..aac135f 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -2952,7 +2952,7 @@ compose_ipfix_action(struct xlate_ctx *ctx, odp_port_t output_odp_port)
>            * OVS_USERSPACE_ATTR_TUNNEL_OUT_PORT
>            */
>           if (dpif_ipfix_get_bridge_exporter_tunnel_sampling(ipfix) &&
> -            dpif_ipfix_get_tunnel_port(ipfix, output_odp_port) ) {
> +            dpif_ipfix_is_tunnel_port(ipfix, output_odp_port) ) {
>              tunnel_out_port = output_odp_port;
>           }
>       }
> @@ -5211,7 +5211,7 @@ xlate_sample_action(struct xlate_ctx *ctx,
>   
>           if (dpif_ipfix_get_flow_exporter_tunnel_sampling(ipfix,
>                                                            os->collector_set_id)
> -            && dpif_ipfix_get_tunnel_port(ipfix, output_odp_port)) {
> +            && dpif_ipfix_is_tunnel_port(ipfix, output_odp_port)) {
>               tunnel_out_port = output_odp_port;
>               emit_set_tunnel = true;
>           }
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 1a8e829..f9c8749 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -1872,9 +1872,6 @@ port_construct(struct ofport *port_)
>           }
>   
>           port->is_tunnel = true;
> -        if (ofproto->ipfix) {
> -           dpif_ipfix_add_tunnel_port(ofproto->ipfix, port_, port->odp_port);
> -        }
>       } else {
>           /* Sanity-check that a mapping doesn't already exist.  This
>            * shouldn't happen for non-tunnel ports. */
> @@ -1895,6 +1892,9 @@ port_construct(struct ofport *port_)
>       if (ofproto->sflow) {
>           dpif_sflow_add_port(ofproto->sflow, port_, port->odp_port);
>       }
> +    if (ofproto->ipfix) {
> +       dpif_ipfix_add_port(ofproto->ipfix, port_, port->odp_port);
> +    }
>   
>       return 0;
>   }
> @@ -1940,10 +1940,6 @@ port_destruct(struct ofport *port_, bool del)
>           atomic_count_dec(&ofproto->backer->tnl_count);
>       }
>   
> -    if (port->is_tunnel && ofproto->ipfix) {
> -       dpif_ipfix_del_tunnel_port(ofproto->ipfix, port->odp_port);
> -    }
> -
>       tnl_port_del(port);
>       sset_find_and_delete(&ofproto->ports, devname);
>       sset_find_and_delete(&ofproto->ghost_ports, devname);
> @@ -1958,6 +1954,9 @@ port_destruct(struct ofport *port_, bool del)
>       if (ofproto->sflow) {
>           dpif_sflow_del_port(ofproto->sflow, port->odp_port);
>       }
> +    if (ofproto->ipfix) {
> +       dpif_ipfix_del_port(ofproto->ipfix, port->odp_port);
> +    }
>   
>       free(port->qdscp);
>   }
> @@ -2083,13 +2082,11 @@ set_ipfix(
>               di, bridge_exporter_options, flow_exporters_options,
>               n_flow_exporters_options);
>   
> -        /* Add tunnel ports only when a new ipfix created */
> +        /* Add ports only when a new ipfix created */
>           if (new_di == true) {
>               struct ofport_dpif *ofport;
>               HMAP_FOR_EACH (ofport, up.hmap_node, &ofproto->up.ports) {
> -                if (ofport->is_tunnel == true) {
> -                    dpif_ipfix_add_tunnel_port(di, &ofport->up, ofport->odp_port);
> -                }
> +                dpif_ipfix_add_port(di, &ofport->up, ofport->odp_port);
>               }
>           }
>   
>
Weglicki, MichalX Oct. 4, 2017, 8:18 a.m. | #2
> -----Original Message-----
> From: Greg Rose [mailto:gvrose8192@gmail.com]
> Sent: Wednesday, October 4, 2017 12:21 AM
> To: Weglicki, MichalX <michalx.weglicki@intel.com>; dev@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH v5 2/2] ofproto-dpif-ipfix: add interface Information Elements to flow key
> 
> On 10/02/2017 07:49 AM, Michal Weglicki wrote:
> > Extend flow key part of data record to include following Information Elements:
> > - ingressInterface
> > - ingressInterfaceType
> > - egressInterface
> > - egressInterfaceType
> > - interfaceName
> > - interfaceDescription
> >
> > In case of input sampling we don't have information about egress port.
> > Define templates depending not only on protocol types, but also on flow
> > direction. Only egress flow will include egress information elements.
> >
> > With this change, dpif_ipfix_exporter stores every port in hmap rather
> > than only tunnel ports. It allows to easily retrieve required
> > information about interfaces during sampling upcalls.
> >
> > v1->v2
> > * Add interfaceType and interfaceDescription
> > * Rework ipfix_get_iface_data_record function
> > v2->v3: Code rebase.
> > v3->v4: Minor comments applied.
> > v4->v5: Clang complation problem fix.
> >
> > Co-authored-by: Michal Weglicki <michalx.weglicki@intel.com>
> > Signed-off-by: Michal Weglicki <michalx.weglicki@intel.com>
> > Signed-off-by: Przemyslaw Szczerbik <przemyslawx.szczerbik@intel.com>
> 
> Michal and Przemyslaw,
> 
> There is one more small nit pick that I came across noted below.
> 
> Other than that....
> 
> Tested-by: Greg Rose <gvrose8192@gmail.com>
> Reviewed-by: Greg Rose <gvrose8192@gmail.com>
> 
> > ---
> >   ofproto/ofproto-dpif-ipfix.c | 358 +++++++++++++++++++++++++++++++------------
> >   ofproto/ofproto-dpif-ipfix.h |   6 +-
> >   ofproto/ofproto-dpif-xlate.c |   4 +-
> >   ofproto/ofproto-dpif.c       |  19 +--
> >   4 files changed, 277 insertions(+), 110 deletions(-)
> >
> > diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
> > index 472c272..138c325 100644
> > --- a/ofproto/ofproto-dpif-ipfix.c
> > +++ b/ofproto/ofproto-dpif-ipfix.c
> > @@ -115,11 +115,12 @@ struct dpif_ipfix_global_stats {
> >   };
> >
> >   struct dpif_ipfix_port {
> > -    struct hmap_node hmap_node; /* In struct dpif_ipfix's "tunnel_ports" hmap. */
> > +    struct hmap_node hmap_node; /* In struct dpif_ipfix's "ports" hmap. */
> >       struct ofport *ofport;      /* To retrieve port stats. */
> >       odp_port_t odp_port;
> >       enum dpif_ipfix_tunnel_type tunnel_type;
> >       uint8_t tunnel_key_length;
> > +    uint32_t ifindex;
> >   };
> >
> >   struct dpif_ipfix_exporter {
> > @@ -157,9 +158,9 @@ struct dpif_ipfix_flow_exporter_map_node {
> >   struct dpif_ipfix {
> >       struct dpif_ipfix_bridge_exporter bridge_exporter;
> >       struct hmap flow_exporter_map;  /* dpif_ipfix_flow_exporter_map_node. */
> > -    struct hmap tunnel_ports;       /* Contains "struct dpif_ipfix_port"s.
> > -                                     * It makes tunnel port lookups faster in
> > -                                     * sampling upcalls. */
> > +    struct hmap ports;              /* Contains "struct dpif_ipfix_port"s.
> > +                                     * It makes port lookups faster in sampling
> > +                                     * upcalls. */
> >       struct ovs_refcount ref_cnt;
> >   };
> >
> > @@ -293,7 +294,8 @@ BUILD_ASSERT_DECL(sizeof(struct ipfix_template_field_specifier) == 8);
> >   /* Cf. IETF RFC 5102 Section 5.11.6. */
> >   enum ipfix_flow_direction {
> >       INGRESS_FLOW = 0x00,
> > -    EGRESS_FLOW = 0x01
> > +    EGRESS_FLOW = 0x01,
> > +    NUM_IPFIX_FLOW_DIRECTION
> >   };
> >
> >   /* Part of data record flow key for common metadata and Ethernet entities. */
> > @@ -308,6 +310,18 @@ struct ipfix_data_record_flow_key_common {
> >   });
> >   BUILD_ASSERT_DECL(sizeof(struct ipfix_data_record_flow_key_common) == 20);
> >
> > +/* Part of data record flow key for interface information. Since some of the
> > + * elements have variable length, members of this structure should be appended
> > + * to the 'struct dp_packet' one by one. */
> > +struct ipfix_data_record_flow_key_iface {
> > +    ovs_be32 if_index;     /* (INGRESS | EGRESS)_INTERFACE */
> > +    ovs_be32 if_type;     /* (INGRESS | EGRESS)_INTERFACE_TYPE */
> > +    uint8_t if_name_len;   /* Variable length element: INTERFACE_NAME */
> > +    char *if_name;
> > +    uint8_t if_descr_len; /* Variable length element: INTERFACE_DESCRIPTION */
> > +    char *if_descr;
> > +};
> > +
> 
> I think we can close some holes in this structure by placing the two uint8_t members
> beside each other.
> 
> Like this maybe?
> 
>  > +struct ipfix_data_record_flow_key_iface {
>  > +    ovs_be32 if_index;     /* (INGRESS | EGRESS)_INTERFACE */
>  > +    ovs_be32 if_type;     /* (INGRESS | EGRESS)_INTERFACE_TYPE */
>  > +    char *if_name;
>  > +    char *if_descr;
>  > +    uint8_t if_name_len;   /* Variable length element: INTERFACE_NAME */
>  > +    uint8_t if_descr_len; /* Variable length element: INTERFACE_DESCRIPTION */
>  > +};
I really think that keeping each pointer with length value makes more sense, 
As those values are strictly connected to each other. Keeping it that way 
make it visible that those values are describing same buffer. 

> 
> >   /* Part of data record flow key for VLAN entities. */
> >   OVS_PACKED(
> >   struct ipfix_data_record_flow_key_vlan {
> > @@ -745,7 +759,7 @@ dpif_ipfix_find_port(const struct dpif_ipfix *di,
> >       struct dpif_ipfix_port *dip;
> >
> >       HMAP_FOR_EACH_IN_BUCKET (dip, hmap_node, hash_odp_port(odp_port),
> > -                             &di->tunnel_ports) {
> > +                             &di->ports) {
> >           if (dip->odp_port == odp_port) {
> >               return dip;
> >           }
> > @@ -754,82 +768,116 @@ dpif_ipfix_find_port(const struct dpif_ipfix *di,
> >   }
> >
> >   static void
> > -dpif_ipfix_del_port(struct dpif_ipfix *di,
> > +dpif_ipfix_del_port__(struct dpif_ipfix *di,
> >                         struct dpif_ipfix_port *dip)
> >       OVS_REQUIRES(mutex)
> >   {
> > -    hmap_remove(&di->tunnel_ports, &dip->hmap_node);
> > +    hmap_remove(&di->ports, &dip->hmap_node);
> >       free(dip);
> >   }
> >
> > +static enum dpif_ipfix_tunnel_type
> > +dpif_ipfix_tunnel_type(const struct ofport *ofport)
> > +{
> > +    const char *type = netdev_get_type(ofport->netdev);
> > +
> > +    if (type == NULL) {
> > +        return DPIF_IPFIX_TUNNEL_UNKNOWN;
> > +    }
> > +    if (strcmp(type, "gre") == 0) {
> > +        return DPIF_IPFIX_TUNNEL_GRE;
> > +    } else if (strcmp(type, "vxlan") == 0) {
> > +        return DPIF_IPFIX_TUNNEL_VXLAN;
> > +    } else if (strcmp(type, "lisp") == 0) {
> > +        return DPIF_IPFIX_TUNNEL_LISP;
> > +    } else if (strcmp(type, "geneve") == 0) {
> > +        return DPIF_IPFIX_TUNNEL_GENEVE;
> > +    } else if (strcmp(type, "stt") == 0) {
> > +        return DPIF_IPFIX_TUNNEL_STT;
> > +    }
> > +
> > +    return DPIF_IPFIX_TUNNEL_UNKNOWN;
> > +}
> > +
> > +static uint8_t
> > +dpif_ipfix_tunnel_key_length(enum dpif_ipfix_tunnel_type tunnel_type)
> > +{
> > +
> > +    switch (tunnel_type) {
> > +        case DPIF_IPFIX_TUNNEL_GRE:
> > +            /* 32-bit key gre */
> > +            return 4;
> > +        case DPIF_IPFIX_TUNNEL_VXLAN:
> > +        case DPIF_IPFIX_TUNNEL_LISP:
> > +        case DPIF_IPFIX_TUNNEL_GENEVE:
> > +            return 3;
> > +        case DPIF_IPFIX_TUNNEL_STT:
> > +            return 8;
> > +        case DPIF_IPFIX_TUNNEL_UNKNOWN:
> > +        case NUM_DPIF_IPFIX_TUNNEL:
> > +        default:
> > +            return 0;
> > +    }
> > +}
> > +
> >   void
> > -dpif_ipfix_add_tunnel_port(struct dpif_ipfix *di, struct ofport *ofport,
> > -                           odp_port_t odp_port) OVS_EXCLUDED(mutex)
> > +dpif_ipfix_add_port(struct dpif_ipfix *di, struct ofport *ofport,
> > +                    odp_port_t odp_port) OVS_EXCLUDED(mutex)
> >   {
> >       struct dpif_ipfix_port *dip;
> > -    const char *type;
> > +    int64_t ifindex;
> >
> >       ovs_mutex_lock(&mutex);
> >       dip = dpif_ipfix_find_port(di, odp_port);
> >       if (dip) {
> > -        dpif_ipfix_del_port(di, dip);
> > +        dpif_ipfix_del_port__(di, dip);
> >       }
> >
> > -    type = netdev_get_type(ofport->netdev);
> > -    if (type == NULL) {
> > -        goto out;
> > +    ifindex = netdev_get_ifindex(ofport->netdev);
> > +    if (ifindex < 0) {
> > +        ifindex = 0;
> >       }
> >
> > -    /* Add to table of tunnel ports. */
> > +    /* Add to table of ports. */
> >       dip = xmalloc(sizeof *dip);
> >       dip->ofport = ofport;
> >       dip->odp_port = odp_port;
> > -    if (strcmp(type, "gre") == 0) {
> > -        /* 32-bit key gre */
> > -        dip->tunnel_type = DPIF_IPFIX_TUNNEL_GRE;
> > -        dip->tunnel_key_length = 4;
> > -    } else if (strcmp(type, "vxlan") == 0) {
> > -        dip->tunnel_type = DPIF_IPFIX_TUNNEL_VXLAN;
> > -        dip->tunnel_key_length = 3;
> > -    } else if (strcmp(type, "lisp") == 0) {
> > -        dip->tunnel_type = DPIF_IPFIX_TUNNEL_LISP;
> > -        dip->tunnel_key_length = 3;
> > -    } else if (strcmp(type, "geneve") == 0) {
> > -        dip->tunnel_type = DPIF_IPFIX_TUNNEL_GENEVE;
> > -        dip->tunnel_key_length = 3;
> > -    } else if (strcmp(type, "stt") == 0) {
> > -        dip->tunnel_type = DPIF_IPFIX_TUNNEL_STT;
> > -        dip->tunnel_key_length = 8;
> > -    } else {
> > -        free(dip);
> > -        goto out;
> > -    }
> > -    hmap_insert(&di->tunnel_ports, &dip->hmap_node, hash_odp_port(odp_port));
> > +    dip->tunnel_type = dpif_ipfix_tunnel_type(ofport);
> > +    dip->tunnel_key_length = dpif_ipfix_tunnel_key_length(dip->tunnel_type);
> > +    dip->ifindex = ifindex;
> > +    hmap_insert(&di->ports, &dip->hmap_node, hash_odp_port(odp_port));
> >
> > -out:
> >       ovs_mutex_unlock(&mutex);
> >   }
> >
> >   void
> > -dpif_ipfix_del_tunnel_port(struct dpif_ipfix *di, odp_port_t odp_port)
> > +dpif_ipfix_del_port(struct dpif_ipfix *di, odp_port_t odp_port)
> >       OVS_EXCLUDED(mutex)
> >   {
> >       struct dpif_ipfix_port *dip;
> >       ovs_mutex_lock(&mutex);
> >       dip = dpif_ipfix_find_port(di, odp_port);
> >       if (dip) {
> > -        dpif_ipfix_del_port(di, dip);
> > +        dpif_ipfix_del_port__(di, dip);
> >       }
> >       ovs_mutex_unlock(&mutex);
> >   }
> >
> > +static struct dpif_ipfix_port *
> > +dpif_ipfix_find_tunnel_port(const struct dpif_ipfix *di, odp_port_t odp_port)
> > +    OVS_REQUIRES(mutex)
> > +{
> > +    struct dpif_ipfix_port *dip = dpif_ipfix_find_port(di, odp_port);
> > +    return (dip && dip->tunnel_type != DPIF_IPFIX_TUNNEL_UNKNOWN) ? dip : NULL;
> > +}
> > +
> >   bool
> > -dpif_ipfix_get_tunnel_port(const struct dpif_ipfix *di, odp_port_t odp_port)
> > +dpif_ipfix_is_tunnel_port(const struct dpif_ipfix *di, odp_port_t odp_port)
> >       OVS_EXCLUDED(mutex)
> >   {
> >       struct dpif_ipfix_port *dip;
> >       ovs_mutex_lock(&mutex);
> > -    dip = dpif_ipfix_find_port(di, odp_port);
> > +    dip = dpif_ipfix_find_tunnel_port(di, odp_port);
> >       ovs_mutex_unlock(&mutex);
> >       return dip != NULL;
> >   }
> > @@ -1065,7 +1113,7 @@ dpif_ipfix_create(void)
> >       di = xzalloc(sizeof *di);
> >       dpif_ipfix_bridge_exporter_init(&di->bridge_exporter);
> >       hmap_init(&di->flow_exporter_map);
> > -    hmap_init(&di->tunnel_ports);
> > +    hmap_init(&di->ports);
> >       ovs_refcount_init(&di->ref_cnt);
> >       return di;
> >   }
> > @@ -1159,8 +1207,8 @@ dpif_ipfix_clear(struct dpif_ipfix *di) OVS_REQUIRES(mutex)
> >           free(exp_node);
> >       }
> >
> > -    HMAP_FOR_EACH_SAFE (dip, next, hmap_node, &di->tunnel_ports) {
> > -        dpif_ipfix_del_port(di, dip);
> > +    HMAP_FOR_EACH_SAFE (dip, next, hmap_node, &di->ports) {
> > +        dpif_ipfix_del_port__(di, dip);
> >       }
> >   }
> >
> > @@ -1172,7 +1220,7 @@ dpif_ipfix_unref(struct dpif_ipfix *di) OVS_EXCLUDED(mutex)
> >           dpif_ipfix_clear(di);
> >           dpif_ipfix_bridge_exporter_destroy(&di->bridge_exporter);
> >           hmap_destroy(&di->flow_exporter_map);
> > -        hmap_destroy(&di->tunnel_ports);
> > +        hmap_destroy(&di->ports);
> >           free(di);
> >           ovs_mutex_unlock(&mutex);
> >       }
> > @@ -1211,13 +1259,15 @@ ipfix_send_msg(const struct collectors *collectors, struct dp_packet *msg)
> >
> >   static uint16_t
> >   ipfix_get_template_id(enum ipfix_proto_l2 l2, enum ipfix_proto_l3 l3,
> > -                      enum ipfix_proto_l4 l4, enum ipfix_proto_tunnel tunnel)
> > +                      enum ipfix_proto_l4 l4, enum ipfix_proto_tunnel tunnel,
> > +                      enum ipfix_flow_direction flow_direction)
> >   {
> >       uint16_t template_id;
> >       template_id = l2;
> >       template_id = template_id * NUM_IPFIX_PROTO_L3 + l3;
> >       template_id = template_id * NUM_IPFIX_PROTO_L4 + l4;
> >       template_id = template_id * NUM_IPFIX_PROTO_TUNNEL + tunnel;
> > +    template_id = template_id * NUM_IPFIX_FLOW_DIRECTION + flow_direction;
> >       return IPFIX_TEMPLATE_ID_MIN + template_id;
> >   }
> >
> > @@ -1229,7 +1279,8 @@ ipfix_get_options_template_id(enum ipfix_options_template opt_tmpl_type)
> >       uint16_t max_tmpl_id = ipfix_get_template_id(NUM_IPFIX_PROTO_L2,
> >                                                    NUM_IPFIX_PROTO_L3,
> >                                                    NUM_IPFIX_PROTO_L4,
> > -                                                 NUM_IPFIX_PROTO_TUNNEL);
> > +                                                 NUM_IPFIX_PROTO_TUNNEL,
> > +                                                 NUM_IPFIX_FLOW_DIRECTION);
> >
> >       return max_tmpl_id + opt_tmpl_type;
> >   }
> > @@ -1325,7 +1376,9 @@ ipfix_def_options_template_fields(enum ipfix_options_template opt_tmpl_type,
> >   static uint16_t
> >   ipfix_define_template_fields(enum ipfix_proto_l2 l2, enum ipfix_proto_l3 l3,
> >                                enum ipfix_proto_l4 l4, enum ipfix_proto_tunnel tunnel,
> > -                             bool virtual_obs_id_set, size_t tmpl_hdr_offset,
> > +                             enum ipfix_flow_direction flow_direction,
> > +                             bool virtual_obs_id_set,
> > +                             size_t tmpl_hdr_offset,
> >                                struct dp_packet *msg)
> >   {
> >
> > @@ -1343,6 +1396,19 @@ ipfix_define_template_fields(enum ipfix_proto_l2 l2, enum ipfix_proto_l3 l3,
> >       DEF(ETHERNET_TYPE);
> >       DEF(ETHERNET_HEADER_LENGTH);
> >
> > +    /* Interface Information Elements */
> > +    DEF(INGRESS_INTERFACE);
> > +    DEF(INGRESS_INTERFACE_TYPE);
> > +    DEF(INTERFACE_NAME);
> > +    DEF(INTERFACE_DESCRIPTION);
> > +
> > +    if (flow_direction == EGRESS_FLOW) {
> > +        DEF(EGRESS_INTERFACE);
> > +        DEF(EGRESS_INTERFACE_TYPE);
> > +        DEF(INTERFACE_NAME);
> > +        DEF(INTERFACE_DESCRIPTION);
> > +    }
> > +
> >       if (l2 == IPFIX_PROTO_L2_VLAN) {
> >           DEF(VLAN_ID);
> >           DEF(DOT1Q_VLAN_ID);
> > @@ -1544,6 +1610,24 @@ ipfix_send_options_template_msgs(struct dpif_ipfix_exporter *exporter,
> >   }
> >
> >   static void
> > +ipfix_add_template_record(enum ipfix_proto_l2 l2, enum ipfix_proto_l3 l3,
> > +                          enum ipfix_proto_l4 l4,
> > +                          enum ipfix_proto_tunnel tunnel,
> > +                          enum ipfix_flow_direction flow_direction,
> > +                          bool virtual_obs_id_set,
> > +                          struct dp_packet *msg)
> > +{
> > +    struct ipfix_template_record_header *tmpl_hdr;
> > +    size_t tmpl_hdr_offset = dp_packet_size(msg);
> > +
> > +    tmpl_hdr = dp_packet_put_zeros(msg, sizeof *tmpl_hdr);
> > +    tmpl_hdr->template_id =
> > +        htons(ipfix_get_template_id(l2, l3, l4, tunnel, flow_direction));
> > +    ipfix_define_template_fields(l2, l3, l4, tunnel, flow_direction,
> > +                                 virtual_obs_id_set, tmpl_hdr_offset, msg);
> > +}
> > +
> > +static void
> >   ipfix_send_template_msgs(struct dpif_ipfix_exporter *exporter,
> >                            uint32_t export_time_sec, uint32_t obs_domain_id)
> >   {
> > @@ -1551,14 +1635,14 @@ ipfix_send_template_msgs(struct dpif_ipfix_exporter *exporter,
> >       struct dp_packet msg;
> >       dp_packet_use_stub(&msg, msg_stub, sizeof msg_stub);
> >
> > -    size_t set_hdr_offset, tmpl_hdr_offset, error_pkts;
> > -    struct ipfix_template_record_header *tmpl_hdr;
> > +    size_t set_hdr_offset, error_pkts;
> >       size_t tx_packets = 0;
> >       size_t tx_errors = 0;
> >       enum ipfix_proto_l2 l2;
> >       enum ipfix_proto_l3 l3;
> >       enum ipfix_proto_l4 l4;
> >       enum ipfix_proto_tunnel tunnel;
> > +    enum ipfix_flow_direction flow_direction;
> >
> >       ipfix_init_template_msg(export_time_sec, exporter->seq_number,
> >                               obs_domain_id, IPFIX_SET_ID_TEMPLATE, &msg,
> > @@ -1573,41 +1657,44 @@ ipfix_send_template_msgs(struct dpif_ipfix_exporter *exporter,
> >                       continue;
> >                   }
> >                   for (tunnel = 0; tunnel < NUM_IPFIX_PROTO_TUNNEL; tunnel++) {
> > -                    /* When the size of the template packet reaches
> > -                     * MAX_MESSAGE_LEN(1024), send it out.
> > -                     * And then reinitialize the msg to construct a new
> > -                     * packet for the following templates.
> > -                     */
> > -                    if (dp_packet_size(&msg) >= MAX_MESSAGE_LEN) {
> > -                        /* Send template message. */
> > -                        error_pkts = ipfix_send_template_msg(exporter->collectors,
> > -                                                             &msg, set_hdr_offset);
> > -                        tx_errors += error_pkts;
> > -                        tx_packets += collectors_count(exporter->collectors) - error_pkts;
> > -
> > -                        /* Reinitialize the template msg. */
> > -                        ipfix_init_template_msg(export_time_sec,
> > -                                                exporter->seq_number,
> > -                                                obs_domain_id,
> > -                                                IPFIX_SET_ID_TEMPLATE,
> > -                                                &msg,
> > -                                                &set_hdr_offset);
> > +                    for (flow_direction = 0;
> > +                         flow_direction < NUM_IPFIX_FLOW_DIRECTION;
> > +                         flow_direction++) {
> > +                        /* When the size of the template packet reaches
> > +                         * MAX_MESSAGE_LEN(1024), send it out.
> > +                         * And then reinitialize the msg to construct a new
> > +                         * packet for the following templates.
> > +                         */
> > +                        if (dp_packet_size(&msg) >= MAX_MESSAGE_LEN) {
> > +                            /* Send template message. */
> > +                            error_pkts =
> > +                                ipfix_send_template_msg(exporter->collectors,
> > +                                                        &msg, set_hdr_offset);
> > +                            tx_errors += error_pkts;
> > +                            tx_packets +=
> > +                                collectors_count(exporter->collectors)
> > +                                - error_pkts;
> > +
> > +                            /* Reinitialize the template msg. */
> > +                            ipfix_init_template_msg(export_time_sec,
> > +                                                    exporter->seq_number,
> > +                                                    obs_domain_id,
> > +                                                    IPFIX_SET_ID_TEMPLATE,
> > +                                                    &msg, &set_hdr_offset);
> > +                        }
> > +
> > +                        ipfix_add_template_record(l2, l3, l4, tunnel,
> > +                                flow_direction,
> > +                                exporter->virtual_obs_id != NULL, &msg);
> >                       }
> > -
> > -                    tmpl_hdr_offset = dp_packet_size(&msg);
> > -                    tmpl_hdr = dp_packet_put_zeros(&msg, sizeof *tmpl_hdr);
> > -                    tmpl_hdr->template_id = htons(
> > -                        ipfix_get_template_id(l2, l3, l4, tunnel));
> > -                    ipfix_define_template_fields(
> > -                        l2, l3, l4, tunnel, exporter->virtual_obs_id != NULL,
> > -                        tmpl_hdr_offset, &msg);
> >                   }
> >               }
> >           }
> >       }
> >
> >       /* Send template message. */
> > -    error_pkts = ipfix_send_template_msg(exporter->collectors, &msg, set_hdr_offset);
> > +    error_pkts = ipfix_send_template_msg(exporter->collectors, &msg,
> > +                                         set_hdr_offset);
> >       tx_errors += error_pkts;
> >       tx_packets += collectors_count(exporter->collectors) - error_pkts;
> >
> > @@ -1909,8 +1996,80 @@ ipfix_cache_update(struct dpif_ipfix_exporter *exporter,
> >       }
> >   }
> >
> > +static void
> > +ipfix_destroy_iface_data_record(struct ipfix_data_record_flow_key_iface *data)
> > +{
> > +    free(data->if_descr);
> > +    free(data->if_name);
> > +}
> > +
> > +/* Fills '*data' structure based on port number 'port_no'.  Caller must destroy
> > + * 'data' with ipfix_destroy_iface_data_record(). */
> > +static int
> > +ipfix_get_iface_data_record(const struct dpif_ipfix *di, odp_port_t port_no,
> > +                            struct ipfix_data_record_flow_key_iface *data)
> > +    OVS_REQUIRES(mutex)
> > +{
> > +    struct dpif_ipfix_port *port;
> > +    struct smap netdev_status;
> > +
> > +    port = dpif_ipfix_find_port(di, port_no);
> > +    if (!port) {
> > +        return -1;
> > +    }
> > +
> > +    smap_init(&netdev_status);
> > +    if (!netdev_get_status(port->ofport->netdev, &netdev_status)) {
> > +        data->if_type = htonl(smap_get_ullong(&netdev_status, "if_type", 0));
> > +        data->if_descr = nullable_xstrdup(smap_get(&netdev_status,
> > +                                                   "if_descr"));
> > +    } else {
> > +        data->if_type = 0;
> > +        data->if_descr = NULL;
> > +    }
> > +
> > +    smap_destroy(&netdev_status);
> > +    data->if_index = htonl(port->ifindex);
> > +    data->if_descr_len = data->if_descr ? strlen(data->if_descr) : 0;
> > +    data->if_name = nullable_xstrdup(netdev_get_name(port->ofport->netdev));
> > +    data->if_name_len = data->if_name ? strlen(data->if_name) : 0;
> > +
> > +    return 0;
> > +}
> > +
> > +static void
> > +ipfix_put_iface_data_record(const struct dpif_ipfix *di, odp_port_t port_no,
> > +                            struct dp_packet *msg)
> > +    OVS_REQUIRES(mutex)
> > +{
> > +    struct ipfix_data_record_flow_key_iface data;
> > +    int err;
> > +
> > +    memset(&data, 0, sizeof(struct ipfix_data_record_flow_key_iface));
> > +    err = ipfix_get_iface_data_record(di, port_no, &data);
> > +    if (err == 0) {
> > +        dp_packet_put(msg, &data.if_index, sizeof data.if_index);
> > +        dp_packet_put(msg, &data.if_type, sizeof data.if_type);
> > +        dp_packet_put(msg, &data.if_name_len, sizeof data.if_name_len);
> > +        if (data.if_name_len) {
> > +            dp_packet_put(msg, data.if_name, data.if_name_len);
> > +        }
> > +        dp_packet_put(msg, &data.if_descr_len, sizeof data.if_descr_len);
> > +        if (data.if_descr_len) {
> > +            dp_packet_put(msg, data.if_descr, data.if_descr_len);
> > +        }
> > +        ipfix_destroy_iface_data_record(&data);
> > +    } else {
> > +        dp_packet_put_zeros(msg, sizeof data.if_index);
> > +        dp_packet_put_zeros(msg, sizeof data.if_type);
> > +        dp_packet_put_zeros(msg, sizeof data.if_name_len);
> > +        dp_packet_put_zeros(msg, sizeof data.if_descr_len);
> > +    }
> > +}
> > +
> >   static enum ipfix_sampled_packet_type
> > -ipfix_cache_entry_init(struct ipfix_flow_cache_entry *entry,
> > +ipfix_cache_entry_init(const struct dpif_ipfix *di,
> > +                       struct ipfix_flow_cache_entry *entry,
> >                          const struct dp_packet *packet, const struct flow *flow,
> >                          uint64_t packet_delta_count, uint32_t obs_domain_id,
> >                          uint32_t obs_point_id, odp_port_t output_odp_port,
> > @@ -1919,6 +2078,7 @@ ipfix_cache_entry_init(struct ipfix_flow_cache_entry *entry,
> >                          const struct flow_tnl *tunnel_key,
> >                          struct dpif_ipfix_global_stats *stats,
> >                          const struct dpif_ipfix_actions *ipfix_actions)
> > +    OVS_REQUIRES(mutex)
> >   {
> >       struct ipfix_flow_key *flow_key;
> >       struct dp_packet msg;
> > @@ -1993,8 +2153,14 @@ ipfix_cache_entry_init(struct ipfix_flow_cache_entry *entry,
> >          tunnel = IPFIX_PROTO_TUNNELED;
> >       }
> >
> > +    uint8_t flow_direction =
> > +        (direction == NX_ACTION_SAMPLE_INGRESS ? INGRESS_FLOW
> > +         : direction == NX_ACTION_SAMPLE_EGRESS ? EGRESS_FLOW
> > +         : output_odp_port == ODPP_NONE ? INGRESS_FLOW : EGRESS_FLOW);
> > +
> >       flow_key->obs_domain_id = obs_domain_id;
> > -    flow_key->template_id = ipfix_get_template_id(l2, l3, l4, tunnel);
> > +    flow_key->template_id = ipfix_get_template_id(l2, l3, l4, tunnel,
> > +                                                  flow_direction);
> >
> >       /* The fields defined in the ipfix_data_record_* structs and sent
> >        * below must match exactly the templates defined in
> > @@ -2004,11 +2170,6 @@ ipfix_cache_entry_init(struct ipfix_flow_cache_entry *entry,
> >           ? VLAN_ETH_HEADER_LEN : ETH_HEADER_LEN;
> >       ethernet_total_length = dp_packet_size(packet);
> >
> > -    uint8_t flow_direction =
> > -        (direction == NX_ACTION_SAMPLE_INGRESS ? INGRESS_FLOW
> > -         : direction == NX_ACTION_SAMPLE_EGRESS ? EGRESS_FLOW
> > -         : output_odp_port == ODPP_NONE ? INGRESS_FLOW : EGRESS_FLOW);
> > -
> >       /* Common Ethernet entities. */
> >       {
> >           struct ipfix_data_record_flow_key_common *data_common;
> > @@ -2022,6 +2183,13 @@ ipfix_cache_entry_init(struct ipfix_flow_cache_entry *entry,
> >           data_common->ethernet_header_length = ethernet_header_length;
> >       }
> >
> > +    /* Interface Information Elements */
> > +    ipfix_put_iface_data_record(di, flow->in_port.odp_port, &msg);
> > +
> > +    if (flow_direction == EGRESS_FLOW) {
> > +        ipfix_put_iface_data_record(di, output_odp_port, &msg);
> > +    }
> > +
> >       if (l2 == IPFIX_PROTO_L2_VLAN) {
> >           struct ipfix_data_record_flow_key_vlan *data_vlan;
> >           uint16_t vlan_id = vlan_tci_to_vid(flow->vlans[0].tci);
> > @@ -2469,7 +2637,8 @@ ipfix_send_data_msg(struct dpif_ipfix_exporter *exporter,
> >   }
> >
> >   static void
> > -dpif_ipfix_sample(struct dpif_ipfix_exporter *exporter,
> > +dpif_ipfix_sample(const struct dpif_ipfix *di,
> > +                  struct dpif_ipfix_exporter *exporter,
> >                     const struct dp_packet *packet, const struct flow *flow,
> >                     uint64_t packet_delta_count, uint32_t obs_domain_id,
> >                     uint32_t obs_point_id, odp_port_t output_odp_port,
> > @@ -2477,6 +2646,7 @@ dpif_ipfix_sample(struct dpif_ipfix_exporter *exporter,
> >                     const struct dpif_ipfix_port *tunnel_port,
> >                     const struct flow_tnl *tunnel_key,
> >                     const struct dpif_ipfix_actions *ipfix_actions)
> > +    OVS_REQUIRES(mutex)
> >   {
> >       struct ipfix_flow_cache_entry *entry;
> >       enum ipfix_sampled_packet_type sampled_packet_type;
> > @@ -2484,7 +2654,7 @@ dpif_ipfix_sample(struct dpif_ipfix_exporter *exporter,
> >       /* Create a flow cache entry from the sample. */
> >       entry = xmalloc(sizeof *entry);
> >       sampled_packet_type =
> > -            ipfix_cache_entry_init(entry, packet,
> > +            ipfix_cache_entry_init(di, entry, packet,
> >                                      flow, packet_delta_count,
> >                                      obs_domain_id, obs_point_id,
> >                                      output_odp_port, direction,
> > @@ -2542,16 +2712,16 @@ dpif_ipfix_bridge_sample(struct dpif_ipfix *di, const struct dp_packet *packet,
> >           if (output_odp_port == ODPP_NONE && flow->tunnel.ip_dst) {
> >               /* Input tunnel. */
> >               tunnel_key = &flow->tunnel;
> > -            tunnel_port = dpif_ipfix_find_port(di, input_odp_port);
> > +            tunnel_port = dpif_ipfix_find_tunnel_port(di, input_odp_port);
> >           }
> >           if (output_odp_port != ODPP_NONE && output_tunnel_key) {
> >               /* Output tunnel, output_tunnel_key must be valid. */
> >               tunnel_key = output_tunnel_key;
> > -            tunnel_port = dpif_ipfix_find_port(di, output_odp_port);
> > +            tunnel_port = dpif_ipfix_find_tunnel_port(di, output_odp_port);
> >           }
> >       }
> >
> > -    dpif_ipfix_sample(&di->bridge_exporter.exporter, packet, flow,
> > +    dpif_ipfix_sample(di, &di->bridge_exporter.exporter, packet, flow,
> >                         packet_delta_count,
> >                         di->bridge_exporter.options->obs_domain_id,
> >                         di->bridge_exporter.options->obs_point_id,
> > @@ -2587,16 +2757,16 @@ dpif_ipfix_flow_sample(struct dpif_ipfix *di, const struct dp_packet *packet,
> >               if (output_odp_port == ODPP_NONE && flow->tunnel.ip_dst) {
> >                   /* Input tunnel. */
> >                   tunnel_key = &flow->tunnel;
> > -                tunnel_port = dpif_ipfix_find_port(di, input_odp_port);
> > +                tunnel_port = dpif_ipfix_find_tunnel_port(di, input_odp_port);
> >               }
> >               if (output_odp_port != ODPP_NONE && output_tunnel_key) {
> >                   /* Output tunnel, output_tunnel_key must be valid. */
> >                   tunnel_key = output_tunnel_key;
> > -                tunnel_port = dpif_ipfix_find_port(di, output_odp_port);
> > +                tunnel_port = dpif_ipfix_find_tunnel_port(di, output_odp_port);
> >               }
> >           }
> >
> > -        dpif_ipfix_sample(&node->exporter.exporter, packet, flow,
> > +        dpif_ipfix_sample(di, &node->exporter.exporter, packet, flow,
> >                             packet_delta_count,
> >                             cookie->flow_sample.obs_domain_id,
> >                             cookie->flow_sample.obs_point_id,
> > diff --git a/ofproto/ofproto-dpif-ipfix.h b/ofproto/ofproto-dpif-ipfix.h
> > index f91d041..1309da1 100644
> > --- a/ofproto/ofproto-dpif-ipfix.h
> > +++ b/ofproto/ofproto-dpif-ipfix.h
> > @@ -38,8 +38,8 @@ struct dpif_ipfix *dpif_ipfix_create(void);
> >   struct dpif_ipfix *dpif_ipfix_ref(const struct dpif_ipfix *);
> >   void dpif_ipfix_unref(struct dpif_ipfix *);
> >
> > -void dpif_ipfix_add_tunnel_port(struct dpif_ipfix *, struct ofport *, odp_port_t);
> > -void dpif_ipfix_del_tunnel_port(struct dpif_ipfix *, odp_port_t);
> > +void dpif_ipfix_add_port(struct dpif_ipfix *, struct ofport *, odp_port_t);
> > +void dpif_ipfix_del_port(struct dpif_ipfix *, odp_port_t);
> >
> >   uint32_t dpif_ipfix_get_bridge_exporter_probability(const struct dpif_ipfix *);
> >   bool dpif_ipfix_get_bridge_exporter_tunnel_sampling(const struct dpif_ipfix *);
> > @@ -47,7 +47,7 @@ bool dpif_ipfix_get_bridge_exporter_input_sampling(const struct dpif_ipfix *);
> >   bool dpif_ipfix_get_bridge_exporter_output_sampling(const struct dpif_ipfix *);
> >   bool dpif_ipfix_get_flow_exporter_tunnel_sampling(const struct dpif_ipfix *,
> >                                                     const uint32_t);
> > -bool dpif_ipfix_get_tunnel_port(const struct dpif_ipfix *, odp_port_t);
> > +bool dpif_ipfix_is_tunnel_port(const struct dpif_ipfix *, odp_port_t);
> >   void dpif_ipfix_set_options(
> >       struct dpif_ipfix *,
> >       const struct ofproto_ipfix_bridge_exporter_options *,
> > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> > index 9e1f837..aac135f 100644
> > --- a/ofproto/ofproto-dpif-xlate.c
> > +++ b/ofproto/ofproto-dpif-xlate.c
> > @@ -2952,7 +2952,7 @@ compose_ipfix_action(struct xlate_ctx *ctx, odp_port_t output_odp_port)
> >            * OVS_USERSPACE_ATTR_TUNNEL_OUT_PORT
> >            */
> >           if (dpif_ipfix_get_bridge_exporter_tunnel_sampling(ipfix) &&
> > -            dpif_ipfix_get_tunnel_port(ipfix, output_odp_port) ) {
> > +            dpif_ipfix_is_tunnel_port(ipfix, output_odp_port) ) {
> >              tunnel_out_port = output_odp_port;
> >           }
> >       }
> > @@ -5211,7 +5211,7 @@ xlate_sample_action(struct xlate_ctx *ctx,
> >
> >           if (dpif_ipfix_get_flow_exporter_tunnel_sampling(ipfix,
> >                                                            os->collector_set_id)
> > -            && dpif_ipfix_get_tunnel_port(ipfix, output_odp_port)) {
> > +            && dpif_ipfix_is_tunnel_port(ipfix, output_odp_port)) {
> >               tunnel_out_port = output_odp_port;
> >               emit_set_tunnel = true;
> >           }
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index 1a8e829..f9c8749 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -1872,9 +1872,6 @@ port_construct(struct ofport *port_)
> >           }
> >
> >           port->is_tunnel = true;
> > -        if (ofproto->ipfix) {
> > -           dpif_ipfix_add_tunnel_port(ofproto->ipfix, port_, port->odp_port);
> > -        }
> >       } else {
> >           /* Sanity-check that a mapping doesn't already exist.  This
> >            * shouldn't happen for non-tunnel ports. */
> > @@ -1895,6 +1892,9 @@ port_construct(struct ofport *port_)
> >       if (ofproto->sflow) {
> >           dpif_sflow_add_port(ofproto->sflow, port_, port->odp_port);
> >       }
> > +    if (ofproto->ipfix) {
> > +       dpif_ipfix_add_port(ofproto->ipfix, port_, port->odp_port);
> > +    }
> >
> >       return 0;
> >   }
> > @@ -1940,10 +1940,6 @@ port_destruct(struct ofport *port_, bool del)
> >           atomic_count_dec(&ofproto->backer->tnl_count);
> >       }
> >
> > -    if (port->is_tunnel && ofproto->ipfix) {
> > -       dpif_ipfix_del_tunnel_port(ofproto->ipfix, port->odp_port);
> > -    }
> > -
> >       tnl_port_del(port);
> >       sset_find_and_delete(&ofproto->ports, devname);
> >       sset_find_and_delete(&ofproto->ghost_ports, devname);
> > @@ -1958,6 +1954,9 @@ port_destruct(struct ofport *port_, bool del)
> >       if (ofproto->sflow) {
> >           dpif_sflow_del_port(ofproto->sflow, port->odp_port);
> >       }
> > +    if (ofproto->ipfix) {
> > +       dpif_ipfix_del_port(ofproto->ipfix, port->odp_port);
> > +    }
> >
> >       free(port->qdscp);
> >   }
> > @@ -2083,13 +2082,11 @@ set_ipfix(
> >               di, bridge_exporter_options, flow_exporters_options,
> >               n_flow_exporters_options);
> >
> > -        /* Add tunnel ports only when a new ipfix created */
> > +        /* Add ports only when a new ipfix created */
> >           if (new_di == true) {
> >               struct ofport_dpif *ofport;
> >               HMAP_FOR_EACH (ofport, up.hmap_node, &ofproto->up.ports) {
> > -                if (ofport->is_tunnel == true) {
> > -                    dpif_ipfix_add_tunnel_port(di, &ofport->up, ofport->odp_port);
> > -                }
> > +                dpif_ipfix_add_port(di, &ofport->up, ofport->odp_port);
> >               }
> >           }
> >
> >
Weglicki, MichalX Oct. 12, 2017, 8:02 a.m. | #3
Hello, 

Are there are any other comments? 

Thank you all in advance. 

Br, 
Michal. 

> -----Original Message-----
> From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-bounces@openvswitch.org] On Behalf Of Weglicki, MichalX
> Sent: Wednesday, October 4, 2017 10:19 AM
> To: Greg Rose <gvrose8192@gmail.com>; dev@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH v5 2/2] ofproto-dpif-ipfix: add interface Information Elements to flow key
> 
> > -----Original Message-----
> > From: Greg Rose [mailto:gvrose8192@gmail.com]
> > Sent: Wednesday, October 4, 2017 12:21 AM
> > To: Weglicki, MichalX <michalx.weglicki@intel.com>; dev@openvswitch.org
> > Subject: Re: [ovs-dev] [PATCH v5 2/2] ofproto-dpif-ipfix: add interface Information Elements to flow key
> >
> > On 10/02/2017 07:49 AM, Michal Weglicki wrote:
> > > Extend flow key part of data record to include following Information Elements:
> > > - ingressInterface
> > > - ingressInterfaceType
> > > - egressInterface
> > > - egressInterfaceType
> > > - interfaceName
> > > - interfaceDescription
> > >
> > > In case of input sampling we don't have information about egress port.
> > > Define templates depending not only on protocol types, but also on flow
> > > direction. Only egress flow will include egress information elements.
> > >
> > > With this change, dpif_ipfix_exporter stores every port in hmap rather
> > > than only tunnel ports. It allows to easily retrieve required
> > > information about interfaces during sampling upcalls.
> > >
> > > v1->v2
> > > * Add interfaceType and interfaceDescription
> > > * Rework ipfix_get_iface_data_record function
> > > v2->v3: Code rebase.
> > > v3->v4: Minor comments applied.
> > > v4->v5: Clang complation problem fix.
> > >
> > > Co-authored-by: Michal Weglicki <michalx.weglicki@intel.com>
> > > Signed-off-by: Michal Weglicki <michalx.weglicki@intel.com>
> > > Signed-off-by: Przemyslaw Szczerbik <przemyslawx.szczerbik@intel.com>
> >
> > Michal and Przemyslaw,
> >
> > There is one more small nit pick that I came across noted below.
> >
> > Other than that....
> >
> > Tested-by: Greg Rose <gvrose8192@gmail.com>
> > Reviewed-by: Greg Rose <gvrose8192@gmail.com>
> >
> > > ---
> > >   ofproto/ofproto-dpif-ipfix.c | 358 +++++++++++++++++++++++++++++++------------
> > >   ofproto/ofproto-dpif-ipfix.h |   6 +-
> > >   ofproto/ofproto-dpif-xlate.c |   4 +-
> > >   ofproto/ofproto-dpif.c       |  19 +--
> > >   4 files changed, 277 insertions(+), 110 deletions(-)
> > >
> > > diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
> > > index 472c272..138c325 100644
> > > --- a/ofproto/ofproto-dpif-ipfix.c
> > > +++ b/ofproto/ofproto-dpif-ipfix.c
> > > @@ -115,11 +115,12 @@ struct dpif_ipfix_global_stats {
> > >   };
> > >
> > >   struct dpif_ipfix_port {
> > > -    struct hmap_node hmap_node; /* In struct dpif_ipfix's "tunnel_ports" hmap. */
> > > +    struct hmap_node hmap_node; /* In struct dpif_ipfix's "ports" hmap. */
> > >       struct ofport *ofport;      /* To retrieve port stats. */
> > >       odp_port_t odp_port;
> > >       enum dpif_ipfix_tunnel_type tunnel_type;
> > >       uint8_t tunnel_key_length;
> > > +    uint32_t ifindex;
> > >   };
> > >
> > >   struct dpif_ipfix_exporter {
> > > @@ -157,9 +158,9 @@ struct dpif_ipfix_flow_exporter_map_node {
> > >   struct dpif_ipfix {
> > >       struct dpif_ipfix_bridge_exporter bridge_exporter;
> > >       struct hmap flow_exporter_map;  /* dpif_ipfix_flow_exporter_map_node. */
> > > -    struct hmap tunnel_ports;       /* Contains "struct dpif_ipfix_port"s.
> > > -                                     * It makes tunnel port lookups faster in
> > > -                                     * sampling upcalls. */
> > > +    struct hmap ports;              /* Contains "struct dpif_ipfix_port"s.
> > > +                                     * It makes port lookups faster in sampling
> > > +                                     * upcalls. */
> > >       struct ovs_refcount ref_cnt;
> > >   };
> > >
> > > @@ -293,7 +294,8 @@ BUILD_ASSERT_DECL(sizeof(struct ipfix_template_field_specifier) == 8);
> > >   /* Cf. IETF RFC 5102 Section 5.11.6. */
> > >   enum ipfix_flow_direction {
> > >       INGRESS_FLOW = 0x00,
> > > -    EGRESS_FLOW = 0x01
> > > +    EGRESS_FLOW = 0x01,
> > > +    NUM_IPFIX_FLOW_DIRECTION
> > >   };
> > >
> > >   /* Part of data record flow key for common metadata and Ethernet entities. */
> > > @@ -308,6 +310,18 @@ struct ipfix_data_record_flow_key_common {
> > >   });
> > >   BUILD_ASSERT_DECL(sizeof(struct ipfix_data_record_flow_key_common) == 20);
> > >
> > > +/* Part of data record flow key for interface information. Since some of the
> > > + * elements have variable length, members of this structure should be appended
> > > + * to the 'struct dp_packet' one by one. */
> > > +struct ipfix_data_record_flow_key_iface {
> > > +    ovs_be32 if_index;     /* (INGRESS | EGRESS)_INTERFACE */
> > > +    ovs_be32 if_type;     /* (INGRESS | EGRESS)_INTERFACE_TYPE */
> > > +    uint8_t if_name_len;   /* Variable length element: INTERFACE_NAME */
> > > +    char *if_name;
> > > +    uint8_t if_descr_len; /* Variable length element: INTERFACE_DESCRIPTION */
> > > +    char *if_descr;
> > > +};
> > > +
> >
> > I think we can close some holes in this structure by placing the two uint8_t members
> > beside each other.
> >
> > Like this maybe?
> >
> >  > +struct ipfix_data_record_flow_key_iface {
> >  > +    ovs_be32 if_index;     /* (INGRESS | EGRESS)_INTERFACE */
> >  > +    ovs_be32 if_type;     /* (INGRESS | EGRESS)_INTERFACE_TYPE */
> >  > +    char *if_name;
> >  > +    char *if_descr;
> >  > +    uint8_t if_name_len;   /* Variable length element: INTERFACE_NAME */
> >  > +    uint8_t if_descr_len; /* Variable length element: INTERFACE_DESCRIPTION */
> >  > +};
> I really think that keeping each pointer with length value makes more sense,
> As those values are strictly connected to each other. Keeping it that way
> make it visible that those values are describing same buffer.
> 
> >
> > >   /* Part of data record flow key for VLAN entities. */
> > >   OVS_PACKED(
> > >   struct ipfix_data_record_flow_key_vlan {
> > > @@ -745,7 +759,7 @@ dpif_ipfix_find_port(const struct dpif_ipfix *di,
> > >       struct dpif_ipfix_port *dip;
> > >
> > >       HMAP_FOR_EACH_IN_BUCKET (dip, hmap_node, hash_odp_port(odp_port),
> > > -                             &di->tunnel_ports) {
> > > +                             &di->ports) {
> > >           if (dip->odp_port == odp_port) {
> > >               return dip;
> > >           }
> > > @@ -754,82 +768,116 @@ dpif_ipfix_find_port(const struct dpif_ipfix *di,
> > >   }
> > >
> > >   static void
> > > -dpif_ipfix_del_port(struct dpif_ipfix *di,
> > > +dpif_ipfix_del_port__(struct dpif_ipfix *di,
> > >                         struct dpif_ipfix_port *dip)
> > >       OVS_REQUIRES(mutex)
> > >   {
> > > -    hmap_remove(&di->tunnel_ports, &dip->hmap_node);
> > > +    hmap_remove(&di->ports, &dip->hmap_node);
> > >       free(dip);
> > >   }
> > >
> > > +static enum dpif_ipfix_tunnel_type
> > > +dpif_ipfix_tunnel_type(const struct ofport *ofport)
> > > +{
> > > +    const char *type = netdev_get_type(ofport->netdev);
> > > +
> > > +    if (type == NULL) {
> > > +        return DPIF_IPFIX_TUNNEL_UNKNOWN;
> > > +    }
> > > +    if (strcmp(type, "gre") == 0) {
> > > +        return DPIF_IPFIX_TUNNEL_GRE;
> > > +    } else if (strcmp(type, "vxlan") == 0) {
> > > +        return DPIF_IPFIX_TUNNEL_VXLAN;
> > > +    } else if (strcmp(type, "lisp") == 0) {
> > > +        return DPIF_IPFIX_TUNNEL_LISP;
> > > +    } else if (strcmp(type, "geneve") == 0) {
> > > +        return DPIF_IPFIX_TUNNEL_GENEVE;
> > > +    } else if (strcmp(type, "stt") == 0) {
> > > +        return DPIF_IPFIX_TUNNEL_STT;
> > > +    }
> > > +
> > > +    return DPIF_IPFIX_TUNNEL_UNKNOWN;
> > > +}
> > > +
> > > +static uint8_t
> > > +dpif_ipfix_tunnel_key_length(enum dpif_ipfix_tunnel_type tunnel_type)
> > > +{
> > > +
> > > +    switch (tunnel_type) {
> > > +        case DPIF_IPFIX_TUNNEL_GRE:
> > > +            /* 32-bit key gre */
> > > +            return 4;
> > > +        case DPIF_IPFIX_TUNNEL_VXLAN:
> > > +        case DPIF_IPFIX_TUNNEL_LISP:
> > > +        case DPIF_IPFIX_TUNNEL_GENEVE:
> > > +            return 3;
> > > +        case DPIF_IPFIX_TUNNEL_STT:
> > > +            return 8;
> > > +        case DPIF_IPFIX_TUNNEL_UNKNOWN:
> > > +        case NUM_DPIF_IPFIX_TUNNEL:
> > > +        default:
> > > +            return 0;
> > > +    }
> > > +}
> > > +
> > >   void
> > > -dpif_ipfix_add_tunnel_port(struct dpif_ipfix *di, struct ofport *ofport,
> > > -                           odp_port_t odp_port) OVS_EXCLUDED(mutex)
> > > +dpif_ipfix_add_port(struct dpif_ipfix *di, struct ofport *ofport,
> > > +                    odp_port_t odp_port) OVS_EXCLUDED(mutex)
> > >   {
> > >       struct dpif_ipfix_port *dip;
> > > -    const char *type;
> > > +    int64_t ifindex;
> > >
> > >       ovs_mutex_lock(&mutex);
> > >       dip = dpif_ipfix_find_port(di, odp_port);
> > >       if (dip) {
> > > -        dpif_ipfix_del_port(di, dip);
> > > +        dpif_ipfix_del_port__(di, dip);
> > >       }
> > >
> > > -    type = netdev_get_type(ofport->netdev);
> > > -    if (type == NULL) {
> > > -        goto out;
> > > +    ifindex = netdev_get_ifindex(ofport->netdev);
> > > +    if (ifindex < 0) {
> > > +        ifindex = 0;
> > >       }
> > >
> > > -    /* Add to table of tunnel ports. */
> > > +    /* Add to table of ports. */
> > >       dip = xmalloc(sizeof *dip);
> > >       dip->ofport = ofport;
> > >       dip->odp_port = odp_port;
> > > -    if (strcmp(type, "gre") == 0) {
> > > -        /* 32-bit key gre */
> > > -        dip->tunnel_type = DPIF_IPFIX_TUNNEL_GRE;
> > > -        dip->tunnel_key_length = 4;
> > > -    } else if (strcmp(type, "vxlan") == 0) {
> > > -        dip->tunnel_type = DPIF_IPFIX_TUNNEL_VXLAN;
> > > -        dip->tunnel_key_length = 3;
> > > -    } else if (strcmp(type, "lisp") == 0) {
> > > -        dip->tunnel_type = DPIF_IPFIX_TUNNEL_LISP;
> > > -        dip->tunnel_key_length = 3;
> > > -    } else if (strcmp(type, "geneve") == 0) {
> > > -        dip->tunnel_type = DPIF_IPFIX_TUNNEL_GENEVE;
> > > -        dip->tunnel_key_length = 3;
> > > -    } else if (strcmp(type, "stt") == 0) {
> > > -        dip->tunnel_type = DPIF_IPFIX_TUNNEL_STT;
> > > -        dip->tunnel_key_length = 8;
> > > -    } else {
> > > -        free(dip);
> > > -        goto out;
> > > -    }
> > > -    hmap_insert(&di->tunnel_ports, &dip->hmap_node, hash_odp_port(odp_port));
> > > +    dip->tunnel_type = dpif_ipfix_tunnel_type(ofport);
> > > +    dip->tunnel_key_length = dpif_ipfix_tunnel_key_length(dip->tunnel_type);
> > > +    dip->ifindex = ifindex;
> > > +    hmap_insert(&di->ports, &dip->hmap_node, hash_odp_port(odp_port));
> > >
> > > -out:
> > >       ovs_mutex_unlock(&mutex);
> > >   }
> > >
> > >   void
> > > -dpif_ipfix_del_tunnel_port(struct dpif_ipfix *di, odp_port_t odp_port)
> > > +dpif_ipfix_del_port(struct dpif_ipfix *di, odp_port_t odp_port)
> > >       OVS_EXCLUDED(mutex)
> > >   {
> > >       struct dpif_ipfix_port *dip;
> > >       ovs_mutex_lock(&mutex);
> > >       dip = dpif_ipfix_find_port(di, odp_port);
> > >       if (dip) {
> > > -        dpif_ipfix_del_port(di, dip);
> > > +        dpif_ipfix_del_port__(di, dip);
> > >       }
> > >       ovs_mutex_unlock(&mutex);
> > >   }
> > >
> > > +static struct dpif_ipfix_port *
> > > +dpif_ipfix_find_tunnel_port(const struct dpif_ipfix *di, odp_port_t odp_port)
> > > +    OVS_REQUIRES(mutex)
> > > +{
> > > +    struct dpif_ipfix_port *dip = dpif_ipfix_find_port(di, odp_port);
> > > +    return (dip && dip->tunnel_type != DPIF_IPFIX_TUNNEL_UNKNOWN) ? dip : NULL;
> > > +}
> > > +
> > >   bool
> > > -dpif_ipfix_get_tunnel_port(const struct dpif_ipfix *di, odp_port_t odp_port)
> > > +dpif_ipfix_is_tunnel_port(const struct dpif_ipfix *di, odp_port_t odp_port)
> > >       OVS_EXCLUDED(mutex)
> > >   {
> > >       struct dpif_ipfix_port *dip;
> > >       ovs_mutex_lock(&mutex);
> > > -    dip = dpif_ipfix_find_port(di, odp_port);
> > > +    dip = dpif_ipfix_find_tunnel_port(di, odp_port);
> > >       ovs_mutex_unlock(&mutex);
> > >       return dip != NULL;
> > >   }
> > > @@ -1065,7 +1113,7 @@ dpif_ipfix_create(void)
> > >       di = xzalloc(sizeof *di);
> > >       dpif_ipfix_bridge_exporter_init(&di->bridge_exporter);
> > >       hmap_init(&di->flow_exporter_map);
> > > -    hmap_init(&di->tunnel_ports);
> > > +    hmap_init(&di->ports);
> > >       ovs_refcount_init(&di->ref_cnt);
> > >       return di;
> > >   }
> > > @@ -1159,8 +1207,8 @@ dpif_ipfix_clear(struct dpif_ipfix *di) OVS_REQUIRES(mutex)
> > >           free(exp_node);
> > >       }
> > >
> > > -    HMAP_FOR_EACH_SAFE (dip, next, hmap_node, &di->tunnel_ports) {
> > > -        dpif_ipfix_del_port(di, dip);
> > > +    HMAP_FOR_EACH_SAFE (dip, next, hmap_node, &di->ports) {
> > > +        dpif_ipfix_del_port__(di, dip);
> > >       }
> > >   }
> > >
> > > @@ -1172,7 +1220,7 @@ dpif_ipfix_unref(struct dpif_ipfix *di) OVS_EXCLUDED(mutex)
> > >           dpif_ipfix_clear(di);
> > >           dpif_ipfix_bridge_exporter_destroy(&di->bridge_exporter);
> > >           hmap_destroy(&di->flow_exporter_map);
> > > -        hmap_destroy(&di->tunnel_ports);
> > > +        hmap_destroy(&di->ports);
> > >           free(di);
> > >           ovs_mutex_unlock(&mutex);
> > >       }
> > > @@ -1211,13 +1259,15 @@ ipfix_send_msg(const struct collectors *collectors, struct dp_packet *msg)
> > >
> > >   static uint16_t
> > >   ipfix_get_template_id(enum ipfix_proto_l2 l2, enum ipfix_proto_l3 l3,
> > > -                      enum ipfix_proto_l4 l4, enum ipfix_proto_tunnel tunnel)
> > > +                      enum ipfix_proto_l4 l4, enum ipfix_proto_tunnel tunnel,
> > > +                      enum ipfix_flow_direction flow_direction)
> > >   {
> > >       uint16_t template_id;
> > >       template_id = l2;
> > >       template_id = template_id * NUM_IPFIX_PROTO_L3 + l3;
> > >       template_id = template_id * NUM_IPFIX_PROTO_L4 + l4;
> > >       template_id = template_id * NUM_IPFIX_PROTO_TUNNEL + tunnel;
> > > +    template_id = template_id * NUM_IPFIX_FLOW_DIRECTION + flow_direction;
> > >       return IPFIX_TEMPLATE_ID_MIN + template_id;
> > >   }
> > >
> > > @@ -1229,7 +1279,8 @@ ipfix_get_options_template_id(enum ipfix_options_template opt_tmpl_type)
> > >       uint16_t max_tmpl_id = ipfix_get_template_id(NUM_IPFIX_PROTO_L2,
> > >                                                    NUM_IPFIX_PROTO_L3,
> > >                                                    NUM_IPFIX_PROTO_L4,
> > > -                                                 NUM_IPFIX_PROTO_TUNNEL);
> > > +                                                 NUM_IPFIX_PROTO_TUNNEL,
> > > +                                                 NUM_IPFIX_FLOW_DIRECTION);
> > >
> > >       return max_tmpl_id + opt_tmpl_type;
> > >   }
> > > @@ -1325,7 +1376,9 @@ ipfix_def_options_template_fields(enum ipfix_options_template opt_tmpl_type,
> > >   static uint16_t
> > >   ipfix_define_template_fields(enum ipfix_proto_l2 l2, enum ipfix_proto_l3 l3,
> > >                                enum ipfix_proto_l4 l4, enum ipfix_proto_tunnel tunnel,
> > > -                             bool virtual_obs_id_set, size_t tmpl_hdr_offset,
> > > +                             enum ipfix_flow_direction flow_direction,
> > > +                             bool virtual_obs_id_set,
> > > +                             size_t tmpl_hdr_offset,
> > >                                struct dp_packet *msg)
> > >   {
> > >
> > > @@ -1343,6 +1396,19 @@ ipfix_define_template_fields(enum ipfix_proto_l2 l2, enum ipfix_proto_l3 l3,
> > >       DEF(ETHERNET_TYPE);
> > >       DEF(ETHERNET_HEADER_LENGTH);
> > >
> > > +    /* Interface Information Elements */
> > > +    DEF(INGRESS_INTERFACE);
> > > +    DEF(INGRESS_INTERFACE_TYPE);
> > > +    DEF(INTERFACE_NAME);
> > > +    DEF(INTERFACE_DESCRIPTION);
> > > +
> > > +    if (flow_direction == EGRESS_FLOW) {
> > > +        DEF(EGRESS_INTERFACE);
> > > +        DEF(EGRESS_INTERFACE_TYPE);
> > > +        DEF(INTERFACE_NAME);
> > > +        DEF(INTERFACE_DESCRIPTION);
> > > +    }
> > > +
> > >       if (l2 == IPFIX_PROTO_L2_VLAN) {
> > >           DEF(VLAN_ID);
> > >           DEF(DOT1Q_VLAN_ID);
> > > @@ -1544,6 +1610,24 @@ ipfix_send_options_template_msgs(struct dpif_ipfix_exporter *exporter,
> > >   }
> > >
> > >   static void
> > > +ipfix_add_template_record(enum ipfix_proto_l2 l2, enum ipfix_proto_l3 l3,
> > > +                          enum ipfix_proto_l4 l4,
> > > +                          enum ipfix_proto_tunnel tunnel,
> > > +                          enum ipfix_flow_direction flow_direction,
> > > +                          bool virtual_obs_id_set,
> > > +                          struct dp_packet *msg)
> > > +{
> > > +    struct ipfix_template_record_header *tmpl_hdr;
> > > +    size_t tmpl_hdr_offset = dp_packet_size(msg);
> > > +
> > > +    tmpl_hdr = dp_packet_put_zeros(msg, sizeof *tmpl_hdr);
> > > +    tmpl_hdr->template_id =
> > > +        htons(ipfix_get_template_id(l2, l3, l4, tunnel, flow_direction));
> > > +    ipfix_define_template_fields(l2, l3, l4, tunnel, flow_direction,
> > > +                                 virtual_obs_id_set, tmpl_hdr_offset, msg);
> > > +}
> > > +
> > > +static void
> > >   ipfix_send_template_msgs(struct dpif_ipfix_exporter *exporter,
> > >                            uint32_t export_time_sec, uint32_t obs_domain_id)
> > >   {
> > > @@ -1551,14 +1635,14 @@ ipfix_send_template_msgs(struct dpif_ipfix_exporter *exporter,
> > >       struct dp_packet msg;
> > >       dp_packet_use_stub(&msg, msg_stub, sizeof msg_stub);
> > >
> > > -    size_t set_hdr_offset, tmpl_hdr_offset, error_pkts;
> > > -    struct ipfix_template_record_header *tmpl_hdr;
> > > +    size_t set_hdr_offset, error_pkts;
> > >       size_t tx_packets = 0;
> > >       size_t tx_errors = 0;
> > >       enum ipfix_proto_l2 l2;
> > >       enum ipfix_proto_l3 l3;
> > >       enum ipfix_proto_l4 l4;
> > >       enum ipfix_proto_tunnel tunnel;
> > > +    enum ipfix_flow_direction flow_direction;
> > >
> > >       ipfix_init_template_msg(export_time_sec, exporter->seq_number,
> > >                               obs_domain_id, IPFIX_SET_ID_TEMPLATE, &msg,
> > > @@ -1573,41 +1657,44 @@ ipfix_send_template_msgs(struct dpif_ipfix_exporter *exporter,
> > >                       continue;
> > >                   }
> > >                   for (tunnel = 0; tunnel < NUM_IPFIX_PROTO_TUNNEL; tunnel++) {
> > > -                    /* When the size of the template packet reaches
> > > -                     * MAX_MESSAGE_LEN(1024), send it out.
> > > -                     * And then reinitialize the msg to construct a new
> > > -                     * packet for the following templates.
> > > -                     */
> > > -                    if (dp_packet_size(&msg) >= MAX_MESSAGE_LEN) {
> > > -                        /* Send template message. */
> > > -                        error_pkts = ipfix_send_template_msg(exporter->collectors,
> > > -                                                             &msg, set_hdr_offset);
> > > -                        tx_errors += error_pkts;
> > > -                        tx_packets += collectors_count(exporter->collectors) - error_pkts;
> > > -
> > > -                        /* Reinitialize the template msg. */
> > > -                        ipfix_init_template_msg(export_time_sec,
> > > -                                                exporter->seq_number,
> > > -                                                obs_domain_id,
> > > -                                                IPFIX_SET_ID_TEMPLATE,
> > > -                                                &msg,
> > > -                                                &set_hdr_offset);
> > > +                    for (flow_direction = 0;
> > > +                         flow_direction < NUM_IPFIX_FLOW_DIRECTION;
> > > +                         flow_direction++) {
> > > +                        /* When the size of the template packet reaches
> > > +                         * MAX_MESSAGE_LEN(1024), send it out.
> > > +                         * And then reinitialize the msg to construct a new
> > > +                         * packet for the following templates.
> > > +                         */
> > > +                        if (dp_packet_size(&msg) >= MAX_MESSAGE_LEN) {
> > > +                            /* Send template message. */
> > > +                            error_pkts =
> > > +                                ipfix_send_template_msg(exporter->collectors,
> > > +                                                        &msg, set_hdr_offset);
> > > +                            tx_errors += error_pkts;
> > > +                            tx_packets +=
> > > +                                collectors_count(exporter->collectors)
> > > +                                - error_pkts;
> > > +
> > > +                            /* Reinitialize the template msg. */
> > > +                            ipfix_init_template_msg(export_time_sec,
> > > +                                                    exporter->seq_number,
> > > +                                                    obs_domain_id,
> > > +                                                    IPFIX_SET_ID_TEMPLATE,
> > > +                                                    &msg, &set_hdr_offset);
> > > +                        }
> > > +
> > > +                        ipfix_add_template_record(l2, l3, l4, tunnel,
> > > +                                flow_direction,
> > > +                                exporter->virtual_obs_id != NULL, &msg);
> > >                       }
> > > -
> > > -                    tmpl_hdr_offset = dp_packet_size(&msg);
> > > -                    tmpl_hdr = dp_packet_put_zeros(&msg, sizeof *tmpl_hdr);
> > > -                    tmpl_hdr->template_id = htons(
> > > -                        ipfix_get_template_id(l2, l3, l4, tunnel));
> > > -                    ipfix_define_template_fields(
> > > -                        l2, l3, l4, tunnel, exporter->virtual_obs_id != NULL,
> > > -                        tmpl_hdr_offset, &msg);
> > >                   }
> > >               }
> > >           }
> > >       }
> > >
> > >       /* Send template message. */
> > > -    error_pkts = ipfix_send_template_msg(exporter->collectors, &msg, set_hdr_offset);
> > > +    error_pkts = ipfix_send_template_msg(exporter->collectors, &msg,
> > > +                                         set_hdr_offset);
> > >       tx_errors += error_pkts;
> > >       tx_packets += collectors_count(exporter->collectors) - error_pkts;
> > >
> > > @@ -1909,8 +1996,80 @@ ipfix_cache_update(struct dpif_ipfix_exporter *exporter,
> > >       }
> > >   }
> > >
> > > +static void
> > > +ipfix_destroy_iface_data_record(struct ipfix_data_record_flow_key_iface *data)
> > > +{
> > > +    free(data->if_descr);
> > > +    free(data->if_name);
> > > +}
> > > +
> > > +/* Fills '*data' structure based on port number 'port_no'.  Caller must destroy
> > > + * 'data' with ipfix_destroy_iface_data_record(). */
> > > +static int
> > > +ipfix_get_iface_data_record(const struct dpif_ipfix *di, odp_port_t port_no,
> > > +                            struct ipfix_data_record_flow_key_iface *data)
> > > +    OVS_REQUIRES(mutex)
> > > +{
> > > +    struct dpif_ipfix_port *port;
> > > +    struct smap netdev_status;
> > > +
> > > +    port = dpif_ipfix_find_port(di, port_no);
> > > +    if (!port) {
> > > +        return -1;
> > > +    }
> > > +
> > > +    smap_init(&netdev_status);
> > > +    if (!netdev_get_status(port->ofport->netdev, &netdev_status)) {
> > > +        data->if_type = htonl(smap_get_ullong(&netdev_status, "if_type", 0));
> > > +        data->if_descr = nullable_xstrdup(smap_get(&netdev_status,
> > > +                                                   "if_descr"));
> > > +    } else {
> > > +        data->if_type = 0;
> > > +        data->if_descr = NULL;
> > > +    }
> > > +
> > > +    smap_destroy(&netdev_status);
> > > +    data->if_index = htonl(port->ifindex);
> > > +    data->if_descr_len = data->if_descr ? strlen(data->if_descr) : 0;
> > > +    data->if_name = nullable_xstrdup(netdev_get_name(port->ofport->netdev));
> > > +    data->if_name_len = data->if_name ? strlen(data->if_name) : 0;
> > > +
> > > +    return 0;
> > > +}
> > > +
> > > +static void
> > > +ipfix_put_iface_data_record(const struct dpif_ipfix *di, odp_port_t port_no,
> > > +                            struct dp_packet *msg)
> > > +    OVS_REQUIRES(mutex)
> > > +{
> > > +    struct ipfix_data_record_flow_key_iface data;
> > > +    int err;
> > > +
> > > +    memset(&data, 0, sizeof(struct ipfix_data_record_flow_key_iface));
> > > +    err = ipfix_get_iface_data_record(di, port_no, &data);
> > > +    if (err == 0) {
> > > +        dp_packet_put(msg, &data.if_index, sizeof data.if_index);
> > > +        dp_packet_put(msg, &data.if_type, sizeof data.if_type);
> > > +        dp_packet_put(msg, &data.if_name_len, sizeof data.if_name_len);
> > > +        if (data.if_name_len) {
> > > +            dp_packet_put(msg, data.if_name, data.if_name_len);
> > > +        }
> > > +        dp_packet_put(msg, &data.if_descr_len, sizeof data.if_descr_len);
> > > +        if (data.if_descr_len) {
> > > +            dp_packet_put(msg, data.if_descr, data.if_descr_len);
> > > +        }
> > > +        ipfix_destroy_iface_data_record(&data);
> > > +    } else {
> > > +        dp_packet_put_zeros(msg, sizeof data.if_index);
> > > +        dp_packet_put_zeros(msg, sizeof data.if_type);
> > > +        dp_packet_put_zeros(msg, sizeof data.if_name_len);
> > > +        dp_packet_put_zeros(msg, sizeof data.if_descr_len);
> > > +    }
> > > +}
> > > +
> > >   static enum ipfix_sampled_packet_type
> > > -ipfix_cache_entry_init(struct ipfix_flow_cache_entry *entry,
> > > +ipfix_cache_entry_init(const struct dpif_ipfix *di,
> > > +                       struct ipfix_flow_cache_entry *entry,
> > >                          const struct dp_packet *packet, const struct flow *flow,
> > >                          uint64_t packet_delta_count, uint32_t obs_domain_id,
> > >                          uint32_t obs_point_id, odp_port_t output_odp_port,
> > > @@ -1919,6 +2078,7 @@ ipfix_cache_entry_init(struct ipfix_flow_cache_entry *entry,
> > >                          const struct flow_tnl *tunnel_key,
> > >                          struct dpif_ipfix_global_stats *stats,
> > >                          const struct dpif_ipfix_actions *ipfix_actions)
> > > +    OVS_REQUIRES(mutex)
> > >   {
> > >       struct ipfix_flow_key *flow_key;
> > >       struct dp_packet msg;
> > > @@ -1993,8 +2153,14 @@ ipfix_cache_entry_init(struct ipfix_flow_cache_entry *entry,
> > >          tunnel = IPFIX_PROTO_TUNNELED;
> > >       }
> > >
> > > +    uint8_t flow_direction =
> > > +        (direction == NX_ACTION_SAMPLE_INGRESS ? INGRESS_FLOW
> > > +         : direction == NX_ACTION_SAMPLE_EGRESS ? EGRESS_FLOW
> > > +         : output_odp_port == ODPP_NONE ? INGRESS_FLOW : EGRESS_FLOW);
> > > +
> > >       flow_key->obs_domain_id = obs_domain_id;
> > > -    flow_key->template_id = ipfix_get_template_id(l2, l3, l4, tunnel);
> > > +    flow_key->template_id = ipfix_get_template_id(l2, l3, l4, tunnel,
> > > +                                                  flow_direction);
> > >
> > >       /* The fields defined in the ipfix_data_record_* structs and sent
> > >        * below must match exactly the templates defined in
> > > @@ -2004,11 +2170,6 @@ ipfix_cache_entry_init(struct ipfix_flow_cache_entry *entry,
> > >           ? VLAN_ETH_HEADER_LEN : ETH_HEADER_LEN;
> > >       ethernet_total_length = dp_packet_size(packet);
> > >
> > > -    uint8_t flow_direction =
> > > -        (direction == NX_ACTION_SAMPLE_INGRESS ? INGRESS_FLOW
> > > -         : direction == NX_ACTION_SAMPLE_EGRESS ? EGRESS_FLOW
> > > -         : output_odp_port == ODPP_NONE ? INGRESS_FLOW : EGRESS_FLOW);
> > > -
> > >       /* Common Ethernet entities. */
> > >       {
> > >           struct ipfix_data_record_flow_key_common *data_common;
> > > @@ -2022,6 +2183,13 @@ ipfix_cache_entry_init(struct ipfix_flow_cache_entry *entry,
> > >           data_common->ethernet_header_length = ethernet_header_length;
> > >       }
> > >
> > > +    /* Interface Information Elements */
> > > +    ipfix_put_iface_data_record(di, flow->in_port.odp_port, &msg);
> > > +
> > > +    if (flow_direction == EGRESS_FLOW) {
> > > +        ipfix_put_iface_data_record(di, output_odp_port, &msg);
> > > +    }
> > > +
> > >       if (l2 == IPFIX_PROTO_L2_VLAN) {
> > >           struct ipfix_data_record_flow_key_vlan *data_vlan;
> > >           uint16_t vlan_id = vlan_tci_to_vid(flow->vlans[0].tci);
> > > @@ -2469,7 +2637,8 @@ ipfix_send_data_msg(struct dpif_ipfix_exporter *exporter,
> > >   }
> > >
> > >   static void
> > > -dpif_ipfix_sample(struct dpif_ipfix_exporter *exporter,
> > > +dpif_ipfix_sample(const struct dpif_ipfix *di,
> > > +                  struct dpif_ipfix_exporter *exporter,
> > >                     const struct dp_packet *packet, const struct flow *flow,
> > >                     uint64_t packet_delta_count, uint32_t obs_domain_id,
> > >                     uint32_t obs_point_id, odp_port_t output_odp_port,
> > > @@ -2477,6 +2646,7 @@ dpif_ipfix_sample(struct dpif_ipfix_exporter *exporter,
> > >                     const struct dpif_ipfix_port *tunnel_port,
> > >                     const struct flow_tnl *tunnel_key,
> > >                     const struct dpif_ipfix_actions *ipfix_actions)
> > > +    OVS_REQUIRES(mutex)
> > >   {
> > >       struct ipfix_flow_cache_entry *entry;
> > >       enum ipfix_sampled_packet_type sampled_packet_type;
> > > @@ -2484,7 +2654,7 @@ dpif_ipfix_sample(struct dpif_ipfix_exporter *exporter,
> > >       /* Create a flow cache entry from the sample. */
> > >       entry = xmalloc(sizeof *entry);
> > >       sampled_packet_type =
> > > -            ipfix_cache_entry_init(entry, packet,
> > > +            ipfix_cache_entry_init(di, entry, packet,
> > >                                      flow, packet_delta_count,
> > >                                      obs_domain_id, obs_point_id,
> > >                                      output_odp_port, direction,
> > > @@ -2542,16 +2712,16 @@ dpif_ipfix_bridge_sample(struct dpif_ipfix *di, const struct dp_packet *packet,
> > >           if (output_odp_port == ODPP_NONE && flow->tunnel.ip_dst) {
> > >               /* Input tunnel. */
> > >               tunnel_key = &flow->tunnel;
> > > -            tunnel_port = dpif_ipfix_find_port(di, input_odp_port);
> > > +            tunnel_port = dpif_ipfix_find_tunnel_port(di, input_odp_port);
> > >           }
> > >           if (output_odp_port != ODPP_NONE && output_tunnel_key) {
> > >               /* Output tunnel, output_tunnel_key must be valid. */
> > >               tunnel_key = output_tunnel_key;
> > > -            tunnel_port = dpif_ipfix_find_port(di, output_odp_port);
> > > +            tunnel_port = dpif_ipfix_find_tunnel_port(di, output_odp_port);
> > >           }
> > >       }
> > >
> > > -    dpif_ipfix_sample(&di->bridge_exporter.exporter, packet, flow,
> > > +    dpif_ipfix_sample(di, &di->bridge_exporter.exporter, packet, flow,
> > >                         packet_delta_count,
> > >                         di->bridge_exporter.options->obs_domain_id,
> > >                         di->bridge_exporter.options->obs_point_id,
> > > @@ -2587,16 +2757,16 @@ dpif_ipfix_flow_sample(struct dpif_ipfix *di, const struct dp_packet *packet,
> > >               if (output_odp_port == ODPP_NONE && flow->tunnel.ip_dst) {
> > >                   /* Input tunnel. */
> > >                   tunnel_key = &flow->tunnel;
> > > -                tunnel_port = dpif_ipfix_find_port(di, input_odp_port);
> > > +                tunnel_port = dpif_ipfix_find_tunnel_port(di, input_odp_port);
> > >               }
> > >               if (output_odp_port != ODPP_NONE && output_tunnel_key) {
> > >                   /* Output tunnel, output_tunnel_key must be valid. */
> > >                   tunnel_key = output_tunnel_key;
> > > -                tunnel_port = dpif_ipfix_find_port(di, output_odp_port);
> > > +                tunnel_port = dpif_ipfix_find_tunnel_port(di, output_odp_port);
> > >               }
> > >           }
> > >
> > > -        dpif_ipfix_sample(&node->exporter.exporter, packet, flow,
> > > +        dpif_ipfix_sample(di, &node->exporter.exporter, packet, flow,
> > >                             packet_delta_count,
> > >                             cookie->flow_sample.obs_domain_id,
> > >                             cookie->flow_sample.obs_point_id,
> > > diff --git a/ofproto/ofproto-dpif-ipfix.h b/ofproto/ofproto-dpif-ipfix.h
> > > index f91d041..1309da1 100644
> > > --- a/ofproto/ofproto-dpif-ipfix.h
> > > +++ b/ofproto/ofproto-dpif-ipfix.h
> > > @@ -38,8 +38,8 @@ struct dpif_ipfix *dpif_ipfix_create(void);
> > >   struct dpif_ipfix *dpif_ipfix_ref(const struct dpif_ipfix *);
> > >   void dpif_ipfix_unref(struct dpif_ipfix *);
> > >
> > > -void dpif_ipfix_add_tunnel_port(struct dpif_ipfix *, struct ofport *, odp_port_t);
> > > -void dpif_ipfix_del_tunnel_port(struct dpif_ipfix *, odp_port_t);
> > > +void dpif_ipfix_add_port(struct dpif_ipfix *, struct ofport *, odp_port_t);
> > > +void dpif_ipfix_del_port(struct dpif_ipfix *, odp_port_t);
> > >
> > >   uint32_t dpif_ipfix_get_bridge_exporter_probability(const struct dpif_ipfix *);
> > >   bool dpif_ipfix_get_bridge_exporter_tunnel_sampling(const struct dpif_ipfix *);
> > > @@ -47,7 +47,7 @@ bool dpif_ipfix_get_bridge_exporter_input_sampling(const struct dpif_ipfix *);
> > >   bool dpif_ipfix_get_bridge_exporter_output_sampling(const struct dpif_ipfix *);
> > >   bool dpif_ipfix_get_flow_exporter_tunnel_sampling(const struct dpif_ipfix *,
> > >                                                     const uint32_t);
> > > -bool dpif_ipfix_get_tunnel_port(const struct dpif_ipfix *, odp_port_t);
> > > +bool dpif_ipfix_is_tunnel_port(const struct dpif_ipfix *, odp_port_t);
> > >   void dpif_ipfix_set_options(
> > >       struct dpif_ipfix *,
> > >       const struct ofproto_ipfix_bridge_exporter_options *,
> > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> > > index 9e1f837..aac135f 100644
> > > --- a/ofproto/ofproto-dpif-xlate.c
> > > +++ b/ofproto/ofproto-dpif-xlate.c
> > > @@ -2952,7 +2952,7 @@ compose_ipfix_action(struct xlate_ctx *ctx, odp_port_t output_odp_port)
> > >            * OVS_USERSPACE_ATTR_TUNNEL_OUT_PORT
> > >            */
> > >           if (dpif_ipfix_get_bridge_exporter_tunnel_sampling(ipfix) &&
> > > -            dpif_ipfix_get_tunnel_port(ipfix, output_odp_port) ) {
> > > +            dpif_ipfix_is_tunnel_port(ipfix, output_odp_port) ) {
> > >              tunnel_out_port = output_odp_port;
> > >           }
> > >       }
> > > @@ -5211,7 +5211,7 @@ xlate_sample_action(struct xlate_ctx *ctx,
> > >
> > >           if (dpif_ipfix_get_flow_exporter_tunnel_sampling(ipfix,
> > >                                                            os->collector_set_id)
> > > -            && dpif_ipfix_get_tunnel_port(ipfix, output_odp_port)) {
> > > +            && dpif_ipfix_is_tunnel_port(ipfix, output_odp_port)) {
> > >               tunnel_out_port = output_odp_port;
> > >               emit_set_tunnel = true;
> > >           }
> > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > > index 1a8e829..f9c8749 100644
> > > --- a/ofproto/ofproto-dpif.c
> > > +++ b/ofproto/ofproto-dpif.c
> > > @@ -1872,9 +1872,6 @@ port_construct(struct ofport *port_)
> > >           }
> > >
> > >           port->is_tunnel = true;
> > > -        if (ofproto->ipfix) {
> > > -           dpif_ipfix_add_tunnel_port(ofproto->ipfix, port_, port->odp_port);
> > > -        }
> > >       } else {
> > >           /* Sanity-check that a mapping doesn't already exist.  This
> > >            * shouldn't happen for non-tunnel ports. */
> > > @@ -1895,6 +1892,9 @@ port_construct(struct ofport *port_)
> > >       if (ofproto->sflow) {
> > >           dpif_sflow_add_port(ofproto->sflow, port_, port->odp_port);
> > >       }
> > > +    if (ofproto->ipfix) {
> > > +       dpif_ipfix_add_port(ofproto->ipfix, port_, port->odp_port);
> > > +    }
> > >
> > >       return 0;
> > >   }
> > > @@ -1940,10 +1940,6 @@ port_destruct(struct ofport *port_, bool del)
> > >           atomic_count_dec(&ofproto->backer->tnl_count);
> > >       }
> > >
> > > -    if (port->is_tunnel && ofproto->ipfix) {
> > > -       dpif_ipfix_del_tunnel_port(ofproto->ipfix, port->odp_port);
> > > -    }
> > > -
> > >       tnl_port_del(port);
> > >       sset_find_and_delete(&ofproto->ports, devname);
> > >       sset_find_and_delete(&ofproto->ghost_ports, devname);
> > > @@ -1958,6 +1954,9 @@ port_destruct(struct ofport *port_, bool del)
> > >       if (ofproto->sflow) {
> > >           dpif_sflow_del_port(ofproto->sflow, port->odp_port);
> > >       }
> > > +    if (ofproto->ipfix) {
> > > +       dpif_ipfix_del_port(ofproto->ipfix, port->odp_port);
> > > +    }
> > >
> > >       free(port->qdscp);
> > >   }
> > > @@ -2083,13 +2082,11 @@ set_ipfix(
> > >               di, bridge_exporter_options, flow_exporters_options,
> > >               n_flow_exporters_options);
> > >
> > > -        /* Add tunnel ports only when a new ipfix created */
> > > +        /* Add ports only when a new ipfix created */
> > >           if (new_di == true) {
> > >               struct ofport_dpif *ofport;
> > >               HMAP_FOR_EACH (ofport, up.hmap_node, &ofproto->up.ports) {
> > > -                if (ofport->is_tunnel == true) {
> > > -                    dpif_ipfix_add_tunnel_port(di, &ofport->up, ofport->odp_port);
> > > -                }
> > > +                dpif_ipfix_add_port(di, &ofport->up, ofport->odp_port);
> > >               }
> > >           }
> > >
> > >
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Patch

diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
index 472c272..138c325 100644
--- a/ofproto/ofproto-dpif-ipfix.c
+++ b/ofproto/ofproto-dpif-ipfix.c
@@ -115,11 +115,12 @@  struct dpif_ipfix_global_stats {
 };
 
 struct dpif_ipfix_port {
-    struct hmap_node hmap_node; /* In struct dpif_ipfix's "tunnel_ports" hmap. */
+    struct hmap_node hmap_node; /* In struct dpif_ipfix's "ports" hmap. */
     struct ofport *ofport;      /* To retrieve port stats. */
     odp_port_t odp_port;
     enum dpif_ipfix_tunnel_type tunnel_type;
     uint8_t tunnel_key_length;
+    uint32_t ifindex;
 };
 
 struct dpif_ipfix_exporter {
@@ -157,9 +158,9 @@  struct dpif_ipfix_flow_exporter_map_node {
 struct dpif_ipfix {
     struct dpif_ipfix_bridge_exporter bridge_exporter;
     struct hmap flow_exporter_map;  /* dpif_ipfix_flow_exporter_map_node. */
-    struct hmap tunnel_ports;       /* Contains "struct dpif_ipfix_port"s.
-                                     * It makes tunnel port lookups faster in
-                                     * sampling upcalls. */
+    struct hmap ports;              /* Contains "struct dpif_ipfix_port"s.
+                                     * It makes port lookups faster in sampling
+                                     * upcalls. */
     struct ovs_refcount ref_cnt;
 };
 
@@ -293,7 +294,8 @@  BUILD_ASSERT_DECL(sizeof(struct ipfix_template_field_specifier) == 8);
 /* Cf. IETF RFC 5102 Section 5.11.6. */
 enum ipfix_flow_direction {
     INGRESS_FLOW = 0x00,
-    EGRESS_FLOW = 0x01
+    EGRESS_FLOW = 0x01,
+    NUM_IPFIX_FLOW_DIRECTION
 };
 
 /* Part of data record flow key for common metadata and Ethernet entities. */
@@ -308,6 +310,18 @@  struct ipfix_data_record_flow_key_common {
 });
 BUILD_ASSERT_DECL(sizeof(struct ipfix_data_record_flow_key_common) == 20);
 
+/* Part of data record flow key for interface information. Since some of the
+ * elements have variable length, members of this structure should be appended
+ * to the 'struct dp_packet' one by one. */
+struct ipfix_data_record_flow_key_iface {
+    ovs_be32 if_index;     /* (INGRESS | EGRESS)_INTERFACE */
+    ovs_be32 if_type;     /* (INGRESS | EGRESS)_INTERFACE_TYPE */
+    uint8_t if_name_len;   /* Variable length element: INTERFACE_NAME */
+    char *if_name;
+    uint8_t if_descr_len; /* Variable length element: INTERFACE_DESCRIPTION */
+    char *if_descr;
+};
+
 /* Part of data record flow key for VLAN entities. */
 OVS_PACKED(
 struct ipfix_data_record_flow_key_vlan {
@@ -745,7 +759,7 @@  dpif_ipfix_find_port(const struct dpif_ipfix *di,
     struct dpif_ipfix_port *dip;
 
     HMAP_FOR_EACH_IN_BUCKET (dip, hmap_node, hash_odp_port(odp_port),
-                             &di->tunnel_ports) {
+                             &di->ports) {
         if (dip->odp_port == odp_port) {
             return dip;
         }
@@ -754,82 +768,116 @@  dpif_ipfix_find_port(const struct dpif_ipfix *di,
 }
 
 static void
-dpif_ipfix_del_port(struct dpif_ipfix *di,
+dpif_ipfix_del_port__(struct dpif_ipfix *di,
                       struct dpif_ipfix_port *dip)
     OVS_REQUIRES(mutex)
 {
-    hmap_remove(&di->tunnel_ports, &dip->hmap_node);
+    hmap_remove(&di->ports, &dip->hmap_node);
     free(dip);
 }
 
+static enum dpif_ipfix_tunnel_type
+dpif_ipfix_tunnel_type(const struct ofport *ofport)
+{
+    const char *type = netdev_get_type(ofport->netdev);
+
+    if (type == NULL) {
+        return DPIF_IPFIX_TUNNEL_UNKNOWN;
+    }
+    if (strcmp(type, "gre") == 0) {
+        return DPIF_IPFIX_TUNNEL_GRE;
+    } else if (strcmp(type, "vxlan") == 0) {
+        return DPIF_IPFIX_TUNNEL_VXLAN;
+    } else if (strcmp(type, "lisp") == 0) {
+        return DPIF_IPFIX_TUNNEL_LISP;
+    } else if (strcmp(type, "geneve") == 0) {
+        return DPIF_IPFIX_TUNNEL_GENEVE;
+    } else if (strcmp(type, "stt") == 0) {
+        return DPIF_IPFIX_TUNNEL_STT;
+    }
+
+    return DPIF_IPFIX_TUNNEL_UNKNOWN;
+}
+
+static uint8_t
+dpif_ipfix_tunnel_key_length(enum dpif_ipfix_tunnel_type tunnel_type)
+{
+
+    switch (tunnel_type) {
+        case DPIF_IPFIX_TUNNEL_GRE:
+            /* 32-bit key gre */
+            return 4;
+        case DPIF_IPFIX_TUNNEL_VXLAN:
+        case DPIF_IPFIX_TUNNEL_LISP:
+        case DPIF_IPFIX_TUNNEL_GENEVE:
+            return 3;
+        case DPIF_IPFIX_TUNNEL_STT:
+            return 8;
+        case DPIF_IPFIX_TUNNEL_UNKNOWN:
+        case NUM_DPIF_IPFIX_TUNNEL:
+        default:
+            return 0;
+    }
+}
+
 void
-dpif_ipfix_add_tunnel_port(struct dpif_ipfix *di, struct ofport *ofport,
-                           odp_port_t odp_port) OVS_EXCLUDED(mutex)
+dpif_ipfix_add_port(struct dpif_ipfix *di, struct ofport *ofport,
+                    odp_port_t odp_port) OVS_EXCLUDED(mutex)
 {
     struct dpif_ipfix_port *dip;
-    const char *type;
+    int64_t ifindex;
 
     ovs_mutex_lock(&mutex);
     dip = dpif_ipfix_find_port(di, odp_port);
     if (dip) {
-        dpif_ipfix_del_port(di, dip);
+        dpif_ipfix_del_port__(di, dip);
     }
 
-    type = netdev_get_type(ofport->netdev);
-    if (type == NULL) {
-        goto out;
+    ifindex = netdev_get_ifindex(ofport->netdev);
+    if (ifindex < 0) {
+        ifindex = 0;
     }
 
-    /* Add to table of tunnel ports. */
+    /* Add to table of ports. */
     dip = xmalloc(sizeof *dip);
     dip->ofport = ofport;
     dip->odp_port = odp_port;
-    if (strcmp(type, "gre") == 0) {
-        /* 32-bit key gre */
-        dip->tunnel_type = DPIF_IPFIX_TUNNEL_GRE;
-        dip->tunnel_key_length = 4;
-    } else if (strcmp(type, "vxlan") == 0) {
-        dip->tunnel_type = DPIF_IPFIX_TUNNEL_VXLAN;
-        dip->tunnel_key_length = 3;
-    } else if (strcmp(type, "lisp") == 0) {
-        dip->tunnel_type = DPIF_IPFIX_TUNNEL_LISP;
-        dip->tunnel_key_length = 3;
-    } else if (strcmp(type, "geneve") == 0) {
-        dip->tunnel_type = DPIF_IPFIX_TUNNEL_GENEVE;
-        dip->tunnel_key_length = 3;
-    } else if (strcmp(type, "stt") == 0) {
-        dip->tunnel_type = DPIF_IPFIX_TUNNEL_STT;
-        dip->tunnel_key_length = 8;
-    } else {
-        free(dip);
-        goto out;
-    }
-    hmap_insert(&di->tunnel_ports, &dip->hmap_node, hash_odp_port(odp_port));
+    dip->tunnel_type = dpif_ipfix_tunnel_type(ofport);
+    dip->tunnel_key_length = dpif_ipfix_tunnel_key_length(dip->tunnel_type);
+    dip->ifindex = ifindex;
+    hmap_insert(&di->ports, &dip->hmap_node, hash_odp_port(odp_port));
 
-out:
     ovs_mutex_unlock(&mutex);
 }
 
 void
-dpif_ipfix_del_tunnel_port(struct dpif_ipfix *di, odp_port_t odp_port)
+dpif_ipfix_del_port(struct dpif_ipfix *di, odp_port_t odp_port)
     OVS_EXCLUDED(mutex)
 {
     struct dpif_ipfix_port *dip;
     ovs_mutex_lock(&mutex);
     dip = dpif_ipfix_find_port(di, odp_port);
     if (dip) {
-        dpif_ipfix_del_port(di, dip);
+        dpif_ipfix_del_port__(di, dip);
     }
     ovs_mutex_unlock(&mutex);
 }
 
+static struct dpif_ipfix_port *
+dpif_ipfix_find_tunnel_port(const struct dpif_ipfix *di, odp_port_t odp_port)
+    OVS_REQUIRES(mutex)
+{
+    struct dpif_ipfix_port *dip = dpif_ipfix_find_port(di, odp_port);
+    return (dip && dip->tunnel_type != DPIF_IPFIX_TUNNEL_UNKNOWN) ? dip : NULL;
+}
+
 bool
-dpif_ipfix_get_tunnel_port(const struct dpif_ipfix *di, odp_port_t odp_port)
+dpif_ipfix_is_tunnel_port(const struct dpif_ipfix *di, odp_port_t odp_port)
     OVS_EXCLUDED(mutex)
 {
     struct dpif_ipfix_port *dip;
     ovs_mutex_lock(&mutex);
-    dip = dpif_ipfix_find_port(di, odp_port);
+    dip = dpif_ipfix_find_tunnel_port(di, odp_port);
     ovs_mutex_unlock(&mutex);
     return dip != NULL;
 }
@@ -1065,7 +1113,7 @@  dpif_ipfix_create(void)
     di = xzalloc(sizeof *di);
     dpif_ipfix_bridge_exporter_init(&di->bridge_exporter);
     hmap_init(&di->flow_exporter_map);
-    hmap_init(&di->tunnel_ports);
+    hmap_init(&di->ports);
     ovs_refcount_init(&di->ref_cnt);
     return di;
 }
@@ -1159,8 +1207,8 @@  dpif_ipfix_clear(struct dpif_ipfix *di) OVS_REQUIRES(mutex)
         free(exp_node);
     }
 
-    HMAP_FOR_EACH_SAFE (dip, next, hmap_node, &di->tunnel_ports) {
-        dpif_ipfix_del_port(di, dip);
+    HMAP_FOR_EACH_SAFE (dip, next, hmap_node, &di->ports) {
+        dpif_ipfix_del_port__(di, dip);
     }
 }
 
@@ -1172,7 +1220,7 @@  dpif_ipfix_unref(struct dpif_ipfix *di) OVS_EXCLUDED(mutex)
         dpif_ipfix_clear(di);
         dpif_ipfix_bridge_exporter_destroy(&di->bridge_exporter);
         hmap_destroy(&di->flow_exporter_map);
-        hmap_destroy(&di->tunnel_ports);
+        hmap_destroy(&di->ports);
         free(di);
         ovs_mutex_unlock(&mutex);
     }
@@ -1211,13 +1259,15 @@  ipfix_send_msg(const struct collectors *collectors, struct dp_packet *msg)
 
 static uint16_t
 ipfix_get_template_id(enum ipfix_proto_l2 l2, enum ipfix_proto_l3 l3,
-                      enum ipfix_proto_l4 l4, enum ipfix_proto_tunnel tunnel)
+                      enum ipfix_proto_l4 l4, enum ipfix_proto_tunnel tunnel,
+                      enum ipfix_flow_direction flow_direction)
 {
     uint16_t template_id;
     template_id = l2;
     template_id = template_id * NUM_IPFIX_PROTO_L3 + l3;
     template_id = template_id * NUM_IPFIX_PROTO_L4 + l4;
     template_id = template_id * NUM_IPFIX_PROTO_TUNNEL + tunnel;
+    template_id = template_id * NUM_IPFIX_FLOW_DIRECTION + flow_direction;
     return IPFIX_TEMPLATE_ID_MIN + template_id;
 }
 
@@ -1229,7 +1279,8 @@  ipfix_get_options_template_id(enum ipfix_options_template opt_tmpl_type)
     uint16_t max_tmpl_id = ipfix_get_template_id(NUM_IPFIX_PROTO_L2,
                                                  NUM_IPFIX_PROTO_L3,
                                                  NUM_IPFIX_PROTO_L4,
-                                                 NUM_IPFIX_PROTO_TUNNEL);
+                                                 NUM_IPFIX_PROTO_TUNNEL,
+                                                 NUM_IPFIX_FLOW_DIRECTION);
 
     return max_tmpl_id + opt_tmpl_type;
 }
@@ -1325,7 +1376,9 @@  ipfix_def_options_template_fields(enum ipfix_options_template opt_tmpl_type,
 static uint16_t
 ipfix_define_template_fields(enum ipfix_proto_l2 l2, enum ipfix_proto_l3 l3,
                              enum ipfix_proto_l4 l4, enum ipfix_proto_tunnel tunnel,
-                             bool virtual_obs_id_set, size_t tmpl_hdr_offset,
+                             enum ipfix_flow_direction flow_direction,
+                             bool virtual_obs_id_set,
+                             size_t tmpl_hdr_offset,
                              struct dp_packet *msg)
 {
 
@@ -1343,6 +1396,19 @@  ipfix_define_template_fields(enum ipfix_proto_l2 l2, enum ipfix_proto_l3 l3,
     DEF(ETHERNET_TYPE);
     DEF(ETHERNET_HEADER_LENGTH);
 
+    /* Interface Information Elements */
+    DEF(INGRESS_INTERFACE);
+    DEF(INGRESS_INTERFACE_TYPE);
+    DEF(INTERFACE_NAME);
+    DEF(INTERFACE_DESCRIPTION);
+
+    if (flow_direction == EGRESS_FLOW) {
+        DEF(EGRESS_INTERFACE);
+        DEF(EGRESS_INTERFACE_TYPE);
+        DEF(INTERFACE_NAME);
+        DEF(INTERFACE_DESCRIPTION);
+    }
+
     if (l2 == IPFIX_PROTO_L2_VLAN) {
         DEF(VLAN_ID);
         DEF(DOT1Q_VLAN_ID);
@@ -1544,6 +1610,24 @@  ipfix_send_options_template_msgs(struct dpif_ipfix_exporter *exporter,
 }
 
 static void
+ipfix_add_template_record(enum ipfix_proto_l2 l2, enum ipfix_proto_l3 l3,
+                          enum ipfix_proto_l4 l4,
+                          enum ipfix_proto_tunnel tunnel,
+                          enum ipfix_flow_direction flow_direction,
+                          bool virtual_obs_id_set,
+                          struct dp_packet *msg)
+{
+    struct ipfix_template_record_header *tmpl_hdr;
+    size_t tmpl_hdr_offset = dp_packet_size(msg);
+
+    tmpl_hdr = dp_packet_put_zeros(msg, sizeof *tmpl_hdr);
+    tmpl_hdr->template_id =
+        htons(ipfix_get_template_id(l2, l3, l4, tunnel, flow_direction));
+    ipfix_define_template_fields(l2, l3, l4, tunnel, flow_direction,
+                                 virtual_obs_id_set, tmpl_hdr_offset, msg);
+}
+
+static void
 ipfix_send_template_msgs(struct dpif_ipfix_exporter *exporter,
                          uint32_t export_time_sec, uint32_t obs_domain_id)
 {
@@ -1551,14 +1635,14 @@  ipfix_send_template_msgs(struct dpif_ipfix_exporter *exporter,
     struct dp_packet msg;
     dp_packet_use_stub(&msg, msg_stub, sizeof msg_stub);
 
-    size_t set_hdr_offset, tmpl_hdr_offset, error_pkts;
-    struct ipfix_template_record_header *tmpl_hdr;
+    size_t set_hdr_offset, error_pkts;
     size_t tx_packets = 0;
     size_t tx_errors = 0;
     enum ipfix_proto_l2 l2;
     enum ipfix_proto_l3 l3;
     enum ipfix_proto_l4 l4;
     enum ipfix_proto_tunnel tunnel;
+    enum ipfix_flow_direction flow_direction;
 
     ipfix_init_template_msg(export_time_sec, exporter->seq_number,
                             obs_domain_id, IPFIX_SET_ID_TEMPLATE, &msg,
@@ -1573,41 +1657,44 @@  ipfix_send_template_msgs(struct dpif_ipfix_exporter *exporter,
                     continue;
                 }
                 for (tunnel = 0; tunnel < NUM_IPFIX_PROTO_TUNNEL; tunnel++) {
-                    /* When the size of the template packet reaches
-                     * MAX_MESSAGE_LEN(1024), send it out.
-                     * And then reinitialize the msg to construct a new
-                     * packet for the following templates.
-                     */
-                    if (dp_packet_size(&msg) >= MAX_MESSAGE_LEN) {
-                        /* Send template message. */
-                        error_pkts = ipfix_send_template_msg(exporter->collectors,
-                                                             &msg, set_hdr_offset);
-                        tx_errors += error_pkts;
-                        tx_packets += collectors_count(exporter->collectors) - error_pkts;
-
-                        /* Reinitialize the template msg. */
-                        ipfix_init_template_msg(export_time_sec,
-                                                exporter->seq_number,
-                                                obs_domain_id,
-                                                IPFIX_SET_ID_TEMPLATE,
-                                                &msg,
-                                                &set_hdr_offset);
+                    for (flow_direction = 0;
+                         flow_direction < NUM_IPFIX_FLOW_DIRECTION;
+                         flow_direction++) {
+                        /* When the size of the template packet reaches
+                         * MAX_MESSAGE_LEN(1024), send it out.
+                         * And then reinitialize the msg to construct a new
+                         * packet for the following templates.
+                         */
+                        if (dp_packet_size(&msg) >= MAX_MESSAGE_LEN) {
+                            /* Send template message. */
+                            error_pkts =
+                                ipfix_send_template_msg(exporter->collectors,
+                                                        &msg, set_hdr_offset);
+                            tx_errors += error_pkts;
+                            tx_packets +=
+                                collectors_count(exporter->collectors)
+                                - error_pkts;
+
+                            /* Reinitialize the template msg. */
+                            ipfix_init_template_msg(export_time_sec,
+                                                    exporter->seq_number,
+                                                    obs_domain_id,
+                                                    IPFIX_SET_ID_TEMPLATE,
+                                                    &msg, &set_hdr_offset);
+                        }
+
+                        ipfix_add_template_record(l2, l3, l4, tunnel,
+                                flow_direction,
+                                exporter->virtual_obs_id != NULL, &msg);
                     }
-
-                    tmpl_hdr_offset = dp_packet_size(&msg);
-                    tmpl_hdr = dp_packet_put_zeros(&msg, sizeof *tmpl_hdr);
-                    tmpl_hdr->template_id = htons(
-                        ipfix_get_template_id(l2, l3, l4, tunnel));
-                    ipfix_define_template_fields(
-                        l2, l3, l4, tunnel, exporter->virtual_obs_id != NULL,
-                        tmpl_hdr_offset, &msg);
                 }
             }
         }
     }
 
     /* Send template message. */
-    error_pkts = ipfix_send_template_msg(exporter->collectors, &msg, set_hdr_offset);
+    error_pkts = ipfix_send_template_msg(exporter->collectors, &msg,
+                                         set_hdr_offset);
     tx_errors += error_pkts;
     tx_packets += collectors_count(exporter->collectors) - error_pkts;
 
@@ -1909,8 +1996,80 @@  ipfix_cache_update(struct dpif_ipfix_exporter *exporter,
     }
 }
 
+static void
+ipfix_destroy_iface_data_record(struct ipfix_data_record_flow_key_iface *data)
+{
+    free(data->if_descr);
+    free(data->if_name);
+}
+
+/* Fills '*data' structure based on port number 'port_no'.  Caller must destroy
+ * 'data' with ipfix_destroy_iface_data_record(). */
+static int
+ipfix_get_iface_data_record(const struct dpif_ipfix *di, odp_port_t port_no,
+                            struct ipfix_data_record_flow_key_iface *data)
+    OVS_REQUIRES(mutex)
+{
+    struct dpif_ipfix_port *port;
+    struct smap netdev_status;
+
+    port = dpif_ipfix_find_port(di, port_no);
+    if (!port) {
+        return -1;
+    }
+
+    smap_init(&netdev_status);
+    if (!netdev_get_status(port->ofport->netdev, &netdev_status)) {
+        data->if_type = htonl(smap_get_ullong(&netdev_status, "if_type", 0));
+        data->if_descr = nullable_xstrdup(smap_get(&netdev_status,
+                                                   "if_descr"));
+    } else {
+        data->if_type = 0;
+        data->if_descr = NULL;
+    }
+
+    smap_destroy(&netdev_status);
+    data->if_index = htonl(port->ifindex);
+    data->if_descr_len = data->if_descr ? strlen(data->if_descr) : 0;
+    data->if_name = nullable_xstrdup(netdev_get_name(port->ofport->netdev));
+    data->if_name_len = data->if_name ? strlen(data->if_name) : 0;
+
+    return 0;
+}
+
+static void
+ipfix_put_iface_data_record(const struct dpif_ipfix *di, odp_port_t port_no,
+                            struct dp_packet *msg)
+    OVS_REQUIRES(mutex)
+{
+    struct ipfix_data_record_flow_key_iface data;
+    int err;
+
+    memset(&data, 0, sizeof(struct ipfix_data_record_flow_key_iface));
+    err = ipfix_get_iface_data_record(di, port_no, &data);
+    if (err == 0) {
+        dp_packet_put(msg, &data.if_index, sizeof data.if_index);
+        dp_packet_put(msg, &data.if_type, sizeof data.if_type);
+        dp_packet_put(msg, &data.if_name_len, sizeof data.if_name_len);
+        if (data.if_name_len) {
+            dp_packet_put(msg, data.if_name, data.if_name_len);
+        }
+        dp_packet_put(msg, &data.if_descr_len, sizeof data.if_descr_len);
+        if (data.if_descr_len) {
+            dp_packet_put(msg, data.if_descr, data.if_descr_len);
+        }
+        ipfix_destroy_iface_data_record(&data);
+    } else {
+        dp_packet_put_zeros(msg, sizeof data.if_index);
+        dp_packet_put_zeros(msg, sizeof data.if_type);
+        dp_packet_put_zeros(msg, sizeof data.if_name_len);
+        dp_packet_put_zeros(msg, sizeof data.if_descr_len);
+    }
+}
+
 static enum ipfix_sampled_packet_type
-ipfix_cache_entry_init(struct ipfix_flow_cache_entry *entry,
+ipfix_cache_entry_init(const struct dpif_ipfix *di,
+                       struct ipfix_flow_cache_entry *entry,
                        const struct dp_packet *packet, const struct flow *flow,
                        uint64_t packet_delta_count, uint32_t obs_domain_id,
                        uint32_t obs_point_id, odp_port_t output_odp_port,
@@ -1919,6 +2078,7 @@  ipfix_cache_entry_init(struct ipfix_flow_cache_entry *entry,
                        const struct flow_tnl *tunnel_key,
                        struct dpif_ipfix_global_stats *stats,
                        const struct dpif_ipfix_actions *ipfix_actions)
+    OVS_REQUIRES(mutex)
 {
     struct ipfix_flow_key *flow_key;
     struct dp_packet msg;
@@ -1993,8 +2153,14 @@  ipfix_cache_entry_init(struct ipfix_flow_cache_entry *entry,
        tunnel = IPFIX_PROTO_TUNNELED;
     }
 
+    uint8_t flow_direction =
+        (direction == NX_ACTION_SAMPLE_INGRESS ? INGRESS_FLOW
+         : direction == NX_ACTION_SAMPLE_EGRESS ? EGRESS_FLOW
+         : output_odp_port == ODPP_NONE ? INGRESS_FLOW : EGRESS_FLOW);
+
     flow_key->obs_domain_id = obs_domain_id;
-    flow_key->template_id = ipfix_get_template_id(l2, l3, l4, tunnel);
+    flow_key->template_id = ipfix_get_template_id(l2, l3, l4, tunnel,
+                                                  flow_direction);
 
     /* The fields defined in the ipfix_data_record_* structs and sent
      * below must match exactly the templates defined in
@@ -2004,11 +2170,6 @@  ipfix_cache_entry_init(struct ipfix_flow_cache_entry *entry,
         ? VLAN_ETH_HEADER_LEN : ETH_HEADER_LEN;
     ethernet_total_length = dp_packet_size(packet);
 
-    uint8_t flow_direction =
-        (direction == NX_ACTION_SAMPLE_INGRESS ? INGRESS_FLOW
-         : direction == NX_ACTION_SAMPLE_EGRESS ? EGRESS_FLOW
-         : output_odp_port == ODPP_NONE ? INGRESS_FLOW : EGRESS_FLOW);
-
     /* Common Ethernet entities. */
     {
         struct ipfix_data_record_flow_key_common *data_common;
@@ -2022,6 +2183,13 @@  ipfix_cache_entry_init(struct ipfix_flow_cache_entry *entry,
         data_common->ethernet_header_length = ethernet_header_length;
     }
 
+    /* Interface Information Elements */
+    ipfix_put_iface_data_record(di, flow->in_port.odp_port, &msg);
+
+    if (flow_direction == EGRESS_FLOW) {
+        ipfix_put_iface_data_record(di, output_odp_port, &msg);
+    }
+
     if (l2 == IPFIX_PROTO_L2_VLAN) {
         struct ipfix_data_record_flow_key_vlan *data_vlan;
         uint16_t vlan_id = vlan_tci_to_vid(flow->vlans[0].tci);
@@ -2469,7 +2637,8 @@  ipfix_send_data_msg(struct dpif_ipfix_exporter *exporter,
 }
 
 static void
-dpif_ipfix_sample(struct dpif_ipfix_exporter *exporter,
+dpif_ipfix_sample(const struct dpif_ipfix *di,
+                  struct dpif_ipfix_exporter *exporter,
                   const struct dp_packet *packet, const struct flow *flow,
                   uint64_t packet_delta_count, uint32_t obs_domain_id,
                   uint32_t obs_point_id, odp_port_t output_odp_port,
@@ -2477,6 +2646,7 @@  dpif_ipfix_sample(struct dpif_ipfix_exporter *exporter,
                   const struct dpif_ipfix_port *tunnel_port,
                   const struct flow_tnl *tunnel_key,
                   const struct dpif_ipfix_actions *ipfix_actions)
+    OVS_REQUIRES(mutex)
 {
     struct ipfix_flow_cache_entry *entry;
     enum ipfix_sampled_packet_type sampled_packet_type;
@@ -2484,7 +2654,7 @@  dpif_ipfix_sample(struct dpif_ipfix_exporter *exporter,
     /* Create a flow cache entry from the sample. */
     entry = xmalloc(sizeof *entry);
     sampled_packet_type =
-            ipfix_cache_entry_init(entry, packet,
+            ipfix_cache_entry_init(di, entry, packet,
                                    flow, packet_delta_count,
                                    obs_domain_id, obs_point_id,
                                    output_odp_port, direction,
@@ -2542,16 +2712,16 @@  dpif_ipfix_bridge_sample(struct dpif_ipfix *di, const struct dp_packet *packet,
         if (output_odp_port == ODPP_NONE && flow->tunnel.ip_dst) {
             /* Input tunnel. */
             tunnel_key = &flow->tunnel;
-            tunnel_port = dpif_ipfix_find_port(di, input_odp_port);
+            tunnel_port = dpif_ipfix_find_tunnel_port(di, input_odp_port);
         }
         if (output_odp_port != ODPP_NONE && output_tunnel_key) {
             /* Output tunnel, output_tunnel_key must be valid. */
             tunnel_key = output_tunnel_key;
-            tunnel_port = dpif_ipfix_find_port(di, output_odp_port);
+            tunnel_port = dpif_ipfix_find_tunnel_port(di, output_odp_port);
         }
     }
 
-    dpif_ipfix_sample(&di->bridge_exporter.exporter, packet, flow,
+    dpif_ipfix_sample(di, &di->bridge_exporter.exporter, packet, flow,
                       packet_delta_count,
                       di->bridge_exporter.options->obs_domain_id,
                       di->bridge_exporter.options->obs_point_id,
@@ -2587,16 +2757,16 @@  dpif_ipfix_flow_sample(struct dpif_ipfix *di, const struct dp_packet *packet,
             if (output_odp_port == ODPP_NONE && flow->tunnel.ip_dst) {
                 /* Input tunnel. */
                 tunnel_key = &flow->tunnel;
-                tunnel_port = dpif_ipfix_find_port(di, input_odp_port);
+                tunnel_port = dpif_ipfix_find_tunnel_port(di, input_odp_port);
             }
             if (output_odp_port != ODPP_NONE && output_tunnel_key) {
                 /* Output tunnel, output_tunnel_key must be valid. */
                 tunnel_key = output_tunnel_key;
-                tunnel_port = dpif_ipfix_find_port(di, output_odp_port);
+                tunnel_port = dpif_ipfix_find_tunnel_port(di, output_odp_port);
             }
         }
 
-        dpif_ipfix_sample(&node->exporter.exporter, packet, flow,
+        dpif_ipfix_sample(di, &node->exporter.exporter, packet, flow,
                           packet_delta_count,
                           cookie->flow_sample.obs_domain_id,
                           cookie->flow_sample.obs_point_id,
diff --git a/ofproto/ofproto-dpif-ipfix.h b/ofproto/ofproto-dpif-ipfix.h
index f91d041..1309da1 100644
--- a/ofproto/ofproto-dpif-ipfix.h
+++ b/ofproto/ofproto-dpif-ipfix.h
@@ -38,8 +38,8 @@  struct dpif_ipfix *dpif_ipfix_create(void);
 struct dpif_ipfix *dpif_ipfix_ref(const struct dpif_ipfix *);
 void dpif_ipfix_unref(struct dpif_ipfix *);
 
-void dpif_ipfix_add_tunnel_port(struct dpif_ipfix *, struct ofport *, odp_port_t);
-void dpif_ipfix_del_tunnel_port(struct dpif_ipfix *, odp_port_t);
+void dpif_ipfix_add_port(struct dpif_ipfix *, struct ofport *, odp_port_t);
+void dpif_ipfix_del_port(struct dpif_ipfix *, odp_port_t);
 
 uint32_t dpif_ipfix_get_bridge_exporter_probability(const struct dpif_ipfix *);
 bool dpif_ipfix_get_bridge_exporter_tunnel_sampling(const struct dpif_ipfix *);
@@ -47,7 +47,7 @@  bool dpif_ipfix_get_bridge_exporter_input_sampling(const struct dpif_ipfix *);
 bool dpif_ipfix_get_bridge_exporter_output_sampling(const struct dpif_ipfix *);
 bool dpif_ipfix_get_flow_exporter_tunnel_sampling(const struct dpif_ipfix *,
                                                   const uint32_t);
-bool dpif_ipfix_get_tunnel_port(const struct dpif_ipfix *, odp_port_t);
+bool dpif_ipfix_is_tunnel_port(const struct dpif_ipfix *, odp_port_t);
 void dpif_ipfix_set_options(
     struct dpif_ipfix *,
     const struct ofproto_ipfix_bridge_exporter_options *,
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 9e1f837..aac135f 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2952,7 +2952,7 @@  compose_ipfix_action(struct xlate_ctx *ctx, odp_port_t output_odp_port)
          * OVS_USERSPACE_ATTR_TUNNEL_OUT_PORT
          */
         if (dpif_ipfix_get_bridge_exporter_tunnel_sampling(ipfix) &&
-            dpif_ipfix_get_tunnel_port(ipfix, output_odp_port) ) {
+            dpif_ipfix_is_tunnel_port(ipfix, output_odp_port) ) {
            tunnel_out_port = output_odp_port;
         }
     }
@@ -5211,7 +5211,7 @@  xlate_sample_action(struct xlate_ctx *ctx,
 
         if (dpif_ipfix_get_flow_exporter_tunnel_sampling(ipfix,
                                                          os->collector_set_id)
-            && dpif_ipfix_get_tunnel_port(ipfix, output_odp_port)) {
+            && dpif_ipfix_is_tunnel_port(ipfix, output_odp_port)) {
             tunnel_out_port = output_odp_port;
             emit_set_tunnel = true;
         }
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 1a8e829..f9c8749 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -1872,9 +1872,6 @@  port_construct(struct ofport *port_)
         }
 
         port->is_tunnel = true;
-        if (ofproto->ipfix) {
-           dpif_ipfix_add_tunnel_port(ofproto->ipfix, port_, port->odp_port);
-        }
     } else {
         /* Sanity-check that a mapping doesn't already exist.  This
          * shouldn't happen for non-tunnel ports. */
@@ -1895,6 +1892,9 @@  port_construct(struct ofport *port_)
     if (ofproto->sflow) {
         dpif_sflow_add_port(ofproto->sflow, port_, port->odp_port);
     }
+    if (ofproto->ipfix) {
+       dpif_ipfix_add_port(ofproto->ipfix, port_, port->odp_port);
+    }
 
     return 0;
 }
@@ -1940,10 +1940,6 @@  port_destruct(struct ofport *port_, bool del)
         atomic_count_dec(&ofproto->backer->tnl_count);
     }
 
-    if (port->is_tunnel && ofproto->ipfix) {
-       dpif_ipfix_del_tunnel_port(ofproto->ipfix, port->odp_port);
-    }
-
     tnl_port_del(port);
     sset_find_and_delete(&ofproto->ports, devname);
     sset_find_and_delete(&ofproto->ghost_ports, devname);
@@ -1958,6 +1954,9 @@  port_destruct(struct ofport *port_, bool del)
     if (ofproto->sflow) {
         dpif_sflow_del_port(ofproto->sflow, port->odp_port);
     }
+    if (ofproto->ipfix) {
+       dpif_ipfix_del_port(ofproto->ipfix, port->odp_port);
+    }
 
     free(port->qdscp);
 }
@@ -2083,13 +2082,11 @@  set_ipfix(
             di, bridge_exporter_options, flow_exporters_options,
             n_flow_exporters_options);
 
-        /* Add tunnel ports only when a new ipfix created */
+        /* Add ports only when a new ipfix created */
         if (new_di == true) {
             struct ofport_dpif *ofport;
             HMAP_FOR_EACH (ofport, up.hmap_node, &ofproto->up.ports) {
-                if (ofport->is_tunnel == true) {
-                    dpif_ipfix_add_tunnel_port(di, &ofport->up, ofport->odp_port);
-                }
+                dpif_ipfix_add_port(di, &ofport->up, ofport->odp_port);
             }
         }