Re: [PATCH 18 of 20] ipath - kbuild infrastructure

Re: [PATCH 18 of 20] ipath - kbuild infrastructure

[Roland Dreier]

 > +	depends on 64BIT && (PCIEPORTBUS || X86_HT)

Why do you depend on X86_HT?  I think you got confused: the HT stands
for hyperthreading, not hypertransport.  In fact if you compile a
kernel optimized for K8, X86_HT is disabled.

And why do you depend on PCIEPORTBUS?  I don't see you using anything
from the pcie_port_service stuff.

I think the correct thing for you to depend on is just "PCI", and
build your whole driver (both pe800 and ht400) unconditionally.
There's no special hypertransport or generic PCIe support config
that you can test.

 > --- /dev/null	Thu Jan  1 00:00:00 1970 +0000
 > +++ b/drivers/infiniband/hw/ipath/Makefile	Thu Mar  9 08:46:47 2006 -0800

I've been suggesting that new files be called "Kbuild", since Sam has
deprecated the "Makefile" name.

 > +ipath_core-y :=
 > +
 > +ipath_core-y += ipath_copy.o
 > +ipath_core-y += ipath_diag.o
 > +ipath_core-y += ipath_driver.o
 > +ipath_core-y += ipath_file_ops.o
 > +ipath_core-y += ipath_i2c.o
 > +ipath_core-y += ipath_init_chip.o
 > +ipath_core-y += ipath_intr.o
 > +ipath_core-y += ipath_layer.o
 > +ipath_core-y += ipath_sma.o
 > +ipath_core-y += ipath_stats.o
 > +ipath_core-y += ipath_sysfs.o
 > +ipath_core-y += ipath_user_pages.o

This is a very strange style.  I would just put all the ipath_core-y
stuff on one or two lines.

 - R.

Re: [PATCH 18 of 20] ipath - kbuild infrastructure

[Sam Ravnborg]

On Thu, Mar 09, 2006 at 09:53:04AM -0800, Roland Dreier wrote:
>  > +	depends on 64BIT && (PCIEPORTBUS || X86_HT)
> 
 
>  > --- /dev/null	Thu Jan  1 00:00:00 1970 +0000
>  > +++ b/drivers/infiniband/hw/ipath/Makefile	Thu Mar  9 08:46:47 2006 -0800
> 
> I've been suggesting that new files be called "Kbuild", since Sam has
> deprecated the "Makefile" name.
Eventually - yes.
But not just now. Kbuild was introduced because it was needed in the
top-level directory and it made good sense to do so.
But for now keeping Makefile is a good choice. This is anyway what
people are used to.

	Sam

Re: [PATCH 18 of 20] ipath - kbuild infrastructure

[Roland Dreier]

    Sam> Eventually - yes.  But not just now. Kbuild was introduced
    Sam> because it was needed in the top-level directory and it made
    Sam> good sense to do so.  But for now keeping Makefile is a good
    Sam> choice. This is anyway what people are used to.

OK, disregard my suggestion then.  Should we patch
Documentation/kbuild/makefiles.txt to correct the current
documentation, which says:

  The preferred name for the kbuild files is 'Kbuild' but 'Makefile'
  will continue to be supported. All new developmen is expected to use
  the Kbuild filename.

 - R.

Re: [PATCH 18 of 20] ipath - kbuild infrastructure

[Sam Ravnborg]

On Thu, Mar 09, 2006 at 11:00:07AM -0800, Roland Dreier wrote:
>     Sam> Eventually - yes.  But not just now. Kbuild was introduced
>     Sam> because it was needed in the top-level directory and it made
>     Sam> good sense to do so.  But for now keeping Makefile is a good
>     Sam> choice. This is anyway what people are used to.
> 
> OK, disregard my suggestion then.  Should we patch
> Documentation/kbuild/makefiles.txt to correct the current
> documentation, which says:
> 
>   The preferred name for the kbuild files is 'Kbuild' but 'Makefile'
>   will continue to be supported. All new developmen is expected to use
>   the Kbuild filename.
> 
Yes - I will do so. Thanks.

	Sam

