Re: [ovs-dev] [PATCH 1/7] Add support for 802.1ad (QinQ tunneling)

[Xiao Liang]

On Fri, Mar 17, 2017 at 11:57 AM, Ben Pfaff  wrote:
> On Thu, Mar 16, 2017 at 09:18:00PM -0400, Eric Garver wrote:
>> On Thu, Mar 16, 2017 at 03:23:38PM -0700, Ben Pfaff wrote:
>> > Thanks for all the iteration on this patch.
>> >
>> > I've folded in the following incremental and applied this to master.
>>
>> Great! Thanks for the feedback and clean ups.
>>
>> Do you intend to take the tests as well? The macro OVS_CHECK_8021AD()
>> probably needs adjusted due the incremental below.
>>
>> The dot1q-tunnel patches should stand on their own and can be re-spun if
>> needed.
>
> I'm working on the dot1q-tunnel patch now and expect to apply it
> tomorrow, or if not then at least review it.
>
> After that, I think that I'll probably ask for a respin of the remaining
> patches.
>
> I am so glad to finally get QinQ support in, and I imagine that goes
> double for the authors of this patch.

I am really happy to see this feature goes in. Thanks for the efforts
from Thomas, Ben, Andy, and especially Eric.

Thanks,
Xiao
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/7] Add support for 802.1ad (QinQ tunneling)

[Ben Pfaff]

On Thu, Mar 16, 2017 at 09:18:00PM -0400, Eric Garver wrote:
> On Thu, Mar 16, 2017 at 03:23:38PM -0700, Ben Pfaff wrote:
> > Thanks for all the iteration on this patch.
> > 
> > I've folded in the following incremental and applied this to master.
> 
> Great! Thanks for the feedback and clean ups.
> 
> Do you intend to take the tests as well? The macro OVS_CHECK_8021AD()
> probably needs adjusted due the incremental below.
>
> The dot1q-tunnel patches should stand on their own and can be re-spun if
> needed.

I'm working on the dot1q-tunnel patch now and expect to apply it
tomorrow, or if not then at least review it.

After that, I think that I'll probably ask for a respin of the remaining
patches.

I am so glad to finally get QinQ support in, and I imagine that goes
double for the authors of this patch.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/7] Add support for 802.1ad (QinQ tunneling)

[Eric Garver]

On Thu, Mar 16, 2017 at 03:23:38PM -0700, Ben Pfaff wrote:
> On Wed, Mar 01, 2017 at 05:47:59PM -0500, Eric Garver wrote:
> > Flow key handling changes:
> >  - Add VLAN header array in struct flow, to record multiple 802.1q VLAN
> >headers.
> >  - Add dpif multi-VLAN capability probing. If datapath supports
> >multi-VLAN, increase the maximum depth of nested OVS_KEY_ATTR_ENCAP.
> > 
> > Refactor VLAN handling in dpif-xlate:
> >  - Introduce 'xvlan' to track VLAN stack during flow processing.
> >  - Input and output VLAN translation according to the xbundle type.
> > 
> > Push VLAN action support:
> >  - Allow ethertype 0x88a8 in VLAN headers and push_vlan action.
> >  - Support push_vlan on dot1q packets.
> > 
> > Use other_config:vlan-limit in table Open_vSwitch to limit maximum VLANs
> > that can be matched. This allows us to preserve backwards compatibility.
> > 
> > Add test cases for VLAN depth limit, Multi-VLAN actions and QinQ VLAN
> > handling
> > 
> > Co-authored-by: Thomas F Herbert 
> > Signed-off-by: Thomas F Herbert 
> > Co-authored-by: Xiao Liang 
> > Signed-off-by: Xiao Liang 
> > Signed-off-by: Eric Garver 
> 
> Thanks for all the iteration on this patch.
> 
> I've folded in the following incremental and applied this to master.

Great! Thanks for the feedback and clean ups.

Do you intend to take the tests as well? The macro OVS_CHECK_8021AD()
probably needs adjusted due the incremental below.

The dot1q-tunnel patches should stand on their own and can be re-spun if
needed.

> 
> Some reasoning for my changes:
> 
>   - I changed the ROUND_UP in struct flow to a build assertion that
> FLOW_MAX_VLAN_HEADERS is a multiple of 2 because we have a fair
> number of cases where we memcpy an array of FLOW_MAX_VLAN_HEADERS
> elements to or from vlans[].  That was going to be risky if we ever
> used "sizeof" on the vlans[] member and FLOW_MAX_VLAN_HEADERS was
> not a multiple of 2, because it would copy too many bytes.  So, on
> balance, it seemed safer this way.

