[PATCH net-next v2 0/3] bonding: support QinQ for bond arp interval

[PATCH net-next v2 0/3] bonding: support QinQ for bond arp interval

[Ding Tianhong]

v1->v2: remvoe the comment "TODO: QinQ?".
	convert pr_xxx() to pr_xxx_ratelimited() for arp interval.

Ding Tianhong (3):
  vlan: make a new function vlan_dev_vlan_proto() and export
  bonding: support QinQ for bond arp interval
  bonding: convert pr_xxx() to pr_xxx_ratelimited() for arp interval

 drivers/net/bonding/bond_main.c | 81 ++++++++++++++++++++++++++++-------------
 drivers/net/bonding/bonding.h   |  5 +++
 include/linux/if_vlan.h         |  7 ++++
 net/8021q/vlan_core.c           |  6 +++
 4 files changed, 73 insertions(+), 26 deletions(-)

-- 
1.8.0

[PATCH net-next v2 1/3] vlan: make a new function vlan_dev_vlan_proto() and export

[Ding Tianhong]

The vlan support 2 proto: 802.1q and 802.1ad, so make a new function
called vlan_dev_vlan_proto() which could return the vlan proto for
input dev.

Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
---
 include/linux/if_vlan.h | 7 +++++++
 net/8021q/vlan_core.c   | 6 ++++++
 2 files changed, 13 insertions(+)

diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
index bbedfb5..967a428 100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -110,6 +110,7 @@ extern struct net_device *__vlan_find_dev_deep(struct net_device *real_dev,
 					       __be16 vlan_proto, u16 vlan_id);
 extern struct net_device *vlan_dev_real_dev(const struct net_device *dev);
 extern u16 vlan_dev_vlan_id(const struct net_device *dev);
+extern __be16 vlan_dev_vlan_proto(const struct net_device *dev);
 
 /**
  *	struct vlan_priority_tci_mapping - vlan egress priority mappings
@@ -216,6 +217,12 @@ static inline u16 vlan_dev_vlan_id(const struct net_device *dev)
 	return 0;
 }
 
+static inline __be16 vlan_dev_vlan_proto(const struct net_device *dev)
+{
+	BUG();
+	return 0;
+}
+
 static inline u16 vlan_dev_get_egress_qos_mask(struct net_device *dev,
 					       u32 skprio)
 {
diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
index 35b3c19..3c32bd2 100644
--- a/net/8021q/vlan_core.c
+++ b/net/8021q/vlan_core.c
@@ -106,6 +106,12 @@ u16 vlan_dev_vlan_id(const struct net_device *dev)
 }
 EXPORT_SYMBOL(vlan_dev_vlan_id);
 
+__be16 vlan_dev_vlan_proto(const struct net_device *dev)
+{
+	return vlan_dev_priv(dev)->vlan_proto;
+}
+EXPORT_SYMBOL(vlan_dev_vlan_proto);
+
 static struct sk_buff *vlan_reorder_header(struct sk_buff *skb)
 {
 	if (skb_cow(skb, skb_headroom(skb)) < 0)
-- 
1.8.0

[PATCH net-next v2 2/3] bonding: support QinQ for bond arp interval

[Ding Tianhong]

The bond send arp request to indicate that the slave is active, and if the bond dev
is a vlan dev, it will set the vlan tag in skb to notice the vlan group, but the
bond could only send a skb with 802.1q proto, not support for QinQ.

So add outer tag for lower vlan tag and inner tag for upper vlan tag to support QinQ,
The new skb will be consist of two vlan tag just like this:

dst mac | src mac | outer vlan tag | inner vlan tag | data | .....

If We don't need QinQ, the inner vlan tag could be set to 0 and use outer vlan tag
 as a normal vlan group.

Using "ip link" to configure the bond for QinQ and add test log:

ip link add link bond0  bond0.20 type vlan proto 802.1ad id 20
ip link add link bond0.20  bond0.20.200 type vlan proto 802.1q id 200

ifconfig bond0.20 11.11.20.36/24
ifconfig bond0.20.200 11.11.200.36/24

echo +11.11.200.37 > /sys/class/net/bond0/bonding/arp_ip_target

90:e2:ba:07:4a:5c (oui Unknown) > Broadcast, ethertype 802.1Q-QinQ (0x88a8),length 50: vlan 20, p 0,ethertype 802.1Q, vlan 200, p 0, ethertype ARP, Ethernet (len 6), IPv4 (len 4), Request who-has 11.11.200.37 tell 11.11.200.36, length 28

90:e2:ba:06:f9:86 (oui Unknown) > 90:e2:ba:07:4a:5c (oui Unknown), ethertype 802.1Q-QinQ (0x88a8), length 50: vlan 20, p 0, ethertype 802.1Q, vlan 200, p 0, ethertype ARP, Ethernet (len 6), IPv4 (len 4), Reply 11.11.200.37 is-at 90:e2:ba:06:f9:86 (oui Unknown), length 28

v1->v2: remove the comment "TODO: QinQ?".

Cc: Jay Vosburgh <fubar@us.ibm.com>
Cc: Veaceslav Falico <vfalico@redhat.com>
Cc: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
---
 drivers/net/bonding/bond_main.c | 63 +++++++++++++++++++++++++++++------------
 drivers/net/bonding/bonding.h   |  5 ++++
 2 files changed, 50 insertions(+), 18 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index e717db3..6758c2d 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2124,12 +2124,15 @@ static bool bond_has_this_ip(struct bonding *bond, __be32 ip)
  * switches in VLAN mode (especially if ports are configured as
  * "native" to a VLAN) might not pass non-tagged frames.
  */
