On Wed, Apr 19, 2017 at 02:03:15PM +0000, Rzasik, LukaszX wrote:
> Hi Eric,
>
> Thank you for looking at the patch.
>
> I put my comments inline with a tag [Lukasz].
>
> -----Original Message-----
> From: Eric Garver [mailto:[email protected]]
> Sent: Tuesday, April 18, 2017 5:25 PM
> To: Rzasik, LukaszX <[email protected]>
> Cc: [email protected]; Thomas F Herbert <[email protected]>
> Subject: Re: [ovs-dev] [PATCH] ofproto-dpif: Send QinQ related sFlow counters
>
> 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 <[email protected]>
> > CC: Thomas F Herbert <[email protected]>
> > CC: Xiao Liang <[email protected]>
> > CC: Eric Garver <[email protected]>
> > CC: Neil McKee <[email protected]>
> > ---
> > 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.
>
> [Lukasz] Yes, you are right. I'm using 0x8100 to detect the last VLAN tag and
> double stacked 0x8100 will not work. As I understand this is incorrect. From
> looking at flow_count_vlan_headers() it detects the last VLAN tag by checking
> CFI bit. If it is set to zero than it is the last tag.
Using the CFI bit is kind of historical. Before 802.1ad the TPID was not
tracked at all. So the CFI bit was used as a marker to mean "VLAN tag
present" allowing a VID of 0.
> I can change the implementation but I could not find any standard or document
> mentioning such use of that bit. Is it used in this way only internally in
> OVS or am I missing something?
You should be able to key off either the TPID or the CFI bit. Various
points of the code check one or the other. I think what you have is okay
- if not I'd consider it a bug in the flow parsing code.
> > + }
> > +
> > + 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.
>
> [Lukasz] It should add more tags if there are multiple
> OVS_ACTION_ATTR_POP_VLAN actions. sFlow says that only stripped VLAN tags
> should be reported in the stack.
Okay. That makes sense to me. But please take Neil's email under
advisement.
>
> > + }
> > +}
> > +
> > 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=20
> > +00,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
> > [email protected]
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> --------------------------------------------------------------
> 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
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