Understood. Thanks for the cleanup.

> (Maybe we should do the same thing for mpls_lse.)
> 
>   - Some style improvements, mostly related to ?: and to moving variable
> declarations closer to first use.
>
>   - I reverted the change to vswitch.ovsschema since it only updated the
> version without any other changes.

I think that was remnant of when the dot1q-tunnel was part of this
patch. I guess I missed that when I split them.

>   - I made the documentation more explicit.
> 
> It seems that I made a few other minor changes while I was fixing up
> rebase conflicts, most notably making the NEWS item shorter.  Those
> aren't in the diff below (sorry about that).
> 
> --8<--cut here-->8--
> 
> diff --git a/include/openvswitch/flow.h b/include/openvswitch/flow.h
> index e7622af8f4ae..188467dc42d5 100644
> --- a/include/openvswitch/flow.h
> +++ b/include/openvswitch/flow.h
> @@ -23,7 +23,7 @@
>  /* This sequence number should be incremented whenever anything involving 
> flows
>   * or the wildcarding of flows changes.  This will cause build assertion
>   * failures in places which likely need to be updated. */
> -#define FLOW_WC_SEQ 37
> +#define FLOW_WC_SEQ 38
>  
>  /* Number of Open vSwitch extension 32-bit registers. */
>  #define FLOW_N_REGS 16
> @@ -64,8 +64,12 @@ const char *flow_tun_flag_to_string(uint32_t flags);
>  /* Maximum number of supported SAMPLE action nesting. */
>  #define FLOW_MAX_SAMPLE_NESTING 10
>  
> -/* Maximum number of supported VLAN headers. */
> +/* Maximum number of supported VLAN headers.
> + *
> + * We require this to be a multiple of 2 so that vlans[] in struct flow is a
> + * multiple of 64 bits. */
>  #define FLOW_MAX_VLAN_HEADERS 2
> +BUILD_ASSERT_DECL(FLOW_MAX_VLAN_HEADERS % 2 == 0);
>  
>  /* Legacy maximum VLAN headers */
>  #define LEGACY_MAX_VLAN_HEADERS 1
> @@ -114,7 +118,7 @@ struct flow {
>  struct eth_addr dl_src; /* Ethernet source address. */
>  ovs_be16 dl_type;   /* Ethernet frame type. */
>  uint8_t pad2[2];/* Pad to 64 bits. */
> -union flow_vlan_hdr vlans[ROUND_UP(FLOW_MAX_VLAN_HEADERS, 2)]; /* VLANs 
> */
> +union flow_vlan_hdr vlans[FLOW_MAX_VLAN_HEADERS]; /* VLANs */
>  ovs_be32 mpls_lse[ROUND_UP(FLOW_MAX_MPLS_LABELS, 2)]; /* MPLS label stack
>   (with padding). 
> */
>  /* L3 (64-bit aligned) */
> @@ -154,7 +158,7 @@ BUILD_ASSERT_DECL(sizeof(struct flow_tnl) % 
> sizeof(uint64_t) == 0);
>  /* Remember to update FLOW_WC_SEQ when changing 'struct flow'. */
>  BUILD_ASSERT_DECL(offsetof(struct flow, igmp_group_ip4) + sizeof(uint32_t)
>== sizeof(struct flow_tnl) + 300
> -  && FLOW_WC_SEQ == 37);
> +  && FLOW_WC_SEQ == 38);
>  
>  /* 

Re: [ovs-dev] [PATCH 1/7] Add support for 802.1ad (QinQ tunneling)

[Ben Pfaff]

On Wed, Mar 15, 2017 at 03:06:39PM -0700, Andy Zhou wrote:
> On Wed, Mar 1, 2017 at 2:47 PM, Eric Garver  wrote:
> > Flow key handling changes:
> >  - Add VLAN header array in struct flow, to record multiple 802.1q VLAN
> >headers.
> >  - Add dpif multi-VLAN capability probing. If datapath supports
> >multi-VLAN, increase the maximum depth of nested OVS_KEY_ATTR_ENCAP.
> >
> > Refactor VLAN handling in dpif-xlate:
> >  - Introduce 'xvlan' to track VLAN stack during flow processing.
> >  - Input and output VLAN translation according to the xbundle type.
> >
> > Push VLAN action support:
> >  - Allow ethertype 0x88a8 in VLAN headers and push_vlan action.
> >  - Support push_vlan on dot1q packets.
> >
> > Use other_config:vlan-limit in table Open_vSwitch to limit maximum VLANs
> > that can be matched. This allows us to preserve backwards compatibility.
> >
> > Add test cases for VLAN depth limit, Multi-VLAN actions and QinQ VLAN
> > handling
> >
> > Co-authored-by: Thomas F Herbert 
> > Signed-off-by: Thomas F Herbert 
> > Co-authored-by: Xiao Liang 
> > Signed-off-by: Xiao Liang 
> > Signed-off-by: Eric Garver 
> 
> Thanks for working on this. And sorry it took a while to get those reviewed.
> 
> The patch does not apply cleanly to master.  Do you mind rebase and repost?
> I suppose you will need to this anyways for them to be pushed.

Hmm, by coincidence I was also looking at this patch, so I'm going to
respond directly to some of your comments.

> > @@ -103,7 +109,8 @@ struct flow {
> >  struct eth_addr dl_dst; /* Ethernet destination address. */
> >  struct eth_addr dl_src; /* Ethernet source address. */
> >  ovs_be16 dl_type;   /* Ethernet frame type. */
> > -ovs_be16 vlan_tci;  /* If 802.1Q, TCI | VLAN_CFI; otherwise 0. 
> > */
> > +uint8_t pad2[2];/* Pad to 64 bits. */
> Do we need 4 more bytes for 64 bit alignment?

