Re: [PATCH] ixgbe: Introduce new 10GbE driver for Intel 82598 based PCI Express adapters...

2007-07-02 Thread Jeff Garzik
Ayyappan Veeraiyan wrote: +#define IXGBE_TX_FLAGS_CSUM0x0001 +#define IXGBE_TX_FLAGS_VLAN0x0002 +#define IXGBE_TX_FLAGS_TSO 0x0004 +#define IXGBE_TX_FLAGS_IPV40x0008 +#define IXGBE_TX_FLAGS_VLAN_MASK 0x +#define IXGBE_T

Re: [PATCH] ixgbe: Introduce new 10GbE driver for Intel 82598 based PCI Express adapters...

2007-07-02 Thread Arjan van de Ven
>> +u32 alloc_rx_buff_failed; +struct { +unsigned int rx_csum_enabled:1; +unsigned int msi_capable:1; +unsigned int msi_enabled:1; +unsigned int msix_capable:1; +unsigned int msix_enabled:1; +unsigned int imir_enabled

Re: [PATCH] ixgbe: Introduce new 10GbE driver for Intel 82598 based PCI Express adapters...

2007-07-02 Thread Jeff Garzik
Arjan van de Ven wrote: >> +u32 alloc_rx_buff_failed; +struct { +unsigned int rx_csum_enabled:1; +unsigned int msi_capable:1; +unsigned int msi_enabled:1; +unsigned int msix_capable:1; +unsigned int msix_enabled:1; +uns

Re: [PATCH] ixgbe: Introduce new 10GbE driver for Intel 82598 based PCI Express adapters...

2007-07-02 Thread Arjan van de Ven
Jeff Garzik wrote: always avoid bitfields. They generate horrible code, and endian problems abound (though no endian problems are apparent here). they generate no worse code than open coding the checks for these feature flags... That would be the logical assumption, but reality does not bea

Re: [PATCH] ixgbe: Introduce new 10GbE driver for Intel 82598 based PCI Express adapters...

2007-07-02 Thread Arjan van de Ven
Jeff Garzik wrote: I'm not sure whether gcc is confused about ABI alignment or such, on various platforms, but every time I look at the asm generated it is /not/ equivalent to open-coded feature flags and bitwise logic. Often it is demonstrably worse. (I can imagine this being different if y

Re: [PATCH] ixgbe: Introduce new 10GbE driver for Intel 82598 based PCI Express adapters...

2007-07-02 Thread Jeff Garzik
Arjan van de Ven wrote: Jeff Garzik wrote: always avoid bitfields. They generate horrible code, and endian problems abound (though no endian problems are apparent here). they generate no worse code than open coding the checks for these feature flags... That would be the logical assumption,

Re: [PATCH] ixgbe: Introduce new 10GbE driver for Intel 82598 based PCI Express adapters...

2007-07-02 Thread Arjan van de Ven
Jeff Garzik wrote: It's simple logic: using machine integers are the easiest for the compiler to Do The Right Thing, the easiest way to eliminate endian problems, the easiest way for programmers to read and understand struct alignment. using integers pure is obviously natural.. but using int

Re: [PATCH] ixgbe: Introduce new 10GbE driver for Intel 82598 based PCI Express adapters...

2007-07-02 Thread Kok, Auke
Jeff Garzik wrote: Arjan van de Ven wrote: Jeff Garzik wrote: always avoid bitfields. They generate horrible code, and endian problems abound (though no endian problems are apparent here). they generate no worse code than open coding the checks for these feature flags... That would be the log

Re: [PATCH] ixgbe: Introduce new 10GbE driver for Intel 82598 based PCI Express adapters...

2007-07-02 Thread Jeff Garzik
Kok, Auke wrote: Maybe this is not most important for ixgbe, where we only have 8 or so flags, but the new e1000 driver that I posted this weekend currently has 63 (you wanted flags ;)) of them. Do you want me to use 63 integers or just 2 ? Don't be silly. We are talking about single-bit fe

Re: [PATCH] ixgbe: Introduce new 10GbE driver for Intel 82598 based PCI Express adapters...

2007-07-02 Thread Andrew Morton
On Mon, 02 Jul 2007 11:32:41 -0400 Jeff Garzik <[EMAIL PROTECTED]> wrote: > Bitfields are to be avoided for many reasons: > * more difficult, in general, for a compiler to generate optimal code > * in particular, known to generate worse code on various architectures > * often causes endian problem

Re: [PATCH] ixgbe: Introduce new 10GbE driver for Intel 82598 based PCI Express adapters...

2007-07-02 Thread Kok, Auke
Jeff Garzik wrote: Kok, Auke wrote: Maybe this is not most important for ixgbe, where we only have 8 or so flags, but the new e1000 driver that I posted this weekend currently has 63 (you wanted flags ;)) of them. Do you want me to use 63 integers or just 2 ? Don't be silly. We are talking

