Hi Lukasz, Thanks for expanding the 802.1ad support. I'm not familiar with sFlow, so I'll provide what feedback I can.
On Tue, Apr 18, 2017 at 12:08:22PM +0100, Lukasz Rzasik wrote: > This patch implements QinQ related sFlow counters. > It is implemented according to sFlow Version 5, > http://www.sflow.org/sflow_version_5.txt > Open vSwitch will send a stack of stripped VLAN tags > to sFlow collector if the original packets have multiple > VLAN tags. > > Unit tests have been updated accordingly. > > The patch is based on commit f0fb825a37 (Add support > for 802.1ad (QinQ tunneling)) > > Signed-off-by: Lukasz Rzasik <lukaszx.rza...@intel.com> > CC: Thomas F Herbert <thomasfherb...@gmail.com> > CC: Xiao Liang <shaw.l...@gmail.com> > CC: Eric Garver <e...@erig.me> > CC: Neil McKee <neil.mc...@inmon.com> > --- > lib/packets.h | 4 +++ > ofproto/ofproto-dpif-sflow.c | 60 +++++++++++++++++++++++++++++--- > ofproto/ofproto-dpif-sflow.h | 9 +++++ > tests/ofproto-dpif.at | 83 > ++++++++++++++++++++++++++++++++++++++++++++ > tests/test-sflow.c | 25 +++++++++++++ > 5 files changed, 177 insertions(+), 4 deletions(-) > > diff --git a/lib/packets.h b/lib/packets.h > index 755f08d..7555dfc 100644 > --- a/lib/packets.h > +++ b/lib/packets.h > @@ -384,6 +384,10 @@ static inline bool eth_type_vlan(ovs_be16 eth_type) > eth_type == htons(ETH_TYPE_VLAN_8021AD); > } > > +static inline bool eth_type_8021Q(ovs_be16 eth_type) > +{ > + return eth_type == htons(ETH_TYPE_VLAN_8021Q); > +} > > /* Minimum value for an Ethernet type. Values below this are IEEE 802.2 > frame > * lengths. */ > diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c > index 59fafa1..41972e2 100644 > --- a/ofproto/ofproto-dpif-sflow.c > +++ b/ofproto/ofproto-dpif-sflow.c > @@ -1084,6 +1084,31 @@ dpif_sflow_capture_input_mpls(const struct flow *flow, > } > } > > +static void > +dpif_sflow_pop_vlan(const struct flow *flow, > + struct dpif_sflow_actions *sflow_actions) > +{ > + union flow_vlan_hdr vlan = flow->vlans[0]; > + if (eth_type_vlan(vlan.tpid)) { > + int depth = 0; > + int ii; > + /* Calculate depth by detecting 8021Q TPID. */ > + for (ii = 0; ii < FLOW_MAX_VLAN_HEADERS; ii++) { > + vlan = flow->vlans[ii]; > + depth++; > + if (eth_type_8021Q(vlan.tpid)) { > + break; > + } This seems to be using 0x8100 as a marker to stop. Does this mean double stacked 0x8100 will not work? flow_count_vlan_headers() can be used to find the number of tags. > + } > + > + if (depth > 1) { > + sflow_actions->vlan_qtag[sflow_actions->vlan_stack_depth] = > + ntohl(flow->vlans[sflow_actions->vlan_stack_depth].qtag); > + sflow_actions->vlan_stack_depth++; > + } AFAICS, this only adds the outermost tag to the stack. Is that intentional? It's unclear to me if sFlow expects one or two VLANs to be in the stack. > + } > +} > + > void > dpif_sflow_read_actions(const struct flow *flow, > const struct nlattr *actions, size_t actions_len, > @@ -1170,11 +1195,10 @@ dpif_sflow_read_actions(const struct flow *flow, > break; > > case OVS_ACTION_ATTR_PUSH_VLAN: > + break; > + > case OVS_ACTION_ATTR_POP_VLAN: > - /* TODO: 802.1AD(QinQ) is not supported by OVS (yet), so do not > - * construct a VLAN-stack. The sFlow user-action cookie already > - * captures the egress VLAN ID so there is nothing more to do here. > - */ > + dpif_sflow_pop_vlan(flow, sflow_actions); > break; > > case OVS_ACTION_ATTR_PUSH_MPLS: { > @@ -1225,6 +1249,19 @@ dpif_sflow_encode_mpls_stack(SFLLabelStack *stack, > stack->stack[stack->depth - 1] |= MPLS_BOS_MASK; > } > > +static void > +dpif_sflow_encode_vlan_stack(SFLVlanStack *stack, > + uint32_t *vlans_buf, > + const struct dpif_sflow_actions *sflow_actions) > +{ > + int ii; > + stack->depth = sflow_actions->vlan_stack_depth; > + stack->stack = vlans_buf; > + for (ii = 0; ii < stack->depth; ii++) { > + stack->stack[ii] = sflow_actions->vlan_qtag[ii]; > + } > +} > + > /* Extract the output port count from the user action cookie. > * See http://sflow.org/sflow_version_5.txt "Input/Output port information" > */ > @@ -1258,6 +1295,8 @@ dpif_sflow_received(struct dpif_sflow *ds, const struct > dp_packet *packet, > SFLFlow_sample_element vniInElem, vniOutElem; > SFLFlow_sample_element mplsElem; > uint32_t mpls_lse_buf[FLOW_MAX_MPLS_LABELS]; > + SFLFlow_sample_element vlanTunnelElem; > + uint32_t vlans_buf[FLOW_MAX_VLAN_HEADERS]; > SFLSampler *sampler; > struct dpif_sflow_port *in_dsp; > struct dpif_sflow_port *out_dsp; > @@ -1372,6 +1411,19 @@ dpif_sflow_received(struct dpif_sflow *ds, const > struct dp_packet *packet, > SFLADD_ELEMENT(&fs, &mplsElem); > } > > + /* stripped VLAN tags stack. */ > + if (sflow_actions > + && sflow_actions->vlan_stack_depth > 0 > + && dpif_sflow_cookie_num_outputs(cookie) == 1) { > + memset(&vlanTunnelElem, 0, sizeof(vlanTunnelElem)); > + vlanTunnelElem.tag = SFLFLOW_EX_VLAN_TUNNEL; > + dpif_sflow_encode_vlan_stack( > + &vlanTunnelElem.flowType.vlan_tunnel.stack, > + vlans_buf, > + sflow_actions); > + SFLADD_ELEMENT(&fs, &vlanTunnelElem); > + } > + > /* Submit the flow sample to be encoded into the next datagram. */ > SFLADD_ELEMENT(&fs, &hdrElem); > SFLADD_ELEMENT(&fs, &switchElem); > diff --git a/ofproto/ofproto-dpif-sflow.h b/ofproto/ofproto-dpif-sflow.h > index 014e6cc..cfd5492 100644 > --- a/ofproto/ofproto-dpif-sflow.h > +++ b/ofproto/ofproto-dpif-sflow.h > @@ -49,6 +49,15 @@ struct dpif_sflow_actions { > uint32_t mpls_lse[FLOW_MAX_MPLS_LABELS]; /* Out stack in host byte > order. */ > uint32_t mpls_stack_depth; /* Out stack depth. */ > bool mpls_err; /* MPLS actions parse failure. > */ > + > + /* Using host-byte order for the vlan stack here > + to match the expectations of the sFlow library. > + List of stripped 802.1Q TPID/TCI layers. Each > + TPID,TCI pair is represented as a single 32 bit > + integer. Layers listed from outermost to innermost. > + */ > + uint32_t vlan_qtag[FLOW_MAX_VLAN_HEADERS]; /* Stack in host byte order. > */ > + uint32_t vlan_stack_depth; /* Stack depth. */ > }; > > struct dpif_sflow *dpif_sflow_create(void); > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at > index 0c2ea38..f096175 100644 > --- a/tests/ofproto-dpif.at > +++ b/tests/ofproto-dpif.at > @@ -6333,6 +6333,89 @@ HEADER > OVS_VSWITCHD_STOP > AT_CLEANUP > > +AT_SETUP([ofproto-dpif - sFlow packet sampling - Extended VLAN tunnel]) > +AT_XFAIL_IF([test "$IS_WIN32" = "yes"]) > +OVS_VSWITCHD_START > +AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg]) > +add_of_ports br0 1 2 > +AT_DATA([flows.txt], [dnl > +table=0 dl_src=50:54:00:00:00:10 actions=strip_vlan,2 > +]) > +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) > + > +dnl set up sFlow logging > +AT_CHECK([ovstest test-sflow --log-file --detach --no-chdir --pidfile > 0:127.0.0.1 > sflow.log], [0], [], [ignore]) > +AT_CAPTURE_FILE([sflow.log]) > +PARSE_LISTENING_PORT([test-sflow.log], [SFLOW_PORT]) > +ovs-appctl time/stop > + > +dnl configure sflow > +ovs-vsctl \ > + set Bridge br0 sflow=@sf -- \ > + --id=@sf create sflow targets=\"127.0.0.1:$SFLOW_PORT\" \ > + header=128 sampling=1 polling=0 > + > +AT_CHECK([ovs-appctl netdev-dummy/receive p1 > 'in_port(1),eth(src=50:54:00:00:00:10,dst=50:54:00:00:00:0a),eth_type(0x88a8),vlan(vid=150,pcp=0,cfi=1),encap(eth_type(0x8100),vlan(vid=2000,pcp=0,cfi=1),encap(eth_type(0x0800)))']) > +AT_CHECK([ovs-appctl netdev-dummy/receive p1 > 'in_port(1),eth(src=50:54:00:00:00:10,dst=50:54:00:00:00:0a),eth_type(0x8100),vlan(vid=2000,pcp=0,cfi=1),encap(eth_type(0x0800))']) > + > +dnl sleep long enough to get the sFlow datagram flushed out (may be delayed > for up to 1 second) > +for i in `seq 1 30`; do > + ovs-appctl time/warp 100 > +done > + > +OVS_APP_EXIT_AND_WAIT([test-sflow]) > + > +AT_CHECK_UNQUOTED([[sort sflow.log | $EGREP 'HEADER|ERROR' | sed 's/ /\ > + /g']], [0], [dnl > +HEADER > + dgramSeqNo=1 > + ds=127.0.0.1>2:1000 > + fsSeqNo=1 > + ext_vlan_tpid_0=0x88a8 > + ext_vlan_vid_0=150 > + ext_vlan_pcp_0=0 > + ext_vlan_cfi_0=1 > + in_vlan=150 > + in_priority=0 > + out_vlan=0 > + out_priority=0 > + meanSkip=1 > + samplePool=1 > + dropEvents=0 > + in_ifindex=0 > + in_format=0 > + out_ifindex=1 > + out_format=2 > + hdr_prot=1 > + pkt_len=22 > + stripped=4 > + hdr_len=18 > + hdr=50-54-00-00-00-0A-50-54-00-00-00-10-88-A8-00-96-81-00 > +HEADER > + dgramSeqNo=1 > + ds=127.0.0.1>2:1000 > + fsSeqNo=2 > + in_vlan=2000 > + in_priority=0 > + out_vlan=0 > + out_priority=0 > + meanSkip=1 > + samplePool=2 > + dropEvents=0 > + in_ifindex=0 > + in_format=0 > + out_ifindex=1 > + out_format=2 > + hdr_prot=1 > + pkt_len=42 > + stripped=4 > + hdr_len=38 > + > hdr=50-54-00-00-00-0A-50-54-00-00-00-10-81-00-07-D0-08-00-45-00-00-14-00-00-00-00-00-00-BA-EB-00-00-00-00-00-00-00-00 > +]) > + > +OVS_VSWITCHD_STOP > +AT_CLEANUP > + > > # CHECK_NETFLOW_EXPIRATION(LOOPBACK_ADDR) > # > diff --git a/tests/test-sflow.c b/tests/test-sflow.c > index 6125d38..7d58ef3 100644 > --- a/tests/test-sflow.c > +++ b/tests/test-sflow.c > @@ -65,6 +65,7 @@ static unixctl_cb_func test_sflow_exit; > #define SFLOW_TAG_PKT_TUNNEL_VNI_OUT 1029 > #define SFLOW_TAG_PKT_TUNNEL_VNI_IN 1030 > #define SFLOW_TAG_PKT_MPLS 1006 > +#define SFLOW_TAG_PKT_EXTENDED_VLAN 1012 > > /* string sizes */ > #define SFL_MAX_PORTNAME_LEN 255 > @@ -116,6 +117,7 @@ struct sflow_xdr { > uint32_t TUNNEL_VNI_OUT; > uint32_t TUNNEL_VNI_IN; > uint32_t MPLS; > + uint32_t EXT_VLAN; > uint32_t IFCOUNTERS; > uint32_t ETHCOUNTERS; > uint32_t LACPCOUNTERS; > @@ -428,6 +430,25 @@ process_flow_sample(struct sflow_xdr *x) > } > } > > + if (x->offset.EXT_VLAN) { > + uint32_t stack_depth, ii; > + union flow_vlan_hdr vlan; > + sflowxdr_setc(x, x->offset.EXT_VLAN); > + /* print stripped VLAN tags stack */ > + stack_depth = sflowxdr_next(x); > + for (ii = 0; ii < stack_depth; ii++) { > + vlan.qtag=sflowxdr_next_n(x); > + printf(" ext_vlan_tpid_%"PRIu32"=0x%"PRIx32, > + ii, ntohs(vlan.tpid)); > + printf(" ext_vlan_vid_%"PRIu32"=%"PRIu32, > + ii, vlan_tci_to_vid(vlan.tci)); > + printf(" ext_vlan_pcp_%"PRIu32"=%"PRIu32, > + ii, vlan_tci_to_pcp(vlan.tci)); > + printf(" ext_vlan_cfi_%"PRIu32"=%"PRIu32, > + ii, vlan_tci_to_cfi(vlan.tci)); > + } > + } > + > if (x->offset.SWITCH) { > sflowxdr_setc(x, x->offset.SWITCH); > printf(" in_vlan=%"PRIu32, sflowxdr_next(x)); > @@ -634,6 +655,10 @@ process_datagram(struct sflow_xdr *x) > sflowxdr_mark_unique(x, &x->offset.MPLS); > break; > > + case SFLOW_TAG_PKT_EXTENDED_VLAN: > + sflowxdr_mark_unique(x, &x->offset.EXT_VLAN); > + break; > + > /* Add others here... */ > } > > -- > 1.9.3 > > -------------------------------------------------------------- > Intel Research and Development Ireland Limited > Registered in Ireland > Registered Office: Collinstown Industrial Park, Leixlip, County Kildare > Registered Number: 308263 > > > This e-mail and any attachments may contain confidential material for the sole > use of the intended recipient(s). Any review or distribution by others is > strictly prohibited. If you are not the intended recipient, please contact the > sender and delete all copies. > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