I don't think so, this looks correct to me.  Assuming dl_dst is properly
aligned, there are 6+6+2+2 == 16 bytes from it to the new vlans[] member.

> > +void
> > +flow_pop_vlan(struct flow *flow, struct flow_wildcards *wc)
> > +{
> > +int n = flow_count_vlan_headers(flow);
> > +if (n == 0) {
> > +return;
> > +}
> Can n be 0? or should this be an assert instead?

The use in ofpact_check__() suggests to me that this is correct as-is,
unless we want to change the interface.

> > +if (wc) {
> > +memset(>masks.vlans[1], 0xff,
> > +   sizeof(union flow_vlan_hdr) * (n - 1));
> > +}
> 
> Should we also set the mask for wc->masks.vlans[0]?

Wildcards are mostly about read dependencies.  This only writes to
vlans[0] so I don't think it's necessary.

flow_pop_mpls() follows the same pattern.

> > +memmove(>vlans[0], >vlans[1],
> > +sizeof(union flow_vlan_hdr) * (n - 1));
> > +memset(>vlans[n - 1], 0, sizeof(union flow_vlan_hdr));
> > +}
> > +
> > +void
> > +flow_push_vlan_uninit(struct flow *flow, struct flow_wildcards *wc)
> > +{
> > +int n = flow_count_vlan_headers(flow);
> 
> Do we always know there is room in the flow?

It looks like we don't, but the one caller where it matters has
essentially the same failure case beforehand.

> > +if (wc) {
> > +memset(wc->masks.vlans, 0xff, sizeof(union flow_vlan_hdr) * n);
> > +}
> Should we set mask for the vlan fields that is about to be pushed?

I think that this does not introduce a read dependency.

> > +memmove(>vlans[1], >vlans[0],
> > +sizeof(union flow_vlan_hdr) * (FLOW_MAX_VLAN_HEADERS - 1));
> > +memset(>vlans[0], 0, sizeof(union flow_vlan_hdr));
> The function syas "_uninit",  I don't think the last memset() is needed.

Hmm.  The caller still needs to initialize it, otherwise it's invalid
since the CFI bit isn't set.  You're right that it's zeroed.  This is
not a perfect situation here.

Anyway, I'm pretty happy with this patch and I'm likely to push it (with
changes) pretty soon.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/7] Add support for 802.1ad (QinQ tunneling)

[Andy Zhou]

