Skip to contentSkip to conversation

igb: Allow extra 4 bytes on RX for vlan tags.

PATCH In review [email protected] 6 participants 15 messages Feb 10, 2011 – Aug 25, 2011

Subsystems (3)
Networking Network drivers Packet sockets
lore
Share & tools
lore

[email protected]

author
#0 lore raw

PATCH 1/2 igb: Allow extra 4 bytes on RX for vlan tags.

From: Ben Greear <[email protected]>

This allows the NIC to receive 1518 byte (not counting FCS) packets when MTU is 1500, thus allowing 1500 MTU VLAN frames to be received. Please note that no VLANs were actually configured on the NIC...it was just acting as pass-through device.

Signed-off-by Ben Greear <[email protected]>

---
:100644 100644 58c665b... 30c9cc6... M drivers/net/igb/igb_main.c

 drivers/net/igb/igb_main.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
drivers/net/igb/igb_main.c +3 -2
index 58c665b..30c9cc6 100644
@@ -2281,7 +2281,8 @@ static int __devinit igb_sw_init(struct igb_adapter *adapter)
 	adapter->rx_itr_setting = IGB_DEFAULT_ITR;
 	adapter->tx_itr_setting = IGB_DEFAULT_ITR;
 
-	adapter->max_frame_size = netdev->mtu + ETH_HLEN + ETH_FCS_LEN;
+	adapter->max_frame_size = (netdev->mtu + ETH_HLEN + ETH_FCS_LEN
+				   + VLAN_HLEN);
 	adapter->min_frame_size = ETH_ZLEN + ETH_FCS_LEN;
 
 	spin_lock_init(&adapter->stats64_lock);