-static void bond_arp_send(struct net_device *slave_dev, int arp_op, __be32 dest_ip, __be32 src_ip, unsigned short vlan_id)
+static void bond_arp_send(struct net_device *slave_dev, int arp_op,
+			  __be32 dest_ip, __be32 src_ip,
+			  struct bond_vlan_tag *inner,
+			  struct bond_vlan_tag *outer)
 {
 	struct sk_buff *skb;
 
-	pr_debug("arp %d on slave %s: dst %pI4 src %pI4 vid %d\n",
-		 arp_op, slave_dev->name, &dest_ip, &src_ip, vlan_id);
+	pr_debug("arp %d on slave %s: dst %pI4 src %pI4\n",
+		 arp_op, slave_dev->name, &dest_ip, &src_ip);
 
 	skb = arp_create(arp_op, ETH_P_ARP, dest_ip, slave_dev, src_ip,
 			 NULL, slave_dev->dev_addr, NULL);
@@ -2138,10 +2141,22 @@ static void bond_arp_send(struct net_device *slave_dev, int arp_op, __be32 dest_
 		pr_err("ARP packet allocation failed\n");
 		return;
 	}
-	if (vlan_id) {
-		skb = vlan_put_tag(skb, htons(ETH_P_8021Q), vlan_id);
+	if (outer->vlan_id) {
+		if (inner->vlan_id) {
+			pr_debug("inner tag: proto %X vid %X\n",
+				 ntohs(inner->vlan_proto), inner->vlan_id);
+			skb = __vlan_put_tag(skb, inner->vlan_proto, inner->vlan_id);
+			if (!skb) {
+				pr_err("failed to insert inner VLAN tag\n");
+				return;
+			}
+		}
+
+		pr_debug("outer reg: proto %X vid %X\n",
+			 ntohs(outer->vlan_proto), outer->vlan_id);
+		skb = vlan_put_tag(skb, outer->vlan_proto, outer->vlan_id);
 		if (!skb) {
-			pr_err("failed to insert VLAN tag\n");
+			pr_err("failed to insert outer VLAN tag\n");
 			return;
 		}
 	}
@@ -2154,11 +2169,16 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
 	struct net_device *upper, *vlan_upper;
 	struct list_head *iter, *vlan_iter;
 	struct rtable *rt;
+	struct bond_vlan_tag inner, outer;
 	__be32 *targets = bond->params.arp_targets, addr;
-	int i, vlan_id;
+	int i;
 
 	for (i = 0; i < BOND_MAX_ARP_TARGETS && targets[i]; i++) {
 		pr_debug("basa: target %pI4\n", &targets[i]);
+		inner.vlan_proto = 0;
+		inner.vlan_id = 0;
+		outer.vlan_proto = 0;
+		outer.vlan_id = 0;
 
 		/* Find out through which dev should the packet go */
 		rt = ip_route_output(dev_net(bond->dev), targets[i], 0,
@@ -2170,12 +2190,10 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
 			if (bond->params.arp_validate && net_ratelimit())
 				pr_warn("%s: no route to arp_ip_target %pI4 and arp_validate is set\n",
 					bond->dev->name, &targets[i]);
-			bond_arp_send(slave->dev, ARPOP_REQUEST, targets[i], 0, 0);
+			bond_arp_send(slave->dev, ARPOP_REQUEST, targets[i], 0, &inner, &outer);
 			continue;
 		}
 
-		vlan_id = 0;
-
 		/* bond device itself */
 		if (rt->dst.dev == bond->dev)
 			goto found;
@@ -2185,17 +2203,30 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
 		 * found we verify its upper dev list, searching for the
 		 * rt->dst.dev. If found we save the tag of the vlan and
 		 * proceed to send the packet.
-		 *
-		 * TODO: QinQ?
 		 */
 		netdev_for_each_all_upper_dev_rcu(bond->dev, vlan_upper,
 						  vlan_iter) {
 			if (!is_vlan_dev(vlan_upper))
 				continue;
+
+			if (vlan_upper == rt->dst.dev) {
+				outer.vlan_proto = vlan_dev_vlan_proto(vlan_upper);
+				outer.vlan_id = vlan_dev_vlan_id(vlan_upper);
+				rcu_read_unlock();
+				goto found;
+			}
 			netdev_for_each_all_upper_dev_rcu(vlan_upper, upper,
 							  iter) {
 				if (upper == rt->dst.dev) {
-					vlan_id = vlan_dev_vlan_id(vlan_upper);
+					/* If the upper dev is a vlan dev too,
+					 *  set the vlan tag to inner tag.
+					 */
+					if (is_vlan_dev(upper)) {
+						inner.vlan_proto = vlan_dev_vlan_proto(upper);
+						inner.vlan_id = vlan_dev_vlan_id(upper);
+					}
+					outer.vlan_proto = vlan_dev_vlan_proto(vlan_upper);
+					outer.vlan_id = vlan_dev_vlan_id(vlan_upper);
 					rcu_read_unlock();
 					goto found;
 				}
@@ -2208,10 +2239,6 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
 		 */
 		netdev_for_each_all_upper_dev_rcu(bond->dev, upper, iter) {
 			if (upper == rt->dst.dev) {
-				/* if it's a vlan - get its VID */
-				if (is_vlan_dev(upper))
-					vlan_id = vlan_dev_vlan_id(upper);
-
 				rcu_read_unlock();
 				goto found;
 			}
@@ -2230,7 +2257,7 @@ found:
 		addr = bond_confirm_addr(rt->dst.dev, targets[i], 0);
 		ip_rt_put(rt);
 		bond_arp_send(slave->dev, ARPOP_REQUEST, targets[i],
-			      addr, vlan_id);
+			      addr, &inner, &outer);
 	}
 }
 
diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index 0896f1d..b8bdd0a 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -266,6 +266,11 @@ struct bonding {
 #define bond_slave_get_rtnl(dev) \
 	((struct slave *) rtnl_dereference(dev->rx_handler_data))
 
+struct bond_vlan_tag {
+	__be16		vlan_proto;
+	unsigned short	vlan_id;
+};
+
 /**
  * Returns NULL if the net_device does not belong to any of the bond's slaves
  *
-- 
1.8.0

[PATCH net-next v2 3/3] bonding: convert pr_xxx() to pr_xxx_ratelimited() for arp interval

[Ding Tianhong]

The debug log in the arp interval should be rate limited, otherwise would
occur spam the log, so convert them.

Cc: Jay Vosburgh <fubar@us.ibm.com>
Cc: Veaceslav Falico <vfalico@redhat.com>
Cc: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
---
 drivers/net/bonding/bond_main.c | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 6758c2d..8707bb3 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2131,32 +2131,33 @@ static void bond_arp_send(struct net_device *slave_dev, int arp_op,
 {
 	struct sk_buff *skb;
 
-	pr_debug("arp %d on slave %s: dst %pI4 src %pI4\n",
-		 arp_op, slave_dev->name, &dest_ip, &src_ip);
+	pr_debug_ratelimited("arp %d on slave %s: dst %pI4 src %pI4\n",
+			     arp_op, slave_dev->name, &dest_ip, &src_ip);
 
 	skb = arp_create(arp_op, ETH_P_ARP, dest_ip, slave_dev, src_ip,
 			 NULL, slave_dev->dev_addr, NULL);
 
 	if (!skb) {
-		pr_err("ARP packet allocation failed\n");
+		pr_err_ratelimited("ARP packet allocation failed\n");
 		return;
 	}
 	if (outer->vlan_id) {
 		if (inner->vlan_id) {
-			pr_debug("inner tag: proto %X vid %X\n",
-				 ntohs(inner->vlan_proto), inner->vlan_id);
+			pr_debug_ratelimited("inner tag: proto %X vid %X\n",
+					     ntohs(inner->vlan_proto),
+					     inner->vlan_id);
 			skb = __vlan_put_tag(skb, inner->vlan_proto, inner->vlan_id);
 			if (!skb) {
-				pr_err("failed to insert inner VLAN tag\n");
+				pr_err_ratelimited("failed to insert inner VLAN tag\n");
 				return;
 			}
 		}
 
-		pr_debug("outer reg: proto %X vid %X\n",
-			 ntohs(outer->vlan_proto), outer->vlan_id);
+		pr_debug_ratelimited("outer reg: proto %X vid %X\n",
+				     ntohs(outer->vlan_proto), outer->vlan_id);
 		skb = vlan_put_tag(skb, outer->vlan_proto, outer->vlan_id);
 		if (!skb) {
-			pr_err("failed to insert outer VLAN tag\n");
+			pr_err_ratelimited("failed to insert outer VLAN tag\n");
 			return;
 		}
 	}
@@ -2174,7 +2175,7 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
 	int i;
 
 	for (i = 0; i < BOND_MAX_ARP_TARGETS && targets[i]; i++) {
-		pr_debug("basa: target %pI4\n", &targets[i]);
+		pr_debug_ratelimited("basa: target %pI4\n", &targets[i]);
 		inner.vlan_proto = 0;
 		inner.vlan_id = 0;
 		outer.vlan_proto = 0;
@@ -2187,9 +2188,10 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
 			/* there's no route to target - try to send arp
 			 * probe to generate any traffic (arp_validate=0)
 			 */
-			if (bond->params.arp_validate && net_ratelimit())
-				pr_warn("%s: no route to arp_ip_target %pI4 and arp_validate is set\n",
-					bond->dev->name, &targets[i]);
+			if (bond->params.arp_validate)
+				pr_warn_ratelimited("%s: no route to arp_ip_target %pI4 and arp_validate is set\n",
+						    bond->dev->name,
+						    &targets[i]);
 			bond_arp_send(slave->dev, ARPOP_REQUEST, targets[i], 0, &inner, &outer);
 			continue;
 		}
@@ -2246,9 +2248,9 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
 		rcu_read_unlock();
 
 		/* Not our device - skip */
-		pr_debug("%s: no path to arp_ip_target %pI4 via rt.dev %s\n",
-			 bond->dev->name, &targets[i],
-			 rt->dst.dev ? rt->dst.dev->name : "NULL");
+		pr_debug_ratelimited("%s: no path to arp_ip_target %pI4 via rt.dev %s\n",
+				     bond->dev->name, &targets[i],
+				     rt->dst.dev ? rt->dst.dev->name : "NULL");
 
 		ip_rt_put(rt);
 		continue;
-- 
1.8.0

Re: [PATCH net-next v2 3/3] bonding: convert pr_xxx() to pr_xxx_ratelimited() for arp interval

[Joe Perches]

On Tue, 2014-03-18 at 18:43 +0800, Ding Tianhong wrote:
> The debug log in the arp interval should be rate limited, otherwise would
> occur spam the log, so convert them.

Hi Ding.

pr_<level>_ratelimited adds a per-use
rate limit control so each line is
separately rate limited.

Another way to do this is to use the
more global net_ratelimit() before
each existing pr_<level>.

Not suggesting one or the other is
right or wrong here, it's just an option.

> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c

> @@ -2131,32 +2131,33 @@ static void bond_arp_send(struct net_device *slave_dev, int arp_op,
>  {
>  	struct sk_buff *skb;
>  
> -	pr_debug("arp %d on slave %s: dst %pI4 src %pI4\n",
> -		 arp_op, slave_dev->name, &dest_ip, &src_ip);
> +	pr_debug_ratelimited("arp %d on slave %s: dst %pI4 src %pI4\n",
> +			     arp_op, slave_dev->name, &dest_ip, &src_ip);

	if (net_ratelimit())
		pr_debug(etc...)

etc..

Re: [PATCH net-next v2 3/3] bonding: convert pr_xxx() to pr_xxx_ratelimited() for arp interval

[Joe Perches]

On Tue, 2014-03-18 at 04:01 -0700, Joe Perches wrote:
> On Tue, 2014-03-18 at 18:43 +0800, Ding Tianhong wrote:
> > The debug log in the arp interval should be rate limited, otherwise would
> > occur spam the log, so convert them.
[]
> Another way to do this is to use the
> more global net_ratelimit() before
> each existing pr_<level>.
> 
> Not suggesting one or the other is
> right or wrong here, it's just an option.

Another option is to use the net_<level>_ratelimited
functions, but I'm not sure these are used much
outside of net/.

> > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> 
> > @@ -2131,32 +2131,33 @@ static void bond_arp_send(struct net_device *slave_dev, int arp_op,
> >  {
> >  	struct sk_buff *skb;
> >  
> > -	pr_debug("arp %d on slave %s: dst %pI4 src %pI4\n",
> > -		 arp_op, slave_dev->name, &dest_ip, &src_ip);
> > +	pr_debug_ratelimited("arp %d on slave %s: dst %pI4 src %pI4\n",
> > +			     arp_op, slave_dev->name, &dest_ip, &src_ip);
> 
> 	if (net_ratelimit())
> 		pr_debug(etc...)

or

	net_dbg_ratelimited(etc...)

Re: [PATCH net-next v2 3/3] bonding: convert pr_xxx() to pr_xxx_ratelimited() for arp interval

[Ding Tianhong]

On 2014/3/18 19:14, Joe Perches wrote:
> On Tue, 2014-03-18 at 04:01 -0700, Joe Perches wrote:
>> On Tue, 2014-03-18 at 18:43 +0800, Ding Tianhong wrote:
>>> The debug log in the arp interval should be rate limited, otherwise would
>>> occur spam the log, so convert them.
> []
>> Another way to do this is to use the
>> more global net_ratelimit() before
>> each existing pr_<level>.
>>
>> Not suggesting one or the other is
>> right or wrong here, it's just an option.
> 
> Another option is to use the net_<level>_ratelimited
> functions, but I'm not sure these are used much
> outside of net/.
> 
>>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>
>>> @@ -2131,32 +2131,33 @@ static void bond_arp_send(struct net_device *slave_dev, int arp_op,
>>>  {
>>>  	struct sk_buff *skb;
>>>  
>>> -	pr_debug("arp %d on slave %s: dst %pI4 src %pI4\n",
>>> -		 arp_op, slave_dev->name, &dest_ip, &src_ip);
>>> +	pr_debug_ratelimited("arp %d on slave %s: dst %pI4 src %pI4\n",
>>> +			     arp_op, slave_dev->name, &dest_ip, &src_ip);
>>
>> 	if (net_ratelimit())
>> 		pr_debug(etc...)
> 
> or
> 
> 	net_dbg_ratelimited(etc...)
> 
> 
Thanks for these opinion. It looks like the net_ratelimit() is more reasonable.

It really need to spend time to distinguish which one is better here.
sometimes more choices more headaches. :)

Regards
Ding
> 
> 

Re: [PATCH net-next v2 2/3] bonding: support QinQ for bond arp interval

[Veaceslav Falico]

On Tue, Mar 18, 2014 at 06:43:52PM +0800, Ding Tianhong wrote:
...snip...
>-	if (vlan_id) {
>-		skb = vlan_put_tag(skb, htons(ETH_P_8021Q), vlan_id);
>+	if (outer->vlan_id) {
>+		if (inner->vlan_id) {
>+			pr_debug("inner tag: proto %X vid %X\n",
>+				 ntohs(inner->vlan_proto), inner->vlan_id);
>+			skb = __vlan_put_tag(skb, inner->vlan_proto, inner->vlan_id);
>+			if (!skb) {
>+				pr_err("failed to insert inner VLAN tag\n");
>+				return;
>+			}
>+		}
>+
>+		pr_debug("outer reg: proto %X vid %X\n",
>+			 ntohs(outer->vlan_proto), outer->vlan_id);
>+		skb = vlan_put_tag(skb, outer->vlan_proto, outer->vlan_id);

If I read correctly then the inner->vlan_proto will always be ETH_P_8021AD,
whilst the outer will also always be ETH_P_8021Q. So I think it'd be a lot
easier (and more readable) to just pass 2 vlan ids, and set those protos
statically - that will save you from adding that new function to vlan core,
fro madding a new struct that you've added here and make it several lines
less.

Re: [PATCH net-next v2 2/3] bonding: support QinQ for bond arp interval

[Jay Vosburgh]

Veaceslav Falico <vfalico@redhat.com> wrote:

>On Tue, Mar 18, 2014 at 06:43:52PM +0800, Ding Tianhong wrote:
>...snip...
>>-	if (vlan_id) {
>>-		skb = vlan_put_tag(skb, htons(ETH_P_8021Q), vlan_id);
>>+	if (outer->vlan_id) {
>>+		if (inner->vlan_id) {
>>+			pr_debug("inner tag: proto %X vid %X\n",
>>+				 ntohs(inner->vlan_proto), inner->vlan_id);
>>+			skb = __vlan_put_tag(skb, inner->vlan_proto, inner->vlan_id);
>>+			if (!skb) {
>>+				pr_err("failed to insert inner VLAN tag\n");
>>+				return;
>>+			}
>>+		}
>>+
>>+		pr_debug("outer reg: proto %X vid %X\n",
>>+			 ntohs(outer->vlan_proto), outer->vlan_id);
>>+		skb = vlan_put_tag(skb, outer->vlan_proto, outer->vlan_id);
>
>If I read correctly then the inner->vlan_proto will always be ETH_P_8021AD,
>whilst the outer will also always be ETH_P_8021Q. So I think it'd be a lot
>easier (and more readable) to just pass 2 vlan ids, and set those protos
>statically - that will save you from adding that new function to vlan core,
>fro madding a new struct that you've added here and make it several lines
>less.

	Do you mean that the outer will always be 8021AD and the inner
8021Q?  The inner/outer terminology is making my brain hurt, since the
ip commands to configure them look backwards to me.  I think of it as
the outer tag is the first one sequentially in the ethernet header, and
the inner tag is second in the header.

	Anyway, in the past, I've seen configurations with 802.1q VLANs
nested such that the inner and outer tags were both 802.1q 0x8100
ethertype, so I'm not sure that hard-coding these is necessarily a good
idea.

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: [PATCH net-next v2 2/3] bonding: support QinQ for bond arp interval

[Veaceslav Falico]

On Tue, Mar 18, 2014 at 10:59:58AM -0700, Jay Vosburgh wrote:
>Veaceslav Falico <vfalico@redhat.com> wrote:
>
>>On Tue, Mar 18, 2014 at 06:43:52PM +0800, Ding Tianhong wrote:
>>...snip...
>>>-	if (vlan_id) {
>>>-		skb = vlan_put_tag(skb, htons(ETH_P_8021Q), vlan_id);
>>>+	if (outer->vlan_id) {
>>>+		if (inner->vlan_id) {
>>>+			pr_debug("inner tag: proto %X vid %X\n",
>>>+				 ntohs(inner->vlan_proto), inner->vlan_id);
>>>+			skb = __vlan_put_tag(skb, inner->vlan_proto, inner->vlan_id);
>>>+			if (!skb) {
>>>+				pr_err("failed to insert inner VLAN tag\n");
>>>+				return;
>>>+			}
>>>+		}
>>>+
>>>+		pr_debug("outer reg: proto %X vid %X\n",
>>>+			 ntohs(outer->vlan_proto), outer->vlan_id);
>>>+		skb = vlan_put_tag(skb, outer->vlan_proto, outer->vlan_id);
>>
>>If I read correctly then the inner->vlan_proto will always be ETH_P_8021AD,
>>whilst the outer will also always be ETH_P_8021Q. So I think it'd be a lot
>>easier (and more readable) to just pass 2 vlan ids, and set those protos
>>statically - that will save you from adding that new function to vlan core,
>>fro madding a new struct that you've added here and make it several lines
>>less.
>
>	Do you mean that the outer will always be 8021AD and the inner
>8021Q?  The inner/outer terminology is making my brain hurt, since the
>ip commands to configure them look backwards to me.  I think of it as
>the outer tag is the first one sequentially in the ethernet header, and
>the inner tag is second in the header.

Yep, like that. You're right, the outer is the first, the inner is the
second. I always thought about vlans as some type of "encapsulation", with
vlan over vlan over packet meaning "packet encapsulated in an vlan,
encapsulated in a vlan", so the outer and inner vlans make sense for me :).
Though I understand this example isn't technically completely correct.

>
>	Anyway, in the past, I've seen configurations with 802.1q VLANs
>nested such that the inner and outer tags were both 802.1q 0x8100
>ethertype, so I'm not sure that hard-coding these is necessarily a good
>idea.

Yeah, you're right, seems like there are a lot of non-IEEE hacks out there,
so it's definitely better to use the proto specified by the user.

>
>	-J
>
>---
>	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com
>

Re: [PATCH net-next v2 2/3] bonding: support QinQ for bond arp interval

[Ding Tianhong]

On 2014/3/19 3:27, Veaceslav Falico wrote:
> On Tue, Mar 18, 2014 at 10:59:58AM -0700, Jay Vosburgh wrote:
>> Veaceslav Falico <vfalico@redhat.com> wrote:
>>
>>> On Tue, Mar 18, 2014 at 06:43:52PM +0800, Ding Tianhong wrote:
>>> ...snip...
>>>> -    if (vlan_id) {
>>>> -        skb = vlan_put_tag(skb, htons(ETH_P_8021Q), vlan_id);
>>>> +    if (outer->vlan_id) {
>>>> +        if (inner->vlan_id) {
>>>> +            pr_debug("inner tag: proto %X vid %X\n",
>>>> +                 ntohs(inner->vlan_proto), inner->vlan_id);
>>>> +            skb = __vlan_put_tag(skb, inner->vlan_proto, inner->vlan_id);
>>>> +            if (!skb) {
>>>> +                pr_err("failed to insert inner VLAN tag\n");
>>>> +                return;
>>>> +            }
>>>> +        }
>>>> +
>>>> +        pr_debug("outer reg: proto %X vid %X\n",
>>>> +             ntohs(outer->vlan_proto), outer->vlan_id);
>>>> +        skb = vlan_put_tag(skb, outer->vlan_proto, outer->vlan_id);
>>>
>>> If I read correctly then the inner->vlan_proto will always be ETH_P_8021AD,
>>> whilst the outer will also always be ETH_P_8021Q. So I think it'd be a lot
>>> easier (and more readable) to just pass 2 vlan ids, and set those protos
>>> statically - that will save you from adding that new function to vlan core,
>>> fro madding a new struct that you've added here and make it several lines
>>> less.
>>
>>     Do you mean that the outer will always be 8021AD and the inner
>> 8021Q?  The inner/outer terminology is making my brain hurt, since the
>> ip commands to configure them look backwards to me.  I think of it as
>> the outer tag is the first one sequentially in the ethernet header, and
>> the inner tag is second in the header.
> 
> Yep, like that. You're right, the outer is the first, the inner is the
> second. I always thought about vlans as some type of "encapsulation", with
> vlan over vlan over packet meaning "packet encapsulated in an vlan,
> encapsulated in a vlan", so the outer and inner vlans make sense for me :).
> Though I understand this example isn't technically completely correct.
> 
>>
>>     Anyway, in the past, I've seen configurations with 802.1q VLANs
>> nested such that the inner and outer tags were both 802.1q 0x8100
>> ethertype, so I'm not sure that hard-coding these is necessarily a good
>> idea.
> 
> Yeah, you're right, seems like there are a lot of non-IEEE hacks out there,
> so it's definitely better to use the proto specified by the user.
> 
>>
>>     -J
>>

Hi, Sorry for feedback so late.

The QinQ didn't mean that the first(outer) tag must be 802.1ad, and the second(inner) tag must be 802.1q,
some switch even use the double 802.1q to support QinQ, so I think it is not better to set the protos statically.

Regards:
Ding

>> ---
>>     -Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com
>>
> 
> .
> 

Re: [PATCH net-next v2 2/3] bonding: support QinQ for bond arp interval

[Veaceslav Falico]

On Wed, Mar 19, 2014 at 09:29:25AM +0800, Ding Tianhong wrote:
...snip...
>The QinQ didn't mean that the first(outer) tag must be 802.1ad, and the second(inner) tag must be 802.1q,

Do you have a quote for that from the standard? What I know and read
everywhere is that the standard specifies the outer s-tag to always be
0x88A8.

The other thing is that we don't live in an ideal world and there are a
lot of non-standard implementations out there, which might use 0x8100,
0x9100, 0x9200, 0x9300... for the outer s-tag, that's why using the
user-provided proto is better.

Re: [PATCH net-next v2 2/3] bonding: support QinQ for bond arp interval

[Ding Tianhong]

On 2014/3/19 13:39, Veaceslav Falico wrote:
> On Wed, Mar 19, 2014 at 09:29:25AM +0800, Ding Tianhong wrote:
> ...snip...
>> The QinQ didn't mean that the first(outer) tag must be 802.1ad, and the second(inner) tag must be 802.1q,
> 
> Do you have a quote for that from the standard? What I know and read
> everywhere is that the standard specifies the outer s-tag to always be
> 0x88A8.
> 
> The other thing is that we don't live in an ideal world and there are a
> lot of non-standard implementations out there, which might use 0x8100,
> 0x9100, 0x9200, 0x9300... for the outer s-tag, that's why using the
> user-provided proto is better.
> 
> 

Sorry for feedback so late, I'm too busy these days.

Agree, as far as I know, The original "QinQ" means "802.1Q in 802.1Q", but in practical applications,
the network operators could set the out tag to 0x9100, 0x8100 ... as they wished in the switch, this is no
limit, but in linux kernel the vlan only support 802.1ad and 801.1q, so we can only make choices in these values, 
so here I think let the user to decide the real proto is more reasonbale.

Regards
Ding