Re: [PATCH] ixgbe: Introduce new 10GbE driver for Intel 82598 based PCI Express adapters...

2007-07-02 Thread Jeff Garzik
Andrew Morton wrote: On Mon, 02 Jul 2007 11:32:41 -0400 Jeff Garzik <[EMAIL PROTECTED]> wrote: Bitfields are to be avoided for many reasons: * more difficult, in general, for a compiler to generate optimal code * in particular, known to generate worse code on various architectures * often cause

RE: [PATCH] ixgbe: Introduce new 10GbE driver for Intel 82598 based PCI Express adapters...

2007-07-02 Thread Veeraiyan, Ayyappan
On 7/2/07, Jeff Garzik <[EMAIL PROTECTED]> wrote: > Ayyappan Veeraiyan wrote: > > +#define IXGBE_TX_FLAGS_VLAN_MASK 0x > > +#define IXGBE_TX_FLAGS_VLAN_SHIFT16 > > defining bits using the form "(1 << n)" is preferred. Makes it easier > to read, by eliminating the requirement of th

Re: [PATCH] ixgbe: Introduce new 10GbE driver for Intel 82598 based PCI Express adapters...

2007-07-02 Thread Ayyappan Veeraiyan
On 7/2/07, Veeraiyan, Ayyappan <[EMAIL PROTECTED]> wrote: Thanks for the feedback. I will post another version shortly (except the feature flags change and the ring struct name members) which fixes my Just to clarify this, I will wait some more time for feature flags discussion and the ring st

Re: [PATCH] ixgbe: Introduce new 10GbE driver for Intel 82598 based PCI Express adapters...

2007-07-02 Thread Christoph Hellwig
On Mon, Jul 02, 2007 at 12:00:29PM -0700, Veeraiyan, Ayyappan wrote: > Agreed. > Fake netdevs are needed for doing the multiple Rx queues in NAPI mode. > We are thinking to solve in 2 ways. Having netdev pointer in ring > structure or having an array of netdev pointers in ixgbe_adatper struct > wh

Re: [PATCH] ixgbe: Introduce new 10GbE driver for Intel 82598 based PCI Express adapters...

2007-07-02 Thread Stephen Hemminger
On Mon, 2 Jul 2007 12:00:29 -0700 "Veeraiyan, Ayyappan" <[EMAIL PROTECTED]> wrote: > On 7/2/07, Jeff Garzik <[EMAIL PROTECTED]> wrote: > > Ayyappan Veeraiyan wrote: > > > +#define IXGBE_TX_FLAGS_VLAN_MASK 0x > > > +#define IXGBE_TX_FLAGS_VLAN_SHIFT16 > > > > defining bits using th

Re: [PATCH] ixgbe: Introduce new 10GbE driver for Intel 82598 based PCI Express adapters...

2007-07-02 Thread Christoph Hellwig
On Mon, Jul 02, 2007 at 02:09:58PM -0700, Stephen Hemminger wrote: > The patch is close to ready for 2.6.24 when this driver will need to show up. If intel manages to fix up the reamining issues I'd rather see it appear in 2.6.23.. > Since I know Intel will be forced to backport this to older dis

Re: [PATCH] ixgbe: Introduce new 10GbE driver for Intel 82598 based PCI Express adapters...

2007-07-02 Thread Kok, Auke
Christoph Hellwig wrote: On Mon, Jul 02, 2007 at 02:09:58PM -0700, Stephen Hemminger wrote: The patch is close to ready for 2.6.24 when this driver will need to show up. If intel manages to fix up the reamining issues I'd rather see it appear in 2.6.23.. Since I know Intel will be forced to