@@ -4303,7 +4304,7 @@ static int igb_change_mtu(struct net_device *netdev, int new_mtu)
 {
 	struct igb_adapter *adapter = netdev_priv(netdev);
 	struct pci_dev *pdev = adapter->pdev;
-	int max_frame = new_mtu + ETH_HLEN + ETH_FCS_LEN;
+	int max_frame = new_mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN;
 	u32 rx_buffer_len, i;
 
 	if ((new_mtu < 68) || (max_frame > MAX_JUMBO_FRAME_SIZE)) {
Signature · 1 line
1.7.2.3
author
#1 lore raw

PATCH 2/2 network: Allow af_packet to transmit +4 bytes for VLAN packets.

From: Ben Greear <[email protected]>

This allows user-space to send a '1500' MTU VLAN packet on a 1500 MTU ethernet frame. The extra 4 bytes of a VLAN header is not usually charged against the MTU when other parts of the network stack is transmitting vlans...

Signed-off-by Ben Greear <[email protected]>

---
:100644 100644 91cb1d7... ef7f378... M net/packet/af_packet.c

 net/packet/af_packet.c |   31 +++++++++++++++++++++++++++++--
 1 files changed, 29 insertions(+), 2 deletions(-)
net/packet/af_packet.c +29 -2
index 91cb1d7..ef7f378 100644
@@ -466,7 +466,7 @@ retry:
 	 */
 
 	err = -EMSGSIZE;
-	if (len > dev->mtu + dev->hard_header_len)
+	if (len > dev->mtu + dev->hard_header_len + VLAN_HLEN)
 		goto out_unlock;
 
 	if (!skb) {

@@ -497,6 +497,19 @@ retry:
 		goto retry;
 	}
 
+	if (len > (dev->mtu + dev->hard_header_len)) {
+		/* Earlier code assumed this would be a VLAN pkt,
+		 * double-check this now that we have the actual
+		 * packet in hand.
+		 */
+		struct ethhdr *ehdr;
+		skb_reset_mac_header(skb);
+		ehdr = eth_hdr(skb);
+		if (ehdr->h_proto != htons(ETH_P_8021Q)) {
+			err = -EMSGSIZE;
+			goto out_unlock;
+		}
+	}
 
 	skb->protocol = proto;
 	skb->dev = dev;

@@ -1200,7 +1213,7 @@ static int packet_snd(struct socket *sock,
 	}
 
 	err = -EMSGSIZE;
-	if (!gso_type && (len > dev->mtu+reserve))
+	if (!gso_type && (len > dev->mtu + reserve + ETH_HLEN))
 		goto out_unlock;
 
 	err = -ENOBUFS;

@@ -1225,6 +1238,20 @@ static int packet_snd(struct socket *sock,
 	if (err < 0)
 		goto out_free;
 
+	if (!gso_type && (len > dev->mtu + reserve)) {
+		/* Earlier code assumed this would be a VLAN pkt,
+		 * double-check this now that we have the actual
+		 * packet in hand.
+		 */
+		struct ethhdr *ehdr;
+		skb_reset_mac_header(skb);
+		ehdr = eth_hdr(skb);
+		if (ehdr->h_proto != htons(ETH_P_8021Q)) {
+			err = -EMSGSIZE;
+			goto out_free;
+		}
+	}
+
 	skb->protocol = proto;
 	skb->dev = dev;
 	skb->priority = sk->sk_priority;
Signature · 1 line
1.7.2.3
#2 lore raw

Re: [PATCH 2/2] network: Allow af_packet to transmit +4 bytes for VLAN packets.

Le jeudi 10 février 2011 à 13:59 -0800, [email protected] a écrit :

Quoted 31 lines

From: Ben Greear <[email protected]>

This allows user-space to send a '1500' MTU VLAN packet on a 1500 MTU ethernet frame. The extra 4 bytes of a VLAN header is not usually charged against the MTU when other parts of the network stack is transmitting vlans...

Signed-off-by Ben Greear <[email protected]>

---
:100644 100644 91cb1d7... ef7f378... M net/packet/af_packet.c

 net/packet/af_packet.c |   31 +++++++++++++++++++++++++++++--
 1 files changed, 29 insertions(+), 2 deletions(-)
net/packet/af_packet.c +2 -1
index 91cb1d7..ef7f378 100644
@@ -466,7 +466,7 @@ retry:
 	 */
 
 	err = -EMSGSIZE;
-	if (len > dev->mtu + dev->hard_header_len)
+	if (len > dev->mtu + dev->hard_header_len + VLAN_HLEN)
 		goto out_unlock;
 
 	if (!skb) {

@@ -497,6 +497,19 @@ retry:
 		goto retry;
 	}
 
+	if (len > (dev->mtu + dev->hard_header_len)) {

net/packet/af_packet.c retry: · line 510

Quoted 10 lines
net/packet/af_packet.c +10 -0
+		/* Earlier code assumed this would be a VLAN pkt,
+		 * double-check this now that we have the actual
+		 * packet in hand.
+		 */
+		struct ethhdr *ehdr;
+		skb_reset_mac_header(skb);
+		ehdr = eth_hdr(skb);
+		if (ehdr->h_proto != htons(ETH_P_8021Q)) {
+			err = -EMSGSIZE;
+			goto out_unlock;

This would leak skb.

Quoted 5 lines
net/packet/af_packet.c +2 -0
+		}
+	}
 
 	skb->protocol = proto;
 	skb->dev = dev;
author
#3 lore raw

Re: [PATCH 2/2] network: Allow af_packet to transmit +4 bytes for VLAN packets.

Quoted 46 lines · Eric Dumazet

Le jeudi 10 février 2011 à 13:59 -0800, [email protected] a écrit :

Quoted 31 lines

From: Ben Greear<[email protected]>

This allows user-space to send a '1500' MTU VLAN packet on a 1500 MTU ethernet frame. The extra 4 bytes of a VLAN header is not usually charged against the MTU when other parts of the network stack is transmitting vlans...

Signed-off-by Ben Greear<[email protected]>

---
:100644 100644 91cb1d7... ef7f378... M net/packet/af_packet.c

  net/packet/af_packet.c |   31 +++++++++++++++++++++++++++++--
  1 files changed, 29 insertions(+), 2 deletions(-)
net/packet/af_packet.c +2 -1
index 91cb1d7..ef7f378 100644
@@ -466,7 +466,7 @@ retry:
  	 */
 
  	err = -EMSGSIZE;
-	if (len>  dev->mtu + dev->hard_header_len)
+	if (len>  dev->mtu + dev->hard_header_len + VLAN_HLEN)
  		goto out_unlock;
 
  	if (!skb) {

@@ -497,6 +497,19 @@ retry:
  		goto retry;
  	}
 
+	if (len>  (dev->mtu + dev->hard_header_len)) {

net/packet/af_packet.c retry: · line 510

Quoted 10 lines
net/packet/af_packet.c +10 -0
+		/* Earlier code assumed this would be a VLAN pkt,
+		 * double-check this now that we have the actual
+		 * packet in hand.
+		 */
+		struct ethhdr *ehdr;
+		skb_reset_mac_header(skb);
+		ehdr = eth_hdr(skb);
+		if (ehdr->h_proto != htons(ETH_P_8021Q)) {
+			err = -EMSGSIZE;
+			goto out_unlock;

This would leak skb.

Quoted 5 lines

net/packet/af_packet.c retry: · line 515

Quoted 5 lines
net/packet/af_packet.c +2 -0
+		}
+	}
 
  	skb->protocol = proto;
  	skb->dev = dev;

Can you double-check that? Seems to me that in that method the out_unlock falls through to the out_free case. The other method is the opposite, funny enough...

Thanks,
Ben

Quoted 5 lines

--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 2 lines
Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com
#4 lore raw

Re: [PATCH 2/2] network: Allow af_packet to transmit +4 bytes for VLAN packets.

Quoted 3 lines · Ben Greear

Can you double-check that? Seems to me that in that method the out_unlock falls through to the out_free case. The other method is the opposite, funny enough...

Oops you're right, sorry.

Reading again your patch, I see you used one ETH_HLEN instead of VLAN_HLEN in packet_snd()

#5 lore raw
In reply to [email protected]
Quoted 8 lines · On Thu, Feb 10, 2011 at 13:59, <[email protected]> wrote:

From: Ben Greear <[email protected]>

This allows the NIC to receive 1518 byte (not counting FCS) packets when MTU is 1500, thus allowing 1500 MTU VLAN frames to be received.  Please note that no VLANs were actually configured on the NIC...it was just acting as pass-through device.

Quoted 6 lines
Signed-off-by Ben Greear <[email protected]>

---
:100644 100644 58c665b... 30c9cc6... M  drivers/net/igb/igb_main.c drivers/net/igb/igb_main.c |    5 +++--

 1 files changed, 3 insertions(+), 2 deletions(-)

Thanks Ben, I have added the patch to my queue of igb patches.

Signature · 2 lines
Cheers,
Jeff
#6 lore raw
In reply to [email protected]
Quoted 29 lines · On Thu, Feb 10, 2011 at 13:59, <[email protected]> wrote:

From: Ben Greear <[email protected]>

This allows the NIC to receive 1518 byte (not counting FCS) packets when MTU is 1500, thus allowing 1500 MTU VLAN frames to be received.  Please note that no VLANs were actually configured on the NIC...it was just acting as pass-through device.

Signed-off-by Ben Greear <[email protected]>

---
:100644 100644 58c665b... 30c9cc6... M  drivers/net/igb/igb_main.c drivers/net/igb/igb_main.c |    5 +++--

 1 files changed, 3 insertions(+), 2 deletions(-)
drivers/net/igb/igb_main.c +2 -1
index 58c665b..30c9cc6 100644
@@ -2281,7 +2281,8 @@ static int __devinit igb_sw_init(struct igb_adapter *adapter)
        adapter->rx_itr_setting = IGB_DEFAULT_ITR;
        adapter->tx_itr_setting = IGB_DEFAULT_ITR;
 
-       adapter->max_frame_size = netdev->mtu + ETH_HLEN + ETH_FCS_LEN;
+       adapter->max_frame_size = (netdev->mtu + ETH_HLEN + ETH_FCS_LEN
+                                  + VLAN_HLEN);
        adapter->min_frame_size = ETH_ZLEN + ETH_FCS_LEN;
 
        spin_lock_init(&adapter->stats64_lock);

drivers/net/igb/igb_main.c static int igb_change_mtu(struct net_device *netdev, int new_mtu) · line 4310

Quoted 8 lines
drivers/net/igb/igb_main.c +1 -1
@@ -4303,7 +4304,7 @@ static int igb_change_mtu(struct net_device *netdev, int new_mtu)
  {
        struct igb_adapter *adapter = netdev_priv(netdev);
        struct pci_dev *pdev = adapter->pdev;
-       int max_frame = new_mtu + ETH_HLEN + ETH_FCS_LEN;
+       int max_frame = new_mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN;
        u32 rx_buffer_len, i;
 
        if ((new_mtu < 68) || (max_frame > MAX_JUMBO_FRAME_SIZE)) {

While testing this patch, validation found that the patch reduces the maximum mtu size
by 4 bytes (reduces it from 9216 to 9212). This is not a desired side effect of this patch.

Thoughts?

Signature · 2 lines
Cheers,
Jeff
author
#7 lore raw
Quoted 37 lines · Jeff Kirsher
Quoted 36 lines · On Thu, Feb 10, 2011 at 13:59,<[email protected]> wrote:

From: Ben Greear<[email protected]>

This allows the NIC to receive 1518 byte (not counting FCS) packets when MTU is 1500, thus allowing 1500 MTU VLAN frames to be received. Please note that no VLANs were actually configured on the NIC...it was just acting as pass-through device.

Signed-off-by Ben Greear<[email protected]>

---
:100644 100644 58c665b... 30c9cc6... M drivers/net/igb/igb_main.c

  drivers/net/igb/igb_main.c |    5 +++--
  1 files changed, 3 insertions(+), 2 deletions(-)
drivers/net/igb/igb_main.c +3 -2
index 58c665b..30c9cc6 100644
@@ -2281,7 +2281,8 @@ static int __devinit igb_sw_init(struct igb_adapter *adapter)
        adapter->rx_itr_setting = IGB_DEFAULT_ITR;
        adapter->tx_itr_setting = IGB_DEFAULT_ITR;
 
-       adapter->max_frame_size = netdev->mtu + ETH_HLEN + ETH_FCS_LEN;
+       adapter->max_frame_size = (netdev->mtu + ETH_HLEN + ETH_FCS_LEN
+                                  + VLAN_HLEN);
        adapter->min_frame_size = ETH_ZLEN + ETH_FCS_LEN;
 
        spin_lock_init(&adapter->stats64_lock);

@@ -4303,7 +4304,7 @@ static int igb_change_mtu(struct net_device *netdev, int new_mtu)
  {
        struct igb_adapter *adapter = netdev_priv(netdev);
        struct pci_dev *pdev = adapter->pdev;
-       int max_frame = new_mtu + ETH_HLEN + ETH_FCS_LEN;
+       int max_frame = new_mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN;
        u32 rx_buffer_len, i;
Quoted 6 lines

drivers/net/igb/igb_main.c static int igb_change_mtu(struct net_device *netdev, int new_mtu) · line 4310

Quoted 1 line
drivers/net/igb/igb_main.c +0 -0
        if ((new_mtu<  68) || (max_frame>  MAX_JUMBO_FRAME_SIZE)) {

While testing this patch, validation found that the patch reduces the maximum mtu size
by 4 bytes (reduces it from 9216 to 9212). This is not a desired side effect of this patch.

You could add handling for that case and have it act as it used to when new_mtu is greater than 9212?

I tested e1000e and it worked w/out hacking at 1500 MTU, so maybe check how it does it?

Thanks,
Ben

Signature · 2 lines
Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com
#8 lore raw
Quoted 49 lines · Ben Greear
Quoted 41 lines · Jeff Kirsher
Quoted 39 lines · On Thu, Feb 10, 2011 at 13:59,<[email protected]>  wrote:

From: Ben Greear<[email protected]>

This allows the NIC to receive 1518 byte (not counting FCS) packets when MTU is 1500, thus allowing 1500 MTU VLAN frames to be received.  Please note that no VLANs were actually configured on the NIC...it was just acting as pass-through device.

Signed-off-by Ben Greear<[email protected]>

---
:100644 100644 58c665b... 30c9cc6... M  drivers/net/igb/igb_main.c drivers/net/igb/igb_main.c |    5 +++--

 1 files changed, 3 insertions(+), 2 deletions(-)
drivers/net/igb/igb_main.c +2 -1
index 58c665b..30c9cc6 100644
@@ -2281,7 +2281,8 @@ static int __devinit igb_sw_init(struct igb_adapter
 *adapter)
        adapter->rx_itr_setting = IGB_DEFAULT_ITR;
        adapter->tx_itr_setting = IGB_DEFAULT_ITR;
 
-       adapter->max_frame_size = netdev->mtu + ETH_HLEN + ETH_FCS_LEN;
+       adapter->max_frame_size = (netdev->mtu + ETH_HLEN + ETH_FCS_LEN
+                                  + VLAN_HLEN);
        adapter->min_frame_size = ETH_ZLEN + ETH_FCS_LEN;

       spin_lock_init(&adapter->stats64_lock);

drivers/net/igb/igb_main.c +1 -1
@@ -4303,7 +4304,7 @@ static int igb_change_mtu(struct net_device
 *netdev, int new_mtu)
  {
        struct igb_adapter *adapter = netdev_priv(netdev);
        struct pci_dev *pdev = adapter->pdev;
-       int max_frame = new_mtu + ETH_HLEN + ETH_FCS_LEN;
+       int max_frame = new_mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN;
        u32 rx_buffer_len, i;
Quoted 6 lines
Quoted 1 line

       if ((new_mtu<  68) || (max_frame>  MAX_JUMBO_FRAME_SIZE)) {

While testing this patch, validation found that the patch reduces the maximum mtu size
by 4 bytes (reduces it from 9216 to 9212).  This is not a desired side effect of this patch.

Quoted 5 lines

You could add handling for that case and have it act as it used to when new_mtu is greater than 9212?

I tested e1000e and it worked w/out hacking at 1500 MTU, so maybe check how it does it?

I just wanted to bring this up again to see if any progress had been made. We were looking at this driver and trying to figure out the best way to convert it to use the new vlan model but I'm not familiar enough with the hardware to know. It seems that all of the other Intel drivers unconditionally add space for the vlan tag to the receive buffer (and would therefore have similar effects as this patch), is there something different about this card?

I believe that Alex was working on something in this area (in the context of one of my patches from a long time ago) but I'm not sure what came of that.

author
#9 lore raw
Quoted 53 lines · Jesse Gross
Quoted 52 lines · Ben Greear
Quoted 41 lines · Jeff Kirsher
Quoted 39 lines · On Thu, Feb 10, 2011 at 13:59,<[email protected]> wrote:

From: Ben Greear<[email protected]>

This allows the NIC to receive 1518 byte (not counting FCS) packets when MTU is 1500, thus allowing 1500 MTU VLAN frames to be received. Please note that no VLANs were actually configured on the NIC...it was just acting as pass-through device.

Signed-off-by Ben Greear<[email protected]>

---
:100644 100644 58c665b... 30c9cc6... M drivers/net/igb/igb_main.c

  drivers/net/igb/igb_main.c |    5 +++--
  1 files changed, 3 insertions(+), 2 deletions(-)
drivers/net/igb/igb_main.c +2 -1
index 58c665b..30c9cc6 100644
@@ -2281,7 +2281,8 @@ static int __devinit igb_sw_init(struct igb_adapter
 *adapter)
        adapter->rx_itr_setting = IGB_DEFAULT_ITR;
        adapter->tx_itr_setting = IGB_DEFAULT_ITR;
 
-       adapter->max_frame_size = netdev->mtu + ETH_HLEN + ETH_FCS_LEN;
+       adapter->max_frame_size = (netdev->mtu + ETH_HLEN + ETH_FCS_LEN
+                                  + VLAN_HLEN);
        adapter->min_frame_size = ETH_ZLEN + ETH_FCS_LEN;
 

        spin_lock_init(&adapter->stats64_lock);
drivers/net/igb/igb_main.c +1 -1
@@ -4303,7 +4304,7 @@ static int igb_change_mtu(struct net_device
 *netdev, int new_mtu)
  {
        struct igb_adapter *adapter = netdev_priv(netdev);
        struct pci_dev *pdev = adapter->pdev;
-       int max_frame = new_mtu + ETH_HLEN + ETH_FCS_LEN;
+       int max_frame = new_mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN;
        u32 rx_buffer_len, i;
Quoted 6 lines
Quoted 1 line
        if ((new_mtu<    68) || (max_frame>    MAX_JUMBO_FRAME_SIZE)) {

While testing this patch, validation found that the patch reduces the maximum mtu size
by 4 bytes (reduces it from 9216 to 9212). This is not a desired side effect of this patch.

You could add handling for that case and have it act as it used to when new_mtu is greater than 9212?

Quoted 6 lines
Quoted 2 lines

I tested e1000e and it worked w/out hacking at 1500 MTU, so maybe check how it does it?

I just wanted to bring this up again to see if any progress had been made. We were looking at this driver and trying to figure out the best way to convert it to use the new vlan model but I'm not familiar

I've been watching :)

Quoted 8 lines

enough with the hardware to know. It seems that all of the other Intel drivers unconditionally add space for the vlan tag to the receive buffer (and would therefore have similar effects as this patch), is there something different about this card?

I believe that Alex was working on something in this area (in the context of one of my patches from a long time ago) but I'm not sure what came of that.

Truth is, I don't really see why it's a problem to decrease the maximum MTU slightly in order to make it work with VLANs.

I'm not sure if there is some way to make it work with VLANs and not decrease the maximum MTU.

Thanks,
Ben

Signature · 2 lines
Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com
#10 lore raw
Quoted 72 lines · Ben Greear
Quoted 53 lines · Jesse Gross
Quoted 52 lines · Ben Greear
Quoted 41 lines · Jeff Kirsher
Quoted 39 lines · On Thu, Feb 10, 2011 at 13:59,<[email protected]> wrote:

From: Ben Greear<[email protected]>

This allows the NIC to receive 1518 byte (not counting FCS) packets when MTU is 1500, thus allowing 1500 MTU VLAN frames to be received. Please note that no VLANs were actually configured on the NIC...it was just acting as pass-through device.

Signed-off-by Ben Greear<[email protected]>

---
:100644 100644 58c665b... 30c9cc6... M drivers/net/igb/igb_main.c

  drivers/net/igb/igb_main.c |    5 +++--
  1 files changed, 3 insertions(+), 2 deletions(-)
drivers/net/igb/igb_main.c +2 -1
index 58c665b..30c9cc6 100644
@@ -2281,7 +2281,8 @@ static int __devinit igb_sw_init(struct igb_adapter
 *adapter)
        adapter->rx_itr_setting = IGB_DEFAULT_ITR;
        adapter->tx_itr_setting = IGB_DEFAULT_ITR;
 
-       adapter->max_frame_size = netdev->mtu + ETH_HLEN + ETH_FCS_LEN;
+       adapter->max_frame_size = (netdev->mtu + ETH_HLEN + ETH_FCS_LEN
+                                  + VLAN_HLEN);
        adapter->min_frame_size = ETH_ZLEN + ETH_FCS_LEN;
 

        spin_lock_init(&adapter->stats64_lock);
drivers/net/igb/igb_main.c +1 -1
@@ -4303,7 +4304,7 @@ static int igb_change_mtu(struct net_device
 *netdev, int new_mtu)
  {
        struct igb_adapter *adapter = netdev_priv(netdev);
        struct pci_dev *pdev = adapter->pdev;
-       int max_frame = new_mtu + ETH_HLEN + ETH_FCS_LEN;
+       int max_frame = new_mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN;
        u32 rx_buffer_len, i;
Quoted 6 lines
Quoted 1 line
        if ((new_mtu<    68) || (max_frame>    MAX_JUMBO_FRAME_SIZE)) {

While testing this patch, validation found that the patch reduces the maximum mtu size
by 4 bytes (reduces it from 9216 to 9212). This is not a desired side effect of this patch.

You could add handling for that case and have it act as it used to when new_mtu is greater than 9212?

Quoted 6 lines
Quoted 2 lines

I tested e1000e and it worked w/out hacking at 1500 MTU, so maybe check how it does it?

I just wanted to bring this up again to see if any progress had been made. We were looking at this driver and trying to figure out the best way to convert it to use the new vlan model but I'm not familiar

I've been watching :)

Quoted 8 lines

enough with the hardware to know. It seems that all of the other Intel drivers unconditionally add space for the vlan tag to the receive buffer (and would therefore have similar effects as this patch), is there something different about this card?

I believe that Alex was working on something in this area (in the context of one of my patches from a long time ago) but I'm not sure what came of that.

Quoted 5 lines

Truth is, I don't really see why it's a problem to decrease the maximum MTU slightly in order to make it work with VLANs.

I'm not sure if there is some way to make it work with VLANs and not decrease the maximum MTU.

This was the reason this did not get accepted. I was looking into what could be done so that we did not decease the maximum MTU, but I got side-tracked and have not done anything on it in several months.

#11 lore raw
Quoted 76 lines · Jeff Kirsher
Quoted 75 lines · Ben Greear
Quoted 53 lines · Jesse Gross
Quoted 52 lines · Ben Greear
Quoted 41 lines · Jeff Kirsher
Quoted 39 lines · On Thu, Feb 10, 2011 at 13:59,<[email protected]>    wrote:

From: Ben Greear<[email protected]>

This allows the NIC to receive 1518 byte (not counting FCS) packets when MTU is 1500, thus allowing 1500 MTU VLAN frames to be received.  Please note that no VLANs were actually configured on the NIC...it was just acting as pass-through device.

Signed-off-by Ben Greear<[email protected]>

---
:100644 100644 58c665b... 30c9cc6... M  drivers/net/igb/igb_main.c drivers/net/igb/igb_main.c |    5 +++--

  1 files changed, 3 insertions(+), 2 deletions(-)
drivers/net/igb/igb_main.c +2 -1
index 58c665b..30c9cc6 100644
@@ -2281,7 +2281,8 @@ static int __devinit igb_sw_init(struct igb_adapter
 *adapter)
         adapter->rx_itr_setting = IGB_DEFAULT_ITR;
         adapter->tx_itr_setting = IGB_DEFAULT_ITR;
 
-       adapter->max_frame_size = netdev->mtu + ETH_HLEN + ETH_FCS_LEN;
+       adapter->max_frame_size = (netdev->mtu + ETH_HLEN + ETH_FCS_LEN
+                                  + VLAN_HLEN);
         adapter->min_frame_size = ETH_ZLEN + ETH_FCS_LEN;

        spin_lock_init(&adapter->stats64_lock);

drivers/net/igb/igb_main.c +1 -1
@@ -4303,7 +4304,7 @@ static int igb_change_mtu(struct net_device
 *netdev, int new_mtu)
   {
         struct igb_adapter *adapter = netdev_priv(netdev);
         struct pci_dev *pdev = adapter->pdev;
-       int max_frame = new_mtu + ETH_HLEN + ETH_FCS_LEN;
+       int max_frame = new_mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN;
         u32 rx_buffer_len, i;
Quoted 6 lines
Quoted 1 line

        if ((new_mtu<    68) || (max_frame>    MAX_JUMBO_FRAME_SIZE)) {

While testing this patch, validation found that the patch reduces the maximum mtu size
by 4 bytes (reduces it from 9216 to 9212).  This is not a desired side effect of this patch.

You could add handling for that case and have it act as it used to when new_mtu is greater than 9212?

Quoted 6 lines
Quoted 2 lines

I tested e1000e and it worked w/out hacking at 1500 MTU, so maybe check how it does it?

I just wanted to bring this up again to see if any progress had been made.  We were looking at this driver and trying to figure out the best way to convert it to use the new vlan model but I'm not familiar

I've been watching :)

Quoted 8 lines

enough with the hardware to know.  It seems that all of the other Intel drivers unconditionally add space for the vlan tag to the receive buffer (and would therefore have similar effects as this patch), is there something different about this card?

I believe that Alex was working on something in this area (in the context of one of my patches from a long time ago) but I'm not sure what came of that.

Truth is, I don't really see why it's a problem to decrease the maximum MTU slightly in order to make it work with VLANs.

Quoted 7 lines
Quoted 2 lines

I'm not sure if there is some way to make it work with VLANs and not decrease the maximum MTU.

This was the reason this did not get accepted.  I was looking into what could be done so that we did not decease the maximum MTU, but I got side-tracked and have not done anything on it in several months.

I can take a look at fixing this most likely tomorrow. I have some work planned for igb anyway over the next few days.

Odds are it is just a matter of where the VLAN_HLEN is added. As I recall for our drivers the correct spot is in the setting of rx_buffer_len since that is the area more concerned with maximum receive frame size versus the mtu section which is more concerned with the transmit side of things.

Thanks,

Alex

#12 lore raw
Quoted 86 lines · Alexander Duyck
Quoted 76 lines · Jeff Kirsher
Quoted 75 lines · Ben Greear
Quoted 53 lines · Jesse Gross
Quoted 52 lines · Ben Greear
Quoted 41 lines · Jeff Kirsher
Quoted 39 lines · On Thu, Feb 10, 2011 at 13:59,<[email protected]>    wrote:

From: Ben Greear<[email protected]>

This allows the NIC to receive 1518 byte (not counting FCS) packets when MTU is 1500, thus allowing 1500 MTU VLAN frames to be received.  Please note that no VLANs were actually configured on the NIC...it was just acting as pass-through device.

Signed-off-by Ben Greear<[email protected]>

---
:100644 100644 58c665b... 30c9cc6... M  drivers/net/igb/igb_main.c drivers/net/igb/igb_main.c |    5 +++--

  1 files changed, 3 insertions(+), 2 deletions(-)
drivers/net/igb/igb_main.c +2 -1
index 58c665b..30c9cc6 100644
@@ -2281,7 +2281,8 @@ static int __devinit igb_sw_init(struct igb_adapter
 *adapter)
         adapter->rx_itr_setting = IGB_DEFAULT_ITR;
         adapter->tx_itr_setting = IGB_DEFAULT_ITR;
 
-       adapter->max_frame_size = netdev->mtu + ETH_HLEN + ETH_FCS_LEN;
+       adapter->max_frame_size = (netdev->mtu + ETH_HLEN + ETH_FCS_LEN
+                                  + VLAN_HLEN);
         adapter->min_frame_size = ETH_ZLEN + ETH_FCS_LEN;

        spin_lock_init(&adapter->stats64_lock);

drivers/net/igb/igb_main.c +1 -1
@@ -4303,7 +4304,7 @@ static int igb_change_mtu(struct net_device
 *netdev, int new_mtu)
   {
         struct igb_adapter *adapter = netdev_priv(netdev);
         struct pci_dev *pdev = adapter->pdev;
-       int max_frame = new_mtu + ETH_HLEN + ETH_FCS_LEN;
+       int max_frame = new_mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN;
         u32 rx_buffer_len, i;
Quoted 6 lines
Quoted 1 line

        if ((new_mtu<    68) || (max_frame>    MAX_JUMBO_FRAME_SIZE)) {

While testing this patch, validation found that the patch reduces the maximum mtu size
by 4 bytes (reduces it from 9216 to 9212).  This is not a desired side effect of this patch.

You could add handling for that case and have it act as it used to when new_mtu is greater than 9212?

Quoted 6 lines
Quoted 2 lines

I tested e1000e and it worked w/out hacking at 1500 MTU, so maybe check how it does it?

I just wanted to bring this up again to see if any progress had been made.  We were looking at this driver and trying to figure out the best way to convert it to use the new vlan model but I'm not familiar

I've been watching :)

Quoted 8 lines

enough with the hardware to know.  It seems that all of the other Intel drivers unconditionally add space for the vlan tag to the receive buffer (and would therefore have similar effects as this patch), is there something different about this card?

I believe that Alex was working on something in this area (in the context of one of my patches from a long time ago) but I'm not sure what came of that.

Truth is, I don't really see why it's a problem to decrease the maximum MTU slightly in order to make it work with VLANs.

Quoted 7 lines
Quoted 2 lines

I'm not sure if there is some way to make it work with VLANs and not decrease the maximum MTU.

This was the reason this did not get accepted.  I was looking into what could be done so that we did not decease the maximum MTU, but I got side-tracked and have not done anything on it in several months.

Quoted 8 lines

I can take a look at fixing this most likely tomorrow.  I have some work planned for igb anyway over the next few days.

Odds are it is just a matter of where the VLAN_HLEN is added.  As I recall for our drivers the correct spot is in the setting of rx_buffer_len since that is the area more concerned with maximum receive frame size versus the mtu section which is more concerned with the transmit side of things.

Thanks Alex.

author
#13 lore raw
In reply to Alexander Duyck
Quoted 86 lines · Alexander Duyck
Quoted 76 lines · Jeff Kirsher
Quoted 75 lines · Ben Greear
Quoted 53 lines · Jesse Gross
Quoted 52 lines · Ben Greear
Quoted 41 lines · Jeff Kirsher
Quoted 39 lines · On Thu, Feb 10, 2011 at 13:59,<[email protected]> wrote:

From: Ben Greear<[email protected]>

This allows the NIC to receive 1518 byte (not counting FCS) packets when MTU is 1500, thus allowing 1500 MTU VLAN frames to be received. Please note that no VLANs were actually configured on the NIC...it was just acting as pass-through device.

Signed-off-by Ben Greear<[email protected]>

---
:100644 100644 58c665b... 30c9cc6... M drivers/net/igb/igb_main.c

   drivers/net/igb/igb_main.c |    5 +++--
   1 files changed, 3 insertions(+), 2 deletions(-)
drivers/net/igb/igb_main.c +2 -1
index 58c665b..30c9cc6 100644
@@ -2281,7 +2281,8 @@ static int __devinit igb_sw_init(struct igb_adapter
 *adapter)
         adapter->rx_itr_setting = IGB_DEFAULT_ITR;
         adapter->tx_itr_setting = IGB_DEFAULT_ITR;
 
-       adapter->max_frame_size = netdev->mtu + ETH_HLEN + ETH_FCS_LEN;
+       adapter->max_frame_size = (netdev->mtu + ETH_HLEN + ETH_FCS_LEN
+                                  + VLAN_HLEN);
         adapter->min_frame_size = ETH_ZLEN + ETH_FCS_LEN;
 

         spin_lock_init(&adapter->stats64_lock);
drivers/net/igb/igb_main.c +1 -1
@@ -4303,7 +4304,7 @@ static int igb_change_mtu(struct net_device
 *netdev, int new_mtu)
   {
         struct igb_adapter *adapter = netdev_priv(netdev);
         struct pci_dev *pdev = adapter->pdev;
-       int max_frame = new_mtu + ETH_HLEN + ETH_FCS_LEN;
+       int max_frame = new_mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN;
         u32 rx_buffer_len, i;
Quoted 6 lines
Quoted 1 line
         if ((new_mtu<      68) || (max_frame>      MAX_JUMBO_FRAME_SIZE)) {

While testing this patch, validation found that the patch reduces the maximum mtu size
by 4 bytes (reduces it from 9216 to 9212). This is not a desired side effect of this patch.

You could add handling for that case and have it act as it used to when new_mtu is greater than 9212?

Quoted 6 lines
Quoted 2 lines

I tested e1000e and it worked w/out hacking at 1500 MTU, so maybe check how it does it?

I just wanted to bring this up again to see if any progress had been made. We were looking at this driver and trying to figure out the best way to convert it to use the new vlan model but I'm not familiar

I've been watching :)

Quoted 8 lines

enough with the hardware to know. It seems that all of the other Intel drivers unconditionally add space for the vlan tag to the receive buffer (and would therefore have similar effects as this patch), is there something different about this card?

I believe that Alex was working on something in this area (in the context of one of my patches from a long time ago) but I'm not sure what came of that.

Truth is, I don't really see why it's a problem to decrease the maximum MTU slightly in order to make it work with VLANs.

Quoted 7 lines
Quoted 2 lines

I'm not sure if there is some way to make it work with VLANs and not decrease the maximum MTU.

This was the reason this did not get accepted. I was looking into what could be done so that we did not decease the maximum MTU, but I got side-tracked and have not done anything on it in several months.

Quoted 8 lines

I can take a look at fixing this most likely tomorrow. I have some work planned for igb anyway over the next few days.

Odds are it is just a matter of where the VLAN_HLEN is added. As I recall for our drivers the correct spot is in the setting of rx_buffer_len since that is the area more concerned with maximum receive frame size versus the mtu section which is more concerned with the transmit side of things.

Did a patch for this ever get posted? I'll be happy to test it if so...

Thanks,
Ben

Signature · 2 lines
Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com
#14 lore raw
Quoted 101 lines · Ben Greear
Quoted 91 lines · Alexander Duyck
Quoted 81 lines · Jeff Kirsher
Quoted 80 lines · Ben Greear
Quoted 58 lines · Jesse Gross
Quoted 57 lines · Ben Greear
Quoted 45 lines · Jeff Kirsher
Quoted 43 lines · On Thu, Feb 10, 2011 at 13:59,<[email protected]> wrote:

From: Ben Greear<[email protected]>

This allows the NIC to receive 1518 byte (not counting FCS) packets when MTU is 1500, thus allowing 1500 MTU VLAN frames to be received. Please note that no VLANs were actually configured on the NIC...it was just acting as pass-through device.

Signed-off-by Ben Greear<[email protected]>

---
:100644 100644 58c665b... 30c9cc6... M drivers/net/igb/igb_main.c

   drivers/net/igb/igb_main.c |    5 +++--
   1 files changed, 3 insertions(+), 2 deletions(-)
drivers/net/igb/igb_main.c +0 -0
index 58c665b..30c9cc6 100644
@@ -2281,7 +2281,8 @@ static int __devinit igb_sw_init(struct

igb_adapter
*adapter)

         adapter->rx_itr_setting = IGB_DEFAULT_ITR;
         adapter->tx_itr_setting = IGB_DEFAULT_ITR;

- adapter->max_frame_size = netdev->mtu + ETH_HLEN +
ETH_FCS_LEN;
+ adapter->max_frame_size = (netdev->mtu + ETH_HLEN +
ETH_FCS_LEN

drivers/net/igb/igb_main.c +2 -1
+                                  + VLAN_HLEN);
         adapter->min_frame_size = ETH_ZLEN + ETH_FCS_LEN;
 
         spin_lock_init(&adapter->stats64_lock);

@@ -4303,7 +4304,7 @@ static int igb_change_mtu(struct net_device
 *netdev, int new_mtu)
   {
         struct igb_adapter *adapter = netdev_priv(netdev);
         struct pci_dev *pdev = adapter->pdev;
-       int max_frame = new_mtu + ETH_HLEN + ETH_FCS_LEN;
+       int max_frame = new_mtu + ETH_HLEN + ETH_FCS_LEN + 
 VLAN_HLEN;
         u32 rx_buffer_len, i;
Quoted 7 lines
Quoted 2 lines
         if ((new_mtu<      68) || (max_frame>

MAX_JUMBO_FRAME_SIZE)) {

While testing this patch, validation found that the patch reduces the maximum mtu size
by 4 bytes (reduces it from 9216 to 9212). This is not a desired side effect of this patch.

You could add handling for that case and have it act as it used to when new_mtu is greater than 9212?

Quoted 6 lines
Quoted 2 lines

I tested e1000e and it worked w/out hacking at 1500 MTU, so maybe check how it does it?

I just wanted to bring this up again to see if any progress had been made. We were looking at this driver and trying to figure out the best way to convert it to use the new vlan model but I'm not familiar

I've been watching :)

Quoted 8 lines

enough with the hardware to know. It seems that all of the other Intel drivers unconditionally add space for the vlan tag to the receive buffer (and would therefore have similar effects as this patch), is there something different about this card?

I believe that Alex was working on something in this area (in the context of one of my patches from a long time ago) but I'm not sure what came of that.

Truth is, I don't really see why it's a problem to decrease the maximum MTU slightly in order to make it work with VLANs.

Quoted 7 lines
Quoted 2 lines

I'm not sure if there is some way to make it work with VLANs and not decrease the maximum MTU.

This was the reason this did not get accepted. I was looking into what could be done so that we did not decease the maximum MTU, but I got side-tracked and have not done anything on it in several months.

Quoted 8 lines

I can take a look at fixing this most likely tomorrow. I have some work planned for igb anyway over the next few days.

Odds are it is just a matter of where the VLAN_HLEN is added. As I recall for our drivers the correct spot is in the setting of rx_buffer_len since that is the area more concerned with maximum receive frame size versus the mtu section which is more concerned with the transmit side of things.

Quoted 6 lines

Did a patch for this ever get posted? I'll be happy to test it if so...

Thanks,
Ben

We haven't posted one yet. I have one written up but it is currently mixed in with a set of 30 patches that I am testing/cleaning up/formatting before submitting to our formal validation team. I will likely be submitting it to Jeff Kirsher sometime next week and the patches will probably be available a few weeks after that.

Thanks,

Alex