Hello, Are there are any other comments?
Thank you all in advance. Br, Michal. > -----Original Message----- > From: [email protected] > [mailto:[email protected]] On Behalf Of Weglicki, MichalX > Sent: Wednesday, October 4, 2017 10:19 AM > To: Greg Rose <[email protected]>; [email protected] > Subject: Re: [ovs-dev] [PATCH v5 2/2] ofproto-dpif-ipfix: add interface > Information Elements to flow key > > > -----Original Message----- > > From: Greg Rose [mailto:[email protected]] > > Sent: Wednesday, October 4, 2017 12:21 AM > > To: Weglicki, MichalX <[email protected]>; [email protected] > > 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 <[email protected]> > > > Signed-off-by: Michal Weglicki <[email protected]> > > > Signed-off-by: Przemyslaw Szczerbik <[email protected]> > > > > Michal and Przemyslaw, > > > > There is one more small nit pick that I came across noted below. > > > > Other than that.... > > > > Tested-by: Greg Rose <[email protected]> > > Reviewed-by: Greg Rose <[email protected]> > > > > > --- > > > 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 > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
