Re: [ovs-dev] [ovs-discuss] [PATCH v2] [PATCH] QinQ: support more vlan headers.
On Tue, Sep 11, 2018 at 01:10:09AM +, Lilijun (Jerry, Cloud Networking) wrote: > Thanks for your reply. > > In my use case, it's OVS userspace datapath with dpdk. > > My detail case was a bit complicated as follows: > 1. Start the OVS userspace datapath with dpdk in my host server. > 2. A VM was running and the VNIC's vhostuser port on the userspace datapath > is configured as QinQ mode, qinq-ethtype 802.1q. > 3. Another kernel OVS is running in that VM to switch packets of some > containers. Then the container's VNIC port on the kernel datapath is also > configured as QinQ Mode, qinq-ethtype=802.1q . > 4. So when the container sends a packet with VLAN tag, the OVS running in > the host will receive a packet with 2 VLANS from the VM. > 5. Here the QinQ is not worked when we need 3 VLANs. > > Yes, VXLAN or PBB can work but we need change our basic network topology and > push/pop for every packets. That maybe the last choice if QinQ can't support > triple VLAN. There is still a push/pop for the third VLAN tag. I'm not sure it makes sense to support the extra VLANs in upstream OVS. This is non-standard and there are alternatives such as VXLAN. If it wasn't splitting the MPLS labels across cache lines then it would be a harmless change. > > -Original Message- > From: Ben Pfaff [mailto:b...@ovn.org] > Sent: Tuesday, September 11, 2018 3:33 AM > To: Eric Garver ; Lilijun (Jerry, Cloud Networking) > ; d...@openvswitch.org; ovs-disc...@openvswitch.org > Subject: Re: [ovs-discuss] [PATCH v2] [ovs-dev] [PATCH] QinQ: support more > vlan headers. > > On Mon, Sep 10, 2018 at 03:15:21PM -0400, Eric Garver wrote: > > On Mon, Sep 10, 2018 at 03:03:19AM +, Lilijun (Jerry, Cloud Networking) > > wrote: > > > Hi Eric, > > > > > > Yes, I agree with that effect. > > > But how about this issue of QinQ that we can only support at most 2 VLANs > > > ? Do you have any ideas? > > > > I was not NACKing the idea. Just wanted everyone to understand the > > implications of increasing the VLAN field size. > > > > I tried playing with the fields, but didn't come with a reasonable way > > to rearrange them to make room for the extra VLANs. > > > > I'm curious what you're use case is for triple VLAN. I wonder if VXLAN > > or PBB (802.1ah) is a better solution. > > I'd also like to know what datapath we're talking about. The Linux kernel > datapath only supports 2 VLANs in any case. > ___ > 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
Re: [ovs-dev] [ovs-discuss] [PATCH v2] [PATCH] QinQ: support more vlan headers.
Thanks for your reply. In my use case, it's OVS userspace datapath with dpdk. My detail case was a bit complicated as follows: 1. Start the OVS userspace datapath with dpdk in my host server. 2. A VM was running and the VNIC's vhostuser port on the userspace datapath is configured as QinQ mode, qinq-ethtype 802.1q. 3. Another kernel OVS is running in that VM to switch packets of some containers. Then the container's VNIC port on the kernel datapath is also configured as QinQ Mode, qinq-ethtype=802.1q . 4. So when the container sends a packet with VLAN tag, the OVS running in the host will receive a packet with 2 VLANS from the VM. 5. Here the QinQ is not worked when we need 3 VLANs. Yes, VXLAN or PBB can work but we need change our basic network topology and push/pop for every packets. That maybe the last choice if QinQ can't support triple VLAN. -Original Message- From: Ben Pfaff [mailto:b...@ovn.org] Sent: Tuesday, September 11, 2018 3:33 AM To: Eric Garver ; Lilijun (Jerry, Cloud Networking) ; d...@openvswitch.org; ovs-disc...@openvswitch.org Subject: Re: [ovs-discuss] [PATCH v2] [ovs-dev] [PATCH] QinQ: support more vlan headers. On Mon, Sep 10, 2018 at 03:15:21PM -0400, Eric Garver wrote: > On Mon, Sep 10, 2018 at 03:03:19AM +, Lilijun (Jerry, Cloud Networking) > wrote: > > Hi Eric, > > > > Yes, I agree with that effect. > > But how about this issue of QinQ that we can only support at most 2 VLANs ? > > Do you have any ideas? > > I was not NACKing the idea. Just wanted everyone to understand the > implications of increasing the VLAN field size. > > I tried playing with the fields, but didn't come with a reasonable way > to rearrange them to make room for the extra VLANs. > > I'm curious what you're use case is for triple VLAN. I wonder if VXLAN > or PBB (802.1ah) is a better solution. I'd also like to know what datapath we're talking about. The Linux kernel datapath only supports 2 VLANs in any case. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [ovs-discuss] [PATCH v2] [PATCH] QinQ: support more vlan headers.
On Mon, Sep 10, 2018 at 03:15:21PM -0400, Eric Garver wrote: > On Mon, Sep 10, 2018 at 03:03:19AM +, Lilijun (Jerry, Cloud Networking) > wrote: > > Hi Eric, > > > > Yes, I agree with that effect. > > But how about this issue of QinQ that we can only support at most 2 VLANs ? > > Do you have any ideas? > > I was not NACKing the idea. Just wanted everyone to understand the > implications of increasing the VLAN field size. > > I tried playing with the fields, but didn't come with a reasonable way > to rearrange them to make room for the extra VLANs. > > I'm curious what you're use case is for triple VLAN. I wonder if VXLAN > or PBB (802.1ah) is a better solution. I'd also like to know what datapath we're talking about. The Linux kernel datapath only supports 2 VLANs in any case. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [ovs-discuss] [PATCH v2] [PATCH] QinQ: support more vlan headers.
On Mon, Sep 10, 2018 at 03:03:19AM +, Lilijun (Jerry, Cloud Networking)
wrote:
> Hi Eric,
>
> Yes, I agree with that effect.
> But how about this issue of QinQ that we can only support at most 2 VLANs ?
> Do you have any ideas?
I was not NACKing the idea. Just wanted everyone to understand the
implications of increasing the VLAN field size.
I tried playing with the fields, but didn't come with a reasonable way
to rearrange them to make room for the extra VLANs.
I'm curious what you're use case is for triple VLAN. I wonder if VXLAN
or PBB (802.1ah) is a better solution.
>
> Thanks.
>
> -Original Message-
> From: Eric Garver [mailto:e...@garver.life]
> Sent: Friday, September 07, 2018 10:14 PM
> To: Lilijun (Jerry, Cloud Networking)
> Cc: Ben Pfaff ; d...@openvswitch.org;
> ovs-disc...@openvswitch.org
> Subject: Re: [ovs-discuss] [PATCH v2] [ovs-dev] [PATCH] QinQ: support more
> vlan headers.
>
> On Fri, Sep 07, 2018 at 09:53:44AM +, Lilijun (Jerry, Cloud Networking)
> wrote:
> > In my test, vlan-limit is set to 0 that means unlimited the count of
> > vlan headers.
> > ovs-vsctl set Open_vSwitch . other_config:vlan-limit=0
> >
> > But in fact the macro FLOW_MAX_VLAN_HEADERS is defined as 2, so we can
> > only support max two vlan headers. It doesn't work as the config
> > vlan-limit's description.
> >
> > So, when VM sends a packet already with two vlan headers, the vport
> > configured with qinq mode can't add another s-vlan headers.
> > FLOW_MAX_VLAN_HEADERS need to be larger to support more vlan headers.
> >
> > Change-Id: I8449e308d406ce3757b43a2636ff0f326ca12a9d
> > Signed-off-by: Lilijun
> > ---
> > include/openvswitch/flow.h | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/openvswitch/flow.h b/include/openvswitch/flow.h
> > index 5d2cf09..a09650c 100644
> > --- a/include/openvswitch/flow.h
> > +++ b/include/openvswitch/flow.h
> > @@ -72,7 +72,7 @@ const char *flow_tun_flag_to_string(uint32_t flags);
> > *
> > * 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
> > +#define FLOW_MAX_VLAN_HEADERS 4
>
> This change splits the MPLS labels and ct_ipv6_src across cache lines and 64
> byte boundary which affects the miniflow. This change may have a performance
> impact for scenarios that use these fields.
>
> On the plus side it pushes all the nsh fields into the same cache line.
> Previously they were split.
>
> With this patch:
>
> $ pahole -C flow lib/flow.o
> struct flow {
> struct flow_tnltunnel; /* 0 344 */
> /* --- cacheline 5 boundary (320 bytes) was 24 bytes ago --- */
> ovs_be64 metadata; /* 344 8 */
> uint32_t regs[16]; /* 35264 */
> /* --- cacheline 6 boundary (384 bytes) was 32 bytes ago --- */
> uint32_t skb_priority; /* 416 4 */
> uint32_t pkt_mark; /* 420 4 */
> uint32_t dp_hash; /* 424 4 */
> union flow_in_port in_port; /* 428 4 */
> uint32_t recirc_id;/* 432 4 */
> uint8_tct_state; /* 436 1 */
> uint8_tct_nw_proto; /* 437 1 */
> uint16_t ct_zone; /* 438 2 */
> uint32_t ct_mark; /* 440 4 */
> ovs_be32 packet_type; /* 444 4 */
> /* --- cacheline 7 boundary (448 bytes) --- */
> ovs_u128 ct_label; /* 44816 */
> uint32_t conj_id; /* 464 4 */
> ofp_port_t actset_output;/* 468 4 */
> struct eth_addrdl_dst; /* 472 6 */
> struct eth_addrdl_src; /* 478 6 */
> ovs_be16 dl_type; /* 484 2 */
> uint8_tpad1[2]; /* 486 2 */
> union flow_vlan_hdrvlans[4]; /* 48816 */
> ovs_be32 mpls_lse[4]; /* 50416 */
> /* --- cacheline 8 boundary (512 bytes) was 8 bytes ago --- */
> ovs_be32 nw_src; /* 520 4 */
> ovs_be32 nw_dst; /* 524 4 */
> ovs_be32 ct_nw_src;/* 528 4 */
> ovs_be32 ct_nw_dst;/* 532 4 */
> struct in6_addripv6_src; /* 53616 */
> struct in6_addr
Re: [ovs-dev] [ovs-discuss] [PATCH v2] [PATCH] QinQ: support more vlan headers.
Hi Eric,
Yes, I agree with that effect.
But how about this issue of QinQ that we can only support at most 2 VLANs ? Do
you have any ideas?
Thanks.
-Original Message-
From: Eric Garver [mailto:e...@garver.life]
Sent: Friday, September 07, 2018 10:14 PM
To: Lilijun (Jerry, Cloud Networking)
Cc: Ben Pfaff ; d...@openvswitch.org; ovs-disc...@openvswitch.org
Subject: Re: [ovs-discuss] [PATCH v2] [ovs-dev] [PATCH] QinQ: support more vlan
headers.
On Fri, Sep 07, 2018 at 09:53:44AM +, Lilijun (Jerry, Cloud Networking)
wrote:
> In my test, vlan-limit is set to 0 that means unlimited the count of
> vlan headers.
> ovs-vsctl set Open_vSwitch . other_config:vlan-limit=0
>
> But in fact the macro FLOW_MAX_VLAN_HEADERS is defined as 2, so we can
> only support max two vlan headers. It doesn't work as the config
> vlan-limit's description.
>
> So, when VM sends a packet already with two vlan headers, the vport
> configured with qinq mode can't add another s-vlan headers.
> FLOW_MAX_VLAN_HEADERS need to be larger to support more vlan headers.
>
> Change-Id: I8449e308d406ce3757b43a2636ff0f326ca12a9d
> Signed-off-by: Lilijun
> ---
> include/openvswitch/flow.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/openvswitch/flow.h b/include/openvswitch/flow.h
> index 5d2cf09..a09650c 100644
> --- a/include/openvswitch/flow.h
> +++ b/include/openvswitch/flow.h
> @@ -72,7 +72,7 @@ const char *flow_tun_flag_to_string(uint32_t flags);
> *
> * 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
> +#define FLOW_MAX_VLAN_HEADERS 4
This change splits the MPLS labels and ct_ipv6_src across cache lines and 64
byte boundary which affects the miniflow. This change may have a performance
impact for scenarios that use these fields.
On the plus side it pushes all the nsh fields into the same cache line.
Previously they were split.
With this patch:
$ pahole -C flow lib/flow.o
struct flow {
struct flow_tnltunnel; /* 0 344 */
/* --- cacheline 5 boundary (320 bytes) was 24 bytes ago --- */
ovs_be64 metadata; /* 344 8 */
uint32_t regs[16]; /* 35264 */
/* --- cacheline 6 boundary (384 bytes) was 32 bytes ago --- */
uint32_t skb_priority; /* 416 4 */
uint32_t pkt_mark; /* 420 4 */
uint32_t dp_hash; /* 424 4 */
union flow_in_port in_port; /* 428 4 */
uint32_t recirc_id;/* 432 4 */
uint8_tct_state; /* 436 1 */
uint8_tct_nw_proto; /* 437 1 */
uint16_t ct_zone; /* 438 2 */
uint32_t ct_mark; /* 440 4 */
ovs_be32 packet_type; /* 444 4 */
/* --- cacheline 7 boundary (448 bytes) --- */
ovs_u128 ct_label; /* 44816 */
uint32_t conj_id; /* 464 4 */
ofp_port_t actset_output;/* 468 4 */
struct eth_addrdl_dst; /* 472 6 */
struct eth_addrdl_src; /* 478 6 */
ovs_be16 dl_type; /* 484 2 */
uint8_tpad1[2]; /* 486 2 */
union flow_vlan_hdrvlans[4]; /* 48816 */
ovs_be32 mpls_lse[4]; /* 50416 */
/* --- cacheline 8 boundary (512 bytes) was 8 bytes ago --- */
ovs_be32 nw_src; /* 520 4 */
ovs_be32 nw_dst; /* 524 4 */
ovs_be32 ct_nw_src;/* 528 4 */
ovs_be32 ct_nw_dst;/* 532 4 */
struct in6_addripv6_src; /* 53616 */
struct in6_addripv6_dst; /* 55216 */
struct in6_addrct_ipv6_src; /* 56816 */
/* --- cacheline 9 boundary (576 bytes) was 8 bytes ago --- */
struct in6_addrct_ipv6_dst; /* 58416 */
ovs_be32 ipv6_label; /* 600 4 */
uint8_tnw_frag; /* 604 1 */
uint8_tnw_tos; /* 605 1 */
uint8_tnw_ttl; /* 606 1 */
uint8_tnw_proto;
Re: [ovs-dev] [ovs-discuss] [PATCH v2] [PATCH] QinQ: support more vlan headers.
On Fri, Sep 07, 2018 at 09:53:44AM +, Lilijun (Jerry, Cloud Networking)
wrote:
> In my test, vlan-limit is set to 0 that means unlimited the count of vlan
> headers.
> ovs-vsctl set Open_vSwitch . other_config:vlan-limit=0
>
> But in fact the macro FLOW_MAX_VLAN_HEADERS is defined as 2, so we can
> only support max two vlan headers. It doesn't work as the config vlan-limit's
> description.
>
> So, when VM sends a packet already with two vlan headers, the vport
> configured with
> qinq mode can't add another s-vlan headers. FLOW_MAX_VLAN_HEADERS need to
> be larger to support more vlan headers.
>
> Change-Id: I8449e308d406ce3757b43a2636ff0f326ca12a9d
> Signed-off-by: Lilijun
> ---
> include/openvswitch/flow.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/openvswitch/flow.h b/include/openvswitch/flow.h
> index 5d2cf09..a09650c 100644
> --- a/include/openvswitch/flow.h
> +++ b/include/openvswitch/flow.h
> @@ -72,7 +72,7 @@ const char *flow_tun_flag_to_string(uint32_t flags);
> *
> * 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
> +#define FLOW_MAX_VLAN_HEADERS 4
This change splits the MPLS labels and ct_ipv6_src across cache lines
and 64 byte boundary which affects the miniflow. This change may have a
performance impact for scenarios that use these fields.
On the plus side it pushes all the nsh fields into the same cache line.
Previously they were split.
With this patch:
$ pahole -C flow lib/flow.o
struct flow {
struct flow_tnltunnel; /* 0 344 */
/* --- cacheline 5 boundary (320 bytes) was 24 bytes ago --- */
ovs_be64 metadata; /* 344 8 */
uint32_t regs[16]; /* 35264 */
/* --- cacheline 6 boundary (384 bytes) was 32 bytes ago --- */
uint32_t skb_priority; /* 416 4 */
uint32_t pkt_mark; /* 420 4 */
uint32_t dp_hash; /* 424 4 */
union flow_in_port in_port; /* 428 4 */
uint32_t recirc_id;/* 432 4 */
uint8_tct_state; /* 436 1 */
uint8_tct_nw_proto; /* 437 1 */
uint16_t ct_zone; /* 438 2 */
uint32_t ct_mark; /* 440 4 */
ovs_be32 packet_type; /* 444 4 */
/* --- cacheline 7 boundary (448 bytes) --- */
ovs_u128 ct_label; /* 44816 */
uint32_t conj_id; /* 464 4 */
ofp_port_t actset_output;/* 468 4 */
struct eth_addrdl_dst; /* 472 6 */
struct eth_addrdl_src; /* 478 6 */
ovs_be16 dl_type; /* 484 2 */
uint8_tpad1[2]; /* 486 2 */
union flow_vlan_hdrvlans[4]; /* 48816 */
ovs_be32 mpls_lse[4]; /* 50416 */
/* --- cacheline 8 boundary (512 bytes) was 8 bytes ago --- */
ovs_be32 nw_src; /* 520 4 */
ovs_be32 nw_dst; /* 524 4 */
ovs_be32 ct_nw_src;/* 528 4 */
ovs_be32 ct_nw_dst;/* 532 4 */
struct in6_addripv6_src; /* 53616 */
struct in6_addripv6_dst; /* 55216 */
struct in6_addrct_ipv6_src; /* 56816 */
/* --- cacheline 9 boundary (576 bytes) was 8 bytes ago --- */
struct in6_addrct_ipv6_dst; /* 58416 */
ovs_be32 ipv6_label; /* 600 4 */
uint8_tnw_frag; /* 604 1 */
uint8_tnw_tos; /* 605 1 */
uint8_tnw_ttl; /* 606 1 */
uint8_tnw_proto; /* 607 1 */
struct in6_addrnd_target;/* 60816 */
struct eth_addrarp_sha; /* 624 6 */
struct eth_addrarp_tha; /* 630 6 */
ovs_be16 tcp_flags;/* 636 2 */
ovs_be16 pad2; /* 638 2 */
/* --- cacheline 10 boundary (640 bytes) --- */
struct