On Wed, Mar 1, 2017 at 2:47 PM, Eric Garver  wrote:
> Flow key handling changes:
>  - Add VLAN header array in struct flow, to record multiple 802.1q VLAN
>headers.
>  - Add dpif multi-VLAN capability probing. If datapath supports
>multi-VLAN, increase the maximum depth of nested OVS_KEY_ATTR_ENCAP.
>
> Refactor VLAN handling in dpif-xlate:
>  - Introduce 'xvlan' to track VLAN stack during flow processing.
>  - Input and output VLAN translation according to the xbundle type.
>
> Push VLAN action support:
>  - Allow ethertype 0x88a8 in VLAN headers and push_vlan action.
>  - Support push_vlan on dot1q packets.
>
> Use other_config:vlan-limit in table Open_vSwitch to limit maximum VLANs
> that can be matched. This allows us to preserve backwards compatibility.
>
> Add test cases for VLAN depth limit, Multi-VLAN actions and QinQ VLAN
> handling
>
> Co-authored-by: Thomas F Herbert 
> Signed-off-by: Thomas F Herbert 
> Co-authored-by: Xiao Liang 
> Signed-off-by: Xiao Liang 
> Signed-off-by: Eric Garver 

Thanks for working on this. And sorry it took a while to get those reviewed.

The patch does not apply cleanly to master.  Do you mind rebase and repost?
I suppose you will need to this anyways for them to be pushed.

I have a few comments for this patch,  I will review this other
patches more carefully
after rebase.

git am complained about extra while space. I think it is about the
line below ofproto_set_vlan_limit()
>
>  v2.7.0 - xx xxx 
>  -
> diff --git a/include/openvswitch/flow.h b/include/openvswitch/flow.h
> index df80dfe46199..0ed02e08fd2d 100644
> --- a/include/openvswitch/flow.h
> +++ b/include/openvswitch/flow.h
> @@ -23,7 +23,7 @@
>  /* This sequence number should be incremented whenever anything involving 
> flows
>   * or the wildcarding of flows changes.  This will cause build assertion
>   * failures in places which likely need to be updated. */
> -#define FLOW_WC_SEQ 36
> +#define FLOW_WC_SEQ 37
>
>  /* Number of Open vSwitch extension 32-bit registers. */
>  #define FLOW_N_REGS 16
> @@ -61,6 +61,12 @@ const char *flow_tun_flag_to_string(uint32_t flags);
>  /* Maximum number of supported MPLS labels. */
>  #define FLOW_MAX_MPLS_LABELS 3
>
> +/* Maximum number of supported VLAN headers. */
> +#define FLOW_MAX_VLAN_HEADERS 2
> +
> +/* Legacy maximum VLAN headers */
> +#define LEGACY_MAX_VLAN_HEADERS 1
> +
>  /*
>   * A flow in the network.
>   *
> @@ -103,7 +109,8 @@ struct flow {
>  struct eth_addr dl_dst; /* Ethernet destination address. */
>  struct eth_addr dl_src; /* Ethernet source address. */
>  ovs_be16 dl_type;   /* Ethernet frame type. */
> -ovs_be16 vlan_tci;  /* If 802.1Q, TCI | VLAN_CFI; otherwise 0. */
> +uint8_t pad2[2];/* Pad to 64 bits. */
Do we need 4 more bytes for 64 bit alignment?



> +void
> +flow_pop_vlan(struct flow *flow, struct flow_wildcards *wc)
> +{
> +int n = flow_count_vlan_headers(flow);
> +if (n == 0) {
> +return;
> +}
Can n be 0? or should this be an assert instead?
> +if (wc) {
> +memset(>masks.vlans[1], 0xff,
> +   sizeof(union flow_vlan_hdr) * (n - 1));
> +}

Should we also set the mask for wc->masks.vlans[0]?
> +memmove(>vlans[0], >vlans[1],
> +sizeof(union flow_vlan_hdr) * (n - 1));
> +memset(>vlans[n - 1], 0, sizeof(union flow_vlan_hdr));
> +}
> +
> +void
> +flow_push_vlan_uninit(struct flow *flow, struct flow_wildcards *wc)
> +{
> +int n = flow_count_vlan_headers(flow);

Do we always know there is room in the flow?
> +if (wc) {
> +memset(wc->masks.vlans, 0xff, sizeof(union flow_vlan_hdr) * n);
> +}
Should we set mask for the vlan fields that is about to be pushed?

> +memmove(>vlans[1], >vlans[0],
> +sizeof(union flow_vlan_hdr) * (FLOW_MAX_VLAN_HEADERS - 1));
> +memset(>vlans[0], 0, sizeof(union flow_vlan_hdr));
The function syas "_uninit",  I don't think the last memset() is needed.
>  }
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev