[PATCH] [NIU] VLAN does not work with niu driver

[PATCH] [NIU] VLAN does not work with niu driver

[Joyce Yu]

[-- Attachment #1: Type: text/plain, Size: 6 bytes --]


-- 


[-- Attachment #2: 0001-VLAN-does-not-work-with-niu-driver.patch --]
[-- Type: text/x-patch, Size: 1257 bytes --]

>From 0bb77e878758bd72051577bcc568e2b95c87c203 Mon Sep 17 00:00:00 2001
From: Joyce Yu <joyce.yu@sun.com>
Date: Mon, 12 Oct 2009 11:03:54 -0700
Subject: [PATCH] VLAN does not work with niu driver

---
 drivers/net/niu.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/drivers/net/niu.c b/drivers/net/niu.c
index f9364d0..9559e42 100644
--- a/drivers/net/niu.c
+++ b/drivers/net/niu.c
@@ -3480,6 +3480,7 @@ static int niu_process_rx_pkt(struct napi_struct *napi, struct niu *np,
 	unsigned int index = rp->rcr_index;
 	struct sk_buff *skb;
 	int len, num_rcr;
+	struct vlan_ethhdr *veth;
 
 	skb = netdev_alloc_skb(np->dev, RX_SKB_ALLOC_SIZE);
 	if (unlikely(!skb))
@@ -3545,7 +3546,15 @@ static int niu_process_rx_pkt(struct napi_struct *napi, struct niu *np,
 	rp->rcr_index = index;
 
 	skb_reserve(skb, NET_IP_ALIGN);
-	__pskb_pull_tail(skb, min(len, NIU_RXPULL_MAX));
+	__pskb_pull_tail(skb, min(len, VLAN_ETH_HLEN));
+
+	veth = (struct vlan_ethhdr *)skb->data;
+	if (veth->h_vlan_proto != __constant_htons(ETH_P_8021Q)) {
+		skb->tail -= 4;
+		skb->data_len += 4;
+		skb_shinfo(skb)->frags[0].page_offset -= 4;
+		skb_shinfo(skb)->frags[0].size += 4;
+	}
 
 	rp->rx_packets++;
 	rp->rx_bytes += skb->len;
-- 
1.6.4

Re: [PATCH] [NIU] VLAN does not work with niu driver

[David Miller]

From: Joyce Yu <Joyce.Yu@Sun.COM>
Date: Tue, 13 Oct 2009 14:27:27 -0700

> @@ -3545,7 +3546,15 @@ static int niu_process_rx_pkt(struct napi_struct *napi, struct niu *np,
>  	rp->rcr_index = index;
>  
>  	skb_reserve(skb, NET_IP_ALIGN);
> -	__pskb_pull_tail(skb, min(len, NIU_RXPULL_MAX));
> +	__pskb_pull_tail(skb, min(len, VLAN_ETH_HLEN));
> +
> +	veth = (struct vlan_ethhdr *)skb->data;
> +	if (veth->h_vlan_proto != __constant_htons(ETH_P_8021Q)) {
> +		skb->tail -= 4;
> +		skb->data_len += 4;
> +		skb_shinfo(skb)->frags[0].page_offset -= 4;
> +		skb_shinfo(skb)->frags[0].size += 4;
> +	}

So you guys spent an enormous amount of time trying to figure out
why just a plain '__pskb_pull_tail(skb, min(len, VLAN_ETH_HLEN));'
doesn't work.

Don't you think the full details of your discovery might be relevant
either in the commit message or a comment?

Otherwise, how is anyone else in the world going to figure out why you
had to do these frag list adjustments?

Re: [PATCH] [NIU] VLAN does not work with niu driver

[David Miller]

If you send this patch improperly any more times, I'm simply just
going to start ignoring your patch postings completely.

First of all, you did not add a proper commit message with your patch
explaining your change, in detail.  I've asked you for this not ONCE,
but TWICE.

You also did not provide a proper Signed-off-by: tag for your change,
please read linux/Documentation/SubmittingPatches for details.

Every time you submit a patch improperly, you waste a lot of my time.

I have to handle receiving patches from hundreds of people every day,
I don't have time to teach each and every one of them how to submit
things properly.  I especially don't have time to do it MULTIPLE
TIMES, like I have been doing for you.

[PATCH] [NIU] VLAN does not work with niu driver

[Joyce Yu]

[-- Attachment #1: Type: text/plain, Size: 6 bytes --]


-- 


[-- Attachment #2: 0001-VLAN_ETH_HLEN-should-be-used-to-make-sure-that-the-w.patch --]
[-- Type: text/x-patch, Size: 858 bytes --]

>From f301748d3156437d65305f14288c7d5711861980 Mon Sep 17 00:00:00 2001
From: Joyce Yu <joyce.yu@sun.com>
Date: Wed, 21 Oct 2009 05:35:46 -0700
Subject: [PATCH] VLAN_ETH_HLEN should be used to make sure that the whole MAC header was copied to the head buffer in the Vlan packets case
Signed-off-by: Joyce Yu <joyce.yu@sun.com>

---
 drivers/net/niu.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/niu.c b/drivers/net/niu.c
index f9364d0..d6c7ac6 100644
--- a/drivers/net/niu.c
+++ b/drivers/net/niu.c
@@ -3545,7 +3545,7 @@ static int niu_process_rx_pkt(struct napi_struct *napi, struct niu *np,
 	rp->rcr_index = index;
 
 	skb_reserve(skb, NET_IP_ALIGN);
-	__pskb_pull_tail(skb, min(len, NIU_RXPULL_MAX));
+	__pskb_pull_tail(skb, min(len, VLAN_ETH_HLEN));
 
 	rp->rx_packets++;
 	rp->rx_bytes += skb->len;
-- 
1.6.4

Re: [PATCH] [NIU] VLAN does not work with niu driver

[David Miller]

You still forgot your Signed-off-by: tag...

Re: [PATCH] [NIU] VLAN does not work with niu driver

[Joyce Yu]

It was in the patch. Was an empty line needed after the body of the 
explanation? I will send out one with an empty line between body of the 
explanation and signed-off-by tag.

Thanks,
Joyce


 From f301748d3156437d65305f14288c7d5711861980 Mon Sep 17 00:00:00 2001
From: Joyce Yu <joyce.yu@sun.com>
Date: Wed, 21 Oct 2009 05:35:46 -0700
Subject: [PATCH] VLAN_ETH_HLEN should be used to make sure that the 
whole MAC header was copied to the head buffer in the Vlan packets case
Signed-off-by: Joyce Yu <joyce.yu@sun.com>  <=============== tag

---
  drivers/net/niu.c |    2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/niu.c b/drivers/net/niu.c
index f9364d0..d6c7ac6 100644
--- a/drivers/net/niu.c
+++ b/drivers/net/niu.c
@@ -3545,7 +3545,7 @@ static int niu_process_rx_pkt(struct napi_struct 
*napi, struct niu *np,
  	rp->rcr_index = index;

  	skb_reserve(skb, NET_IP_ALIGN);
-	__pskb_pull_tail(skb, min(len, NIU_RXPULL_MAX));
+	__pskb_pull_tail(skb, min(len, VLAN_ETH_HLEN));

  	rp->rx_packets++;
  	rp->rx_bytes += skb->len;
-- 
On 10/21/09 04:09 PM, David Miller wrote:
> You still forgot your Signed-off-by: tag...
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 

Re: [PATCH] [NIU] VLAN does not work with niu driver

[David Miller]

From: Joyce Yu <Joyce.Yu@Sun.COM>
Date: Wed, 21 Oct 2009 16:28:45 -0700

> It was in the patch. Was an empty line needed after the body of the
> explanation? I will send out one with an empty line between body of
> the explanation and signed-off-by tag.

It should be of the form:

--------------------
Subject: Summary description of patch.

Full patch description.

Signed-off-by: ...

The patch.
--------------------

[PATCH] [NIU] VLAN does not work with niu driver

[Joyce Yu]

[-- Attachment #1: Type: text/plain, Size: 6 bytes --]


-- 


[-- Attachment #2: 0001-VLAN-does-not-work-with-niu-driver.patch --]
[-- Type: text/x-patch, Size: 727 bytes --]

>From eb878a6887fed77e6b4c69a014a2c98ea2b52736 Mon Sep 17 00:00:00 2001
From: Joyce Yu <joyce.yu@sun.com>
Date: Thu, 15 Oct 2009 06:49:29 -0700
Subject: [PATCH] VLAN does not work with niu driver

---
 drivers/net/niu.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/niu.c b/drivers/net/niu.c
index f9364d0..d6c7ac6 100644
--- a/drivers/net/niu.c
+++ b/drivers/net/niu.c
@@ -3545,7 +3545,7 @@ static int niu_process_rx_pkt(struct napi_struct *napi, struct niu *np,
 	rp->rcr_index = index;
 
 	skb_reserve(skb, NET_IP_ALIGN);
-	__pskb_pull_tail(skb, min(len, NIU_RXPULL_MAX));
+	__pskb_pull_tail(skb, min(len, VLAN_ETH_HLEN));
 
 	rp->rx_packets++;
 	rp->rx_bytes += skb->len;
-- 
1.6.4

[PATCH] [NIU] VLAN does not work with niu driver

[Joyce Yu]

 From 0bb77e878758bd72051577bcc568e2b95c87c203 Mon Sep 17 00:00:00 2001
From: Joyce Yu <joyce.yu@sun.com>
Date: Mon, 12 Oct 2009 11:03:54 -0700
Subject: [PATCH] VLAN does not work with niu driver
Signed-off-by: Joyce Yu <joyce.yu@sun.com>

---
 drivers/net/niu.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/drivers/net/niu.c b/drivers/net/niu.c
index f9364d0..9559e42 100644
--- a/drivers/net/niu.c
+++ b/drivers/net/niu.c
@@ -3480,6 +3480,7 @@ static int niu_process_rx_pkt(struct napi_struct 
*napi, struct niu *np,
        unsigned int index = rp->rcr_index;
        struct sk_buff *skb;
        int len, num_rcr;
+       struct vlan_ethhdr *veth;

        skb = netdev_alloc_skb(np->dev, RX_SKB_ALLOC_SIZE);
        if (unlikely(!skb))
@@ -3545,7 +3546,15 @@ static int niu_process_rx_pkt(struct napi_struct 
*napi, struct niu *np,
        rp->rcr_index = index;

        skb_reserve(skb, NET_IP_ALIGN);
-       __pskb_pull_tail(skb, min(len, NIU_RXPULL_MAX));
+       __pskb_pull_tail(skb, min(len, VLAN_ETH_HLEN));
+
+       veth = (struct vlan_ethhdr *)skb->data;
+       if (veth->h_vlan_proto != __constant_htons(ETH_P_8021Q)) {
+               skb->tail -= 4;
+               skb->data_len += 4;
+               skb_shinfo(skb)->frags[0].page_offset -= 4;
+               skb_shinfo(skb)->frags[0].size += 4;
+       }

        rp->rx_packets++;
        rp->rx_bytes += skb->len;
--
1.6.4

-- 

Re: [PATCH] [NIU] VLAN does not work with niu driver

[David Miller]

From: Joyce Yu <Joyce.Yu@Sun.COM>
Date: Mon, 12 Oct 2009 22:41:36 -0700

> +++ b/drivers/net/niu.c
> @@ -3480,6 +3480,7 @@ static int niu_process_rx_pkt(struct napi_struct
> *napi, struct niu *np,

Your email client is still breaking up long lines.

Take your time and fix your setup correctly.  Read
'linux/Documentation/email-clients.txt' for tips.

[PATCH] [NIU] VLAN does not work with niu driver

[Joyce Yu]

From: Joyce Yu <joyce.yu@sun.com>
Date: Mon, 12 Oct 2009 11:03:54 -0700
Subject: [PATCH] VLAN does not work with niu driver
Signed-off-by: Joyce Yu <joyce.yu@sun.com>

---
 drivers/net/niu.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/drivers/net/niu.c b/drivers/net/niu.c
index f9364d0..9559e42 100644
--- a/drivers/net/niu.c
+++ b/drivers/net/niu.c
@@ -3480,6 +3480,7 @@ static int niu_process_rx_pkt(struct napi_struct 
*napi, struct niu *np,
        unsigned int index = rp->rcr_index;
        struct sk_buff *skb;
        int len, num_rcr;
+       struct vlan_ethhdr *veth;

        skb = netdev_alloc_skb(np->dev, RX_SKB_ALLOC_SIZE);
        if (unlikely(!skb))
@@ -3545,7 +3546,15 @@ static int niu_process_rx_pkt(struct napi_struct 
*napi, struct niu *np,
        rp->rcr_index = index;

        skb_reserve(skb, NET_IP_ALIGN);
-       __pskb_pull_tail(skb, min(len, NIU_RXPULL_MAX));
+       __pskb_pull_tail(skb, min(len, VLAN_ETH_HLEN));
+
+       veth = (struct vlan_ethhdr *)skb->data;
+       if (veth->h_vlan_proto != __constant_htons(ETH_P_8021Q)) {
+               skb->tail -= 4;
+               skb->data_len += 4;
+               skb_shinfo(skb)->frags[0].page_offset -= 4;
+               skb_shinfo(skb)->frags[0].size += 4;
+       }

        rp->rx_packets++;
        rp->rx_bytes += skb->len;
--
1.6.4

-- 

Re: [PATCH] [NIU] VLAN does not work with niu driver

[David Miller]

From: Joyce Yu <Joyce.Yu@Sun.COM>
Date: Mon, 12 Oct 2009 16:26:52 -0700

> @@ -3480,6 +3480,7 @@ static int niu_process_rx_pkt(struct napi_struct
> *napi, struct niu *np,

Your patch was corrupted by your email client, it breaks up long lines
and turns tab characters into spaces.

Please fix this up and resubmit.

[PATCH] [NIU] VLAN does not work with niu driver

[Joyce Yu]

From: Joyce Yu <joyce.yu@sun.com>
Date: Wed, 9 Sep 2009 08:43:00 -0700
Subject: [PATCH] VLAN does not work with niu driver
 Signed-off-by: Joyce Yu <joyce.yu@sun.com>

---
 drivers/net/niu.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/niu.h b/drivers/net/niu.h
index 3bd0b59..35678db 100644
--- a/drivers/net/niu.h
+++ b/drivers/net/niu.h
@@ -2700,7 +2700,7 @@ struct fcram_hash_ipv6 {
 #define RCR_PKT_TYPE_UDP               0x2
 #define RCR_PKT_TYPE_SCTP              0x3

-#define NIU_RXPULL_MAX                 ETH_HLEN
+#define NIU_RXPULL_MAX                 64

 struct rx_pkt_hdr0 {
 #if defined(__LITTLE_ENDIAN_BITFIELD)
--
1.6.4

-- 

Re: [PATCH] [NIU] VLAN does not work with niu driver

[Stephen Hemminger]

On Wed, 09 Sep 2009 14:10:48 -0700
Joyce Yu <Joyce.Yu@sun.com> wrote:

> From: Joyce Yu <joyce.yu@sun.com>
> Date: Wed, 9 Sep 2009 08:43:00 -0700
> Subject: [PATCH] VLAN does not work with niu driver
>  Signed-off-by: Joyce Yu <joyce.yu@sun.com>
> 
> ---
>  drivers/net/niu.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/niu.h b/drivers/net/niu.h
> index 3bd0b59..35678db 100644
> --- a/drivers/net/niu.h
> +++ b/drivers/net/niu.h
> @@ -2700,7 +2700,7 @@ struct fcram_hash_ipv6 {
>  #define RCR_PKT_TYPE_UDP               0x2
>  #define RCR_PKT_TYPE_SCTP              0x3
> 
> -#define NIU_RXPULL_MAX                 ETH_HLEN
> +#define NIU_RXPULL_MAX                 64
> 
>  struct rx_pkt_hdr0 {
>  #if defined(__LITTLE_ENDIAN_BITFIELD)
> --
> 1.6.4
> 

Shouldn't the vlan driver being doing pskb_may_pull()
or pskb_pull() instead?

-- 

Re: [PATCH] [NIU] VLAN does not work with niu driver

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Wed, 9 Sep 2009 14:35:14 -0700

> Shouldn't the vlan driver being doing pskb_may_pull()
> or pskb_pull() instead?

I thought about it, and my answer is 'probably not'.

Just like for eth_type_trans(), it's pretty reasonable to
expect the link level header to be linearized already.

Re: [PATCH] [NIU] VLAN does not work with niu driver

[David Miller]

From: Joyce Yu <Joyce.Yu@sun.com>
Date: Wed, 09 Sep 2009 14:10:48 -0700

> drivers/net/niu.h |    2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)

Can I get a more verbose commit message than this?

> @@ -2700,7 +2700,7 @@ struct fcram_hash_ipv6 {
> #define RCR_PKT_TYPE_UDP               0x2
> #define RCR_PKT_TYPE_SCTP              0x3
> 
> -#define NIU_RXPULL_MAX                 ETH_HLEN
> +#define NIU_RXPULL_MAX                 64
> 

See, that's why I want a detailed commit message, because if you
described things more clearly I'd understand why you choose the value
'64' as opposed to, say, the size of a VLAN header which to me would
be a more appropriate value to use here.

You just seem to be reverting a change I made a while back, and it
just so happens to fix your problem.  But '64' is too large a value
to use here and it will impact performance.

You did check to see if there were any performance regressions
resulting from your change, right?

Re: [PATCH] [NIU] VLAN does not work with niu driver

[Matheos Worku]

David Miller wrote:
> From: Joyce Yu <Joyce.Yu@sun.com>
> Date: Wed, 09 Sep 2009 14:10:48 -0700
> 
>> drivers/net/niu.h |    2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
> 
> Can I get a more verbose commit message than this?
> 
>> @@ -2700,7 +2700,7 @@ struct fcram_hash_ipv6 {
>> #define RCR_PKT_TYPE_UDP               0x2
>> #define RCR_PKT_TYPE_SCTP              0x3
>>
>> -#define NIU_RXPULL_MAX                 ETH_HLEN
>> +#define NIU_RXPULL_MAX                 64
>>
> 
> See, that's why I want a detailed commit message, because if you
> described things more clearly I'd understand why you choose the value
> '64' as opposed to, say, the size of a VLAN header which to me would
> be a more appropriate value to use here.

Dave,

The frame type in NIU HW  is embedded  in a  HW header,  so it is 
possible to check the HW header and decide whether to pull up ETH_HLEN 
or  VLAN header size of bytes. However, considering the amount of work 
required to get and examine the HW header (including endianess issues), 
we thought pulling up 64 bytes by default (as used in cassini.c) would 
be efficient.

Regards,
Matheos

> 
> You just seem to be reverting a change I made a while back, and it
> just so happens to fix your problem.  But '64' is too large a value
> to use here and it will impact performance.
> 
> You did check to see if there were any performance regressions
> resulting from your change, right?
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [NIU] VLAN does not work with niu driver

[David Miller]

From: Matheos Worku <Matheos.Worku@Sun.COM>
Date: Wed, 09 Sep 2009 18:01:23 -0700

> The frame type in NIU HW is embedded in a HW header, so it is possible
> to check the HW header and decide whether to pull up ETH_HLEN or VLAN
> header size of bytes. However, considering the amount of work required
> to get and examine the HW header (including endianess issues), we
> thought pulling up 64 bytes by default (as used in cassini.c) would be
> efficient.

Well, it was 64 in early versions of the driver, and I decreased it
down to ETH_HLEN.

The less the better since for forwarding applications anything past
the IPV4 header pulled is going to be a waste of CPU cache lines and
thus negatively effect forwarding rates.

That's why I asked if this change was performance regression tested,
because I know it's going to slow down forwarding rates for small
packets.

Re: [PATCH] [NIU] VLAN does not work with niu driver

[Matheos Worku]

David Miller wrote:
> From: Matheos Worku <Matheos.Worku@Sun.COM>
> Date: Wed, 09 Sep 2009 18:01:23 -0700
> 
>> The frame type in NIU HW is embedded in a HW header, so it is possible
>> to check the HW header and decide whether to pull up ETH_HLEN or VLAN
>> header size of bytes. However, considering the amount of work required
>> to get and examine the HW header (including endianess issues), we
>> thought pulling up 64 bytes by default (as used in cassini.c) would be
>> efficient.
> 
> Well, it was 64 in early versions of the driver, and I decreased it
> down to ETH_HLEN.
> 
> The less the better since for forwarding applications anything past
> the IPV4 header pulled is going to be a waste of CPU cache lines and
> thus negatively effect forwarding rates.
> 
> That's why I asked if this change was performance regression tested,
> because I know it's going to slow down forwarding rates for small
> packets.
Dave,

We did throughput testing (netperf) and didn't notice any performance 
degradation. We haven't done forwarding testing however.

We can work on a version which implements HW header checking and do 
pullup accordingly.


Regards,
Matheos

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

Re: [PATCH] [NIU] VLAN does not work with niu driver

[David Miller]

From: Matheos Worku <Matheos.Worku@Sun.COM>
Date: Wed, 09 Sep 2009 18:19:00 -0700

> We can work on a version which implements HW header checking and do
> pullup accordingly.

I think using a constant based on the vlan header size would be
sufficient to fix this.

Re: [PATCH] [NIU] VLAN does not work with niu driver

[Matheos Worku]

David Miller wrote:
> From: Matheos Worku <Matheos.Worku@Sun.COM>
> Date: Wed, 09 Sep 2009 18:19:00 -0700
> 
>> We can work on a version which implements HW header checking and do
>> pullup accordingly.
> 
> I think using a constant based on the vlan header size would be
> sufficient to fix this.
We will have a patch based on vlan header size.

Regards
Matheos

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

Re: [PATCH] [NIU] VLAN does not work with niu driver

[Rick Jones]

> 
> We did throughput testing (netperf) and didn't notice any performance 
> degradation. We haven't done forwarding testing however.
> 

With my "Mr. Netperf" had on I'll ask if you just measured throughput, or if you 
also included CPU utilization/service demand?

rick jones