Re: [PATCH] ixgbe: Introduce new 10GbE driver for Intel 82598 based PCI Express adapters...

2007-07-02 Thread Jeff Garzik
Kok, Auke wrote: I suppose I can fix those, but I really don't understand what all the fuzz is about here. We're only conserving memory and staying far away from the real risks of bitmasks, so forgive me if I don't grasp the problem. Be it machine ints or bitfields, you're not conserving memo

Re: [PATCH] ixgbe: Introduce new 10GbE driver for Intel 82598 based PCI Express adapters...

2007-07-02 Thread Michael Buesch
On Tuesday 03 July 2007 00:02:57 Kok, Auke wrote: > well, FWIW when I started looking at adding these flags I looked in various > subsystems in the kernel and picked an implementation that suited. Guess what > pci.h has? ...: > > unsigned int msi_enabled:1; > unsigned int msix_enable

Re: [PATCH] ixgbe: Introduce new 10GbE driver for Intel 82598 based PCI Express adapters...

2007-07-02 Thread Jeff Garzik
Michael Buesch wrote: On Tuesday 03 July 2007 00:02:57 Kok, Auke wrote: well, FWIW when I started looking at adding these flags I looked in various subsystems in the kernel and picked an implementation that suited. Guess what pci.h has? ...: unsigned int msi_enabled:1; unsigne

RE: [PATCH] ixgbe: Introduce new 10GbE driver for Intel 82598 based PCI Express adapters...

2007-07-02 Thread Veeraiyan, Ayyappan
On 7/2/07, Stephen Hemminger <[EMAIL PROTECTED]> wrote: > > Fake netdevs are needed for doing the multiple Rx queues in NAPI mode. > > We are thinking to solve in 2 ways. Having netdev pointer in ring > > structure or having an array of netdev pointers in ixgbe_adatper struct > > which would be vis

Re: [PATCH] ixgbe: Introduce new 10GbE driver for Intel 82598 based PCI Express adapters...

2007-07-02 Thread Kok, Auke
Jeff Garzik wrote: Michael Buesch wrote: On Tuesday 03 July 2007 00:02:57 Kok, Auke wrote: well, FWIW when I started looking at adding these flags I looked in various subsystems in the kernel and picked an implementation that suited. Guess what pci.h has? ...: unsigned int msi_enable

RE: [PATCH] ixgbe: Introduce new 10GbE driver for Intel 82598 based PCI Express adapters...

2007-07-02 Thread Veeraiyan, Ayyappan
On 7/2/07, Christoph Hellwig <[EMAIL PROTECTED]> wrote: > > But that'll require the single receiver queue version I guess. The > netdevice abuse is the only really major issue I see, although I'd of > course really like to see the driver getting rid of the bitfield abuse > aswell. The submitted

Re: [PATCH] ixgbe: Introduce new 10GbE driver for Intel 82598 based PCI Express adapters...

2007-07-02 Thread Jeff Garzik
Kok, Auke wrote: but let's stay constructive here: ~/git/linux-2.6 $ grep -r 'unsigned int.*:1;' * | wc -l 748 Is anyone going to fix those? If we don't, someone will certainly again submit patches to add more of these bitfields, after all, some very prominent parts of the kernel still use th

Re: [PATCH] ixgbe: Introduce new 10GbE driver for Intel 82598 based PCI Express adapters...

2007-07-02 Thread Inaky Perez-Gonzalez
On Monday 02 July 2007, Kok, Auke wrote: > Jeff Garzik wrote: > > Michael Buesch wrote: > >> On Tuesday 03 July 2007 00:02:57 Kok, Auke wrote: > >>> well, FWIW when I started looking at adding these flags I looked in > >>> various > >>> subsystems in the kernel and picked an implementation that s

Re: [PATCH] ixgbe: Introduce new 10GbE driver for Intel 82598 based PCI Express adapters...

2007-07-03 Thread Neil Horman
On Mon, Jul 02, 2007 at 12:00:29PM -0700, Veeraiyan, Ayyappan wrote: > On 7/2/07, Jeff Garzik <[EMAIL PROTECTED]> wrote: > > Ayyappan Veeraiyan wrote: > > > +#define IXGBE_TX_FLAGS_VLAN_MASK 0x > > > +#define IXGBE_TX_FLAGS_VLAN_SHIFT16 > > > > defining bits using the form "(1 << n