Re: [PATCH 18 of 20] ipath - kbuild infrastructure

[Sam Ravnborg]

On Thu, Mar 09, 2006 at 11:00:07AM -0800, Roland Dreier wrote:
>     Sam> Eventually - yes.  But not just now. Kbuild was introduced
>     Sam> because it was needed in the top-level directory and it made
>     Sam> good sense to do so.  But for now keeping Makefile is a good
>     Sam> choice. This is anyway what people are used to.
> 
> OK, disregard my suggestion then.  Should we patch
> Documentation/kbuild/makefiles.txt to correct the current
> documentation, which says:
> 
>   The preferred name for the kbuild files is 'Kbuild' but 'Makefile'
>   will continue to be supported. All new developmen is expected to use
>   the Kbuild filename.

I've just checked in the following patch:

diff --git a/Documentation/kbuild/makefiles.txt b/Documentation/kbuild/makefiles.txt
index 99d51a5..a9c00fa 100644
--- a/Documentation/kbuild/makefiles.txt
+++ b/Documentation/kbuild/makefiles.txt
@@ -106,9 +106,9 @@ This document is aimed towards normal de
 Most Makefiles within the kernel are kbuild Makefiles that use the
 kbuild infrastructure. This chapter introduce the syntax used in the
 kbuild makefiles.
-The preferred name for the kbuild files is 'Kbuild' but 'Makefile' will
-continue to be supported. All new developmen is expected to use the
-Kbuild filename.
+The preferred name for the kbuild files are 'Makefile' but 'Kbuild' can
+be used and if both a 'Makefile' and a 'Kbuild' file exists then the 'Kbuild'
+file will be used.
 
 Section 3.1 "Goal definitions" is a quick intro, further chapters provide
 more details, with real examples.

[PATCH 0 of 20] [RFC] ipath driver - another round for review

[Bryan O'Sullivan]

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

I posted these patches for review this morning, but due to a bug in my
posting script, only Roland actually received them.  In fact, this version
of these patches contains a few changes in response to his comments.

Thanks, Roland!

The original text from this morning follows.

Here is another set of ipath driver patches for review.  The list of
changes compared to the last patch set I posted is huge, so I won't go
into it here.  Suffice it to say that we've taken every reviewer
comment into account, and done a *lot* of work to clean things up.

I'll point out a few things that I think are worth attention.

  - We've introduced support for our PCI Express chips, so the driver
    is no longer HyperTransport-specific.  It's still a 64-bit driver,
    because 32-bit platforms don't implement readq or writeq.  (It
    does compile cleanly on i386, but of course fails to link.)

  - We've added an ethernet emulation driver so that if you're not
    using Infiniband support, you still have a high-performance net
    device (lower latency and higher bandwidth than IPoIB) for IP
    traffic.

  - There are no longer any fixed tables of device structures.
    Instead we allocate device structures dynamically using
    pci_alloc_consistent or dma_alloc_coherent, and use the
    <linux/idr.h> stuff to number them.

  - There are no more ioctls anywhere.

  - Huge source files have been split up into digestible, logical
    chunks.

  - A few more sparse annotations.

  - Buckets of other cleanups.  Code reformatting, comment
    reformatting, trimming code to <= 76 cols, you name it.

There are still a few things left to do that I know of.

  - Since the core driver isn't really an IB driver at all, perhaps it
    belongs in drivers/char instead of drivers/infiniband/hw?

  - Our hardware only supports MSI interrupts.  I don't know how to
    program it to interrupt us if CONFIG_PCI_MSI is not set.  Right
    now, we have a timer-based hack in place to emulate interrupts.

  - Not all of the code is 80- or 76-col clean yet.  I'm working on
    this.

  - I guess we need to face the music and use sysfs binary attributes
    in the two cases where we're not at the moment :-)

  - There's clearly something wrong with the way we're pinning some
    pages into memory, but I don't actually know what it is.  I'm
    pretty sure our use of get_user_pages is correct, so I suspect it
    must be the code that's doing SetPageReserved (see ipath_driver.c
    and ipath_file_ops.c).

    I've spent some time trying to figure out what the problem is, but
    am stumped.  If someone knows what we should be doing instead, I'd
    be delighted to hear from them.

