| Message ID | 4AA819D8.1020306@Sun.COM |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
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?
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? -- 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
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. -- 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
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 -- 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
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. -- 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
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 -- 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
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. -- 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
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 -- 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
> > 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 -- 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
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)