Re: [PATCH] ixgbe: Introduce new 10GbE driver for Intel 82598 based PCI Express adapters...

2007-07-03 Thread Jeff Garzik
Inaky Perez-Gonzalez wrote: I don't think bitfields are broken. Maybe it's the compiler what should be fixed (*) Then you do not understand bitfields. It is -axiomatic- that bitfields are more difficult for compilers to implement. Access to bitfields are not atomic within the machine int i

Re: [PATCH] ixgbe: Introduce new 10GbE driver for Intel 82598 based PCI Express adapters...

2007-07-03 Thread Inaky Perez-Gonzalez
On Tuesday 03 July 2007, Jeff Garzik wrote: > Inaky Perez-Gonzalez wrote: > > I don't think bitfields are broken. Maybe it's the compiler what should be > > fixed (*) > > Then you do not understand bitfields. It is -axiomatic- that bitfields > are more difficult for compilers to implement. > >

Re: [PATCH] ixgbe: Introduce new 10GbE driver for Intel 82598 based PCI Express adapters...

2007-07-03 Thread Ayyappan Veeraiyan
On 7/2/07, Veeraiyan, Ayyappan <[EMAIL PROTECTED]> wrote: On 7/2/07, Christoph Hellwig <[EMAIL PROTECTED]> wrote: The submitted driver code supports single queue version in case of MSIX allocation failures... As I said in the other mail, I feel, restricting to single Rx queue in NAPI mode is b

Re: [PATCH] ixgbe: Introduce new 10GbE driver for Intel 82598 based PCI Express adapters...

2007-07-05 Thread Neil Horman
On Tue, Jul 03, 2007 at 08:53:46AM -0400, Neil Horman wrote: > On Mon, Jul 02, 2007 at 12:00:29PM -0700, Veeraiyan, Ayyappan wrote: > > On 7/2/07, Jeff Garzik <[EMAIL PROTECTED]> wrote: > > > Ayyappan Veeraiyan wrote: > > > > +#define IXGBE_TX_FLAGS_VLAN_MASK 0x > > > > +#define IXGBE_T

Re: [PATCH] ixgbe: Introduce new 10GbE driver for Intel 82598 based PCI Express adapters...

2007-07-05 Thread Jeff Garzik
Inaky Perez-Gonzalez wrote: On Tuesday 03 July 2007, Jeff Garzik wrote: Inaky Perez-Gonzalez wrote: Access to bitfields are not atomic within the machine int in which they are stored... you need to "unpack" the values stored in bitfields, even if they are single-bit bitfields. Which we do ma

Re: [PATCH] ixgbe: Introduce new 10GbE driver for Intel 82598 based PCI Express adapters...

2007-07-06 Thread Ingo Oeser
Hi Auke, Kok, Auke schrieb: > tg3.c: > > if ((tp->tg3_flags & TG3_FLAG_PCIX_TARGET_HWBUG) || > (tp->tg3_flags2 & TG3_FLG2_ICH_WORKAROUND)) > > is obviously _not_ easier to read than > > if (tp->tg3_flags.pcix_target_hwbug || tp->tg3_flags.ich_workaround) > Yes, but wh

RE: [PATCH] ixgbe: Introduce new 10GbE driver for Intel 82598 based PCI Express adapters...

2007-07-09 Thread Veeraiyan, Ayyappan
>From: Neil Horman [mailto:[EMAIL PROTECTED] >Replying to myself... > I've looked through the driver pretty throughly with regards to my >above >concern, and it appears the driver is reasonably free of netpoll issues at >the >moment, at least as far as what we found in e1000 was concerned. I

Re: [PATCH] ixgbe: Introduce new 10GbE driver for Intel 82598 based PCI Express adapters...

2007-07-09 Thread Neil Horman
On Mon, Jul 09, 2007 at 07:21:24AM -0700, Veeraiyan, Ayyappan wrote: > >From: Neil Horman [mailto:[EMAIL PROTECTED] > >Replying to myself... > > I've looked through the driver pretty throughly with regards to > my > >above > >concern, and it appears the driver is reasonably free of netpoll issu