If you have any comments or suggestions, please let me know.

	<b

[PATCH 18 of 20] ipath - kbuild infrastructure

[Bryan O'Sullivan]

Signed-off-by: Bryan O'Sullivan <bos@pathscale.com>

diff -r 1c88f73c2ac0 -r 867a396dd518 drivers/infiniband/hw/ipath/Kconfig
--- /dev/null	Thu Jan  1 00:00:00 1970 +0000
+++ b/drivers/infiniband/hw/ipath/Kconfig	Thu Mar  9 16:17:00 2006 -0800
@@ -0,0 +1,18 @@
+config IPATH_CORE
+	tristate "PathScale InfiniPath Driver"
+	depends on 64BIT && (PCIEPORTBUS || X86_HT)
+	---help---
+	This is a low-level driver for PathScale InfiniPath host channel
+	adapters (HCAs) based on the HT-400 and PE-800 chips, including
+	the InfiniPath HT-460, the small form factor InfiniPath HT-460,
+	the InfiniPath HT-470 and the Linux Networx LS/X.
+
+config INFINIBAND_IPATH
+	tristate "PathScale InfiniPath Verbs Driver"
+	depends on IPATH_CORE && INFINIBAND
+	---help---
+	This is a driver that provides InfiniBand verbs support for
+	PathScale InfiniPath host channel adapters (HCAs).  This
+	allows these devices to be used with both kernel upper level
+	protocols such as IP-over-InfiniBand as well as with userspace
+	applications (in conjunction with InfiniBand userspace access).
diff -r 1c88f73c2ac0 -r 867a396dd518 drivers/infiniband/hw/ipath/Makefile
--- /dev/null	Thu Jan  1 00:00:00 1970 +0000
+++ b/drivers/infiniband/hw/ipath/Makefile	Thu Mar  9 16:17:00 2006 -0800
@@ -0,0 +1,42 @@
+EXTRA_CFLAGS += -O3
+
+_ipath_idstr:="PathScale $(shell date +%F)"
+EXTRA_CFLAGS += -DIPATH_IDSTR='$(_ipath_idstr)' -DIPATH_KERN_TYPE=0
+
+obj-$(CONFIG_IPATH_CORE) += ipath_core.o
+obj-$(CONFIG_INFINIBAND_IPATH) += ib_ipath.o
+obj-$(CONFIG_IPATH_ETHER) += ipath_ether.o
+
+ipath_core-y := \
+	ipath_copy.o \
+	ipath_diag.o \
+	ipath_driver.o \
+	ipath_eeprom.o \
+	ipath_file_ops.o \
+	ipath_ht400.o \
+	ipath_init_chip.o \
+	ipath_intr.o \
+	ipath_layer.o \
+	ipath_pe800.o \
+	ipath_sma.o \
+	ipath_stats.o \
+	ipath_sysfs.o \
+	ipath_user_pages.o
+
+ipath_core-$(CONFIG_X86_64) += ipath_wc_x86_64.o
+
+ib_ipath-y := \
+	ipath_cq.o \
+	ipath_keys.o \
+	ipath_mad.o \
+	ipath_mr.o \
+	ipath_qp.o \
+	ipath_rc.o \
+	ipath_ruc.o \
+	ipath_srq.o \
+	ipath_uc.o \
+	ipath_ud.o \
+	ipath_verbs.o \
+	ipath_verbs_mcast.o
+
+ipath_ether-y := ipath_eth.o

Re: [PATCH 18 of 20] ipath - kbuild infrastructure

[Adrian Bunk]

On Thu, Mar 09, 2006 at 04:35:48PM -0800, Bryan O'Sullivan wrote:
>...
> --- /dev/null	Thu Jan  1 00:00:00 1970 +0000
> +++ b/drivers/infiniband/hw/ipath/Makefile	Thu Mar  9 16:17:00 2006 -0800
> @@ -0,0 +1,42 @@
> +EXTRA_CFLAGS += -O3

I'm still a bit surprised, since in the rest of the kernel we are even 
going from -O2 to -Os for getting better performance.

Robert said he wanted to post some numbers showing that -O3 is 
measurably better for you [1], but I haven't seen them.

> +_ipath_idstr:="PathScale $(shell date +%F)"
> +EXTRA_CFLAGS += -DIPATH_IDSTR='$(_ipath_idstr)' -DIPATH_KERN_TYPE=0
>...

UTS_VERSION is already available and printed at the top of dmesg.
I don't see the point in printing it a second time.

cu
Adrian

[1] http://lkml.org/lkml/2005/12/17/115

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH 18 of 20] ipath - kbuild infrastructure

[Robert Walsh]

> I'm still a bit surprised, since in the rest of the kernel we are even 
> going from -O2 to -Os for getting better performance.
> 
> Robert said he wanted to post some numbers showing that -O3 is 
> measurably better for you [1], but I haven't seen them.

As you might have guessed, I kind of forgot about this (I was swamped
with unrelated stuff.)  I'll try get someone else here at PathScale to
get these numbers.

Regards,
 Robert.

-- 
Robert Walsh                                 Email: rjwalsh@pathscale.com
PathScale, Inc.                              Phone: +1 650 934 8117
2071 Stierlin Court, Suite 200                 Fax: +1 650 428 1969
Mountain View, CA 94043.

Re: [PATCH 18 of 20] ipath - kbuild infrastructure

[Bryan O'Sullivan]

On Mon, 2006-03-13 at 19:10 +0100, Adrian Bunk wrote:

> I'm still a bit surprised, since in the rest of the kernel we are even 
> going from -O2 to -Os for getting better performance.
> 
> Robert said he wanted to post some numbers showing that -O3 is 
> measurably better for you [1], but I haven't seen them.

I just ran some numbers.  At large packet sizes, it doesn't matter what
options we use, because we spend all of our time in __iowrite_copy32,
which uses the string copy instructions.

For small packets, my quick tests indicate that -Os gives about 5%
*better* performance than -O3 (using gcc 4 on FC4).  This is in line
with what people have been finding in the kernel in general recently.

So if I change that CFLAGS line from -O3 to -Os, are we in OK
shape?  :-)

> > +_ipath_idstr:="PathScale $(shell date +%F)"
> > +EXTRA_CFLAGS += -DIPATH_IDSTR='$(_ipath_idstr)' -DIPATH_KERN_TYPE=0
> >...
> 
> UTS_VERSION is already available and printed at the top of dmesg.
> I don't see the point in printing it a second time.

Good point.  The idstr stuff is for our out-of-tree drivers.

Thanks,

	<b

Re: [PATCH 18 of 20] ipath - kbuild infrastructure

[Sam Ravnborg]

On Mon, Mar 13, 2006 at 11:24:28AM -0800, Bryan O'Sullivan wrote:
> On Mon, 2006-03-13 at 19:10 +0100, Adrian Bunk wrote:
> 
> > I'm still a bit surprised, since in the rest of the kernel we are even 
> > going from -O2 to -Os for getting better performance.
> > 
> > Robert said he wanted to post some numbers showing that -O3 is 
> > measurably better for you [1], but I haven't seen them.
> 
> I just ran some numbers.  At large packet sizes, it doesn't matter what
> options we use, because we spend all of our time in __iowrite_copy32,
> which uses the string copy instructions.
> 
> For small packets, my quick tests indicate that -Os gives about 5%
> *better* performance than -O3 (using gcc 4 on FC4).  This is in line
> with what people have been finding in the kernel in general recently.
> 
> So if I change that CFLAGS line from -O3 to -Os, are we in OK
> shape?  :-)
Use the kernel settings. We cannot have this modified by each and every
driver.

	Sam

Re: [PATCH 18 of 20] ipath - kbuild infrastructure

[Bryan O'Sullivan]

On Mon, 2006-03-13 at 20:36 +0100, Sam Ravnborg wrote:

> Use the kernel settings. We cannot have this modified by each and every
> driver.

Fair enough.

	<b