[PATCH] IB/core: export struct ib_port

[PATCH] IB/core: export struct ib_port

[Ralph Campbell]

This patch moves the definition of struct ib_port from
sysfs.c to ib_verbs.h so that HCAs can create files in
/sys/class/infiniband/<hca>/ports/<N>/

Signed-off-by: Ralph Campbell <ralph.campbell-h88ZbnxC6KDQT0dZR+AlfA@public.gmane.org>
---

diff --git a/drivers/infiniband/core/sysfs.c b/drivers/infiniband/core/sysfs.c
index 158a214..e01f3e7 100644
--- a/drivers/infiniband/core/sysfs.c
+++ b/drivers/infiniband/core/sysfs.c
@@ -39,14 +39,6 @@
 
 #include <rdma/ib_mad.h>
 
-struct ib_port {
-	struct kobject         kobj;
-	struct ib_device      *ibdev;
-	struct attribute_group gid_group;
-	struct attribute_group pkey_group;
-	u8                     port_num;
-};
-
 struct port_attribute {
 	struct attribute attr;
 	ssize_t (*show)(struct ib_port *, struct port_attribute *, char *buf);
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index c179318..5d23957 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -1154,6 +1154,14 @@ struct ib_device {
 	u8                           phys_port_cnt;
 };
 
+struct ib_port {
+	struct kobject         kobj;
+	struct ib_device      *ibdev;
+	struct attribute_group gid_group;
+	struct attribute_group pkey_group;
+	u8                     port_num;
+};
+
 struct ib_client {
 	char  *name;
 	void (*add)   (struct ib_device *);


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

Re: [PATCH] IB/core: export struct ib_port

[Roland Dreier]

 > This patch moves the definition of struct ib_port from
 > sysfs.c to ib_verbs.h so that HCAs can create files in
 > /sys/class/infiniband/<hca>/ports/<N>/

um, maybe, but we need to see how it gets used first.  How do you get
the to struct ib_port in driver code?  Maybe it would make more sense to
add a way for low-level drivers to pass in port attributes that get
added when the port structure gets created?

By the way, any plans to resume working on the upstream driver for
qlogic HCAs?  Do you still plan to deprecate ib_ipath and add a new
driver for new devices?

 - R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/core: export struct ib_port

[Ralph Campbell]

On Wed, 2009-11-11 at 11:19 -0800, Roland Dreier wrote:
> > This patch moves the definition of struct ib_port from
>  > sysfs.c to ib_verbs.h so that HCAs can create files in
>  > /sys/class/infiniband/<hca>/ports/<N>/
> 
> um, maybe, but we need to see how it gets used first.  How do you get
> the to struct ib_port in driver code?  Maybe it would make more sense to
> add a way for low-level drivers to pass in port attributes that get
> added when the port structure gets created?

It is used by the new ib_qib driver to expose the SL to VL table
since the user level MPI library (libpsm) constructs packets including
the IB header. After the driver calls ib_register_device(),
it calls device_create_file() to create the files in
/sys/class/infiniband/qib0/. Then it uses struct ib_device->port_list
to get the pointer to ib_port to add a directory similar to "gids"
and "pkeys" for each SL.

> By the way, any plans to resume working on the upstream driver for
> qlogic HCAs?  Do you still plan to deprecate ib_ipath and add a new
> driver for new devices?

Yes, this is what I'm working on.
The patch is the only change outside of the hw/qib/ directory.
Do you want to see a preview of the sysfs code?

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

Re: [PATCH] IB/core: export struct ib_port

[Roland Dreier]

 > It is used by the new ib_qib driver to expose the SL to VL table
 > since the user level MPI library (libpsm) constructs packets including
 > the IB header. After the driver calls ib_register_device(),
 > it calls device_create_file() to create the files in
 > /sys/class/infiniband/qib0/. Then it uses struct ib_device->port_list
 > to get the pointer to ib_port to add a directory similar to "gids"
 > and "pkeys" for each SL.

Hmm, maybe we should just add a vls directory with sl0 ... sl15 or
something like that in generic code?  I don't see why this needs to be
driver-specific code.

 > Yes, this is what I'm working on.
 > The patch is the only change outside of the hw/qib/ directory.
 > Do you want to see a preview of the sysfs code?

I think the earlier you can post the whole driver, the sooner you'll get
it upstream.

 - R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/core: export struct ib_port

[Dave Olson]

On Wed, 11 Nov 2009, Roland Dreier wrote:
|  > It is used by the new ib_qib driver to expose the SL to VL table
|  > since the user level MPI library (libpsm) constructs packets including
|  > the IB header. After the driver calls ib_register_device(),
|  > it calls device_create_file() to create the files in
|  > /sys/class/infiniband/qib0/. Then it uses struct ib_device->port_list
|  > to get the pointer to ib_port to add a directory similar to "gids"
|  > and "pkeys" for each SL.
| 
| Hmm, maybe we should just add a vls directory with sl0 ... sl15 or
| something like that in generic code?  I don't see why this needs to be
| driver-specific code.

No particular reason, it just didn't seem likely to be useful on other
HCA drivers.   I can redo the patches that way, if people think it's
the right thing to do.

|  > Yes, this is what I'm working on.
|  > The patch is the only change outside of the hw/qib/ directory.
|  > Do you want to see a preview of the sysfs code?
| 
| I think the earlier you can post the whole driver, the sooner you'll get
| it upstream.

Yeah, we know...   Schedule and people conflicts have slowed this down.
We want to do it, and soon.

And yes, the ib_ipath is being fully deprecated.  The "full set" of
patches that adds ib_qib upstream will include a subset that drops
ib_ipath.   All the bug fixes and feature work have been done for ib_qib

Dave Olson
dave.olson-h88ZbnxC6KDQT0dZR+AlfA@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/core: export struct ib_port

[Roland Dreier]

 > | Hmm, maybe we should just add a vls directory with sl0 ... sl15 or
 > | something like that in generic code?  I don't see why this needs to be
 > | driver-specific code.
 > 
 > No particular reason, it just didn't seem likely to be useful on other
 > HCA drivers.   I can redo the patches that way, if people think it's
 > the right thing to do.

To me it does seem like something generic.  SLtoVL table is required of
all CAs, so we might as well create it for all IB devices... as I see it
the advantages of having it core code are:

 - no need to expose internals of sysfs code port structure to low level
   drivers (we could also avoid this layering violation by giving a
   generic way for low-level drivers to add port attributes)
 - IB-specified info is available for all IB devices with the same
   format etc.  It may not be important for non-qlogic devices but there
   is some utility in SL mapping for debugging etc.

the only disadvantage I see is that it adds the overhead of having those
sysfs attributes for all systems with an RDMA devices, even if the
qlogic driver is never loaded.  But that overhead is pretty much just a
small amount of extra code that will never be run and a few sysfs
structures that will never be touched, so it just takes up a little bit
of memory.  For RDMA-using systems, I can't imagine it matters.

 - R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/core: export struct ib_port

[Ralph Campbell]

On Wed, 2009-11-11 at 15:02 -0800, Roland Dreier wrote:
> > | Hmm, maybe we should just add a vls directory with sl0 ... sl15 or
>  > | something like that in generic code?  I don't see why this needs to be
>  > | driver-specific code.
>  > 
>  > No particular reason, it just didn't seem likely to be useful on other
>  > HCA drivers.   I can redo the patches that way, if people think it's
>  > the right thing to do.
> 
> To me it does seem like something generic.  SLtoVL table is required of
> all CAs, so we might as well create it for all IB devices... as I see it
> the advantages of having it core code are:
> 
>  - no need to expose internals of sysfs code port structure to low level
>    drivers (we could also avoid this layering violation by giving a
>    generic way for low-level drivers to add port attributes)
>  - IB-specified info is available for all IB devices with the same
>    format etc.  It may not be important for non-qlogic devices but there
>    is some utility in SL mapping for debugging etc.
> 
> the only disadvantage I see is that it adds the overhead of having those
> sysfs attributes for all systems with an RDMA devices, even if the
> qlogic driver is never loaded.  But that overhead is pretty much just a
> small amount of extra code that will never be run and a few sysfs
> structures that will never be touched, so it just takes up a little bit
> of memory.  For RDMA-using systems, I can't imagine it matters.
> 
>  - R.

While this is true for SLtoVL, we create other files which are
device specific under the port directory too.
It seems like we might need to introduce a callback into the driver to
create the port specific sysfs files.

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

Re: [PATCH] IB/core: export struct ib_port

[Roland Dreier]

 > While this is true for SLtoVL, we create other files which are
 > device specific under the port directory too.
 > It seems like we might need to introduce a callback into the driver to
 > create the port specific sysfs files.

Umm, you could have said there were other things initially!

Anyway, rather than a callback, I guess we could just add a place to
attach a set of port attributes to the structure that gets passed into
ib_register_device() maybe?

And maybe we could clean up the existing code that does
device_create_file() to use a list of device attributes also...

 - R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/core: export struct ib_port

[Dave Olson]

On Wed, 11 Nov 2009, Roland Dreier wrote:

| 
|  > While this is true for SLtoVL, we create other files which are
|  > device specific under the port directory too.
|  > It seems like we might need to introduce a callback into the driver to
|  > create the port specific sysfs files.
| 
| Umm, you could have said there were other things initially!

Those have been there "forever" in qib without requiring the change
in the core sysfs code.  It's only sysfs group entries that require
the patch to expose ib_port.

| Anyway, rather than a callback, I guess we could just add a place to
| attach a set of port attributes to the structure that gets passed into
| ib_register_device() maybe?

Seems like major overkill to have callbacks, when all we need is to
get the structure that "owns" (is the parent kobject of) the directory.

| And maybe we could clean up the existing code that does
| device_create_file() to use a list of device attributes also...

Seems to be a rather different issue, to me.

Dave Olson
dave.olson-h88ZbnxC6KDQT0dZR+AlfA@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/core: export struct ib_port

[Roland Dreier]

 > |  > While this is true for SLtoVL, we create other files which are
 > |  > device specific under the port directory too.
 > |  > It seems like we might need to introduce a callback into the driver to
 > |  > create the port specific sysfs files.
 > | 
 > | Umm, you could have said there were other things initially!
 > 
 > Those have been there "forever" in qib without requiring the change
 > in the core sysfs code.  It's only sysfs group entries that require
 > the patch to expose ib_port.

OK, I'm confused.  Ralph originally said:

 > It [struct ib_port] is used by the new ib_qib driver to expose the SL
 > to VL table since the user level MPI library (libpsm) constructs
 > packets including the IB header.

So if that's the only use, then I'd be in favor of just exposing the
standard, generic SL-to-VL table info for all IB devices.  If there are
other per-port device-specific things, then let's give a clean way for
devices to add per-port attributes without having to know the internals
of how the RDMA core implements sysfs stuff.

 > | Anyway, rather than a callback, I guess we could just add a place to
 > | attach a set of port attributes to the structure that gets passed into
 > | ib_register_device() maybe?
 > 
 > Seems like major overkill to have callbacks, when all we need is to
 > get the structure that "owns" (is the parent kobject of) the directory.

Yes, I agree that callbacks aren't really the best way.  I was
suggesting passing in the per-port attributes as part of the ib_device
structure in ib_register_device().

 > | And maybe we could clean up the existing code that does
 > | device_create_file() to use a list of device attributes also...
 > 
 > Seems to be a rather different issue, to me.

Yes, it's independent.  But if we're passing in per-port attributes, we
might as well take the opportunity to make the API rational and pass in
per-device attributes too.

 - R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/core: export struct ib_port

[Dave Olson]

On Thu, 12 Nov 2009, Roland Dreier wrote:

| 
|  > |  > While this is true for SLtoVL, we create other files which are
|  > |  > device specific under the port directory too.
|  > |  > It seems like we might need to introduce a callback into the driver to
|  > |  > create the port specific sysfs files.
|  > | 
|  > | Umm, you could have said there were other things initially!
|  > 
|  > Those have been there "forever" in qib without requiring the change
|  > in the core sysfs code.  It's only sysfs group entries that require
|  > the patch to expose ib_port.
| 
| OK, I'm confused.  Ralph originally said:
| 
|  > It [struct ib_port] is used by the new ib_qib driver to expose the SL
|  > to VL table since the user level MPI library (libpsm) constructs
|  > packets including the IB header.
| 
| So if that's the only use, then I'd be in favor of just exposing the
| standard, generic SL-to-VL table info for all IB devices.  If there are
| other per-port device-specific things, then let's give a clean way for
| devices to add per-port attributes without having to know the internals
| of how the RDMA core implements sysfs stuff.

The other per-port stuff doesn't require ib_port, because it's not
using the same parent kobject.   They are separate directories with
separate files, not groups attached to the port kobject.

Ralph is correct, but the answer is somewhat misleading, in his response to
your reply.  The new SL2VL table is the only thing in the HCA driver
that needs ib_port to be exposed so far, and "likely" to be the only
thing ever.

| Yes, I agree that callbacks aren't really the best way.  I was
| suggesting passing in the per-port attributes as part of the ib_device
| structure in ib_register_device().

For us, that would be inconvenient, although we could probably make
it work.   And it's not just an attribute list, we'd then have to
export functions somehow (via a pointer table, presumably).  It all
seems pretty inflexible, and a lot more work, for no real gain.

Dave Olson
dave.olson-h88ZbnxC6KDQT0dZR+AlfA@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/core: export struct ib_port

[Jason Gunthorpe]

On Wed, Nov 11, 2009 at 03:22:50PM -0800, Ralph Campbell wrote:

> While this is true for SLtoVL, we create other files which are
> device specific under the port directory too.
> It seems like we might need to introduce a callback into the driver to
> create the port specific sysfs files.

Maybe give some thought to using a syscall interface through uverbs
for some of this?

IMHO, sysfs is getting out of hand for rdma:

$ find /sys/class/infiniband/mlx4_0 -type f | wc -l
660
$ strace -o /tmp/t /opt/ofa-1.5/sbin/perfquery ; grep sys/ /tmp/t | wc -l
289

That is alot of syscalls just to send two SMPs.

It just seems to me there are not that many examples of APIs that
require so much trundling through sysfs to do common every day
application tasks.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/core: export struct ib_port

[Roland Dreier]

 > Maybe give some thought to using a syscall interface through uverbs
 > for some of this?

Actually I think for exposing SL-to-VL and other things like that, sysfs
is pretty good.  Having something usable from both scripts and programs
seems pretty useful, and having an opaque uverbs interface isn't really
an improvement (especially when we have to design something extensible
that device-specific stuff can be put into).

 > IMHO, sysfs is getting out of hand for rdma:

I'm not sure how much of a problem this really is...

 > $ find /sys/class/infiniband/mlx4_0 -type f | wc -l
 > 660

and presumably 512 of those are gid and pkey table entries?

 > $ strace -o /tmp/t /opt/ofa-1.5/sbin/perfquery ; grep sys/ /tmp/t | wc -l
 > 289

That seems a little crazy, but maybe it's an app that's doing silly
stuff?  If I do ibv_rc_pingpong, the only /sys related things I see are:

    open("/sys/class/infiniband_verbs/abi_version", O_RDONLY) = 3
    open("/sys/class/infiniband_verbs", O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC) = 3
    stat("/sys/class/infiniband_verbs/abi_version", {st_mode=S_IFREG|0444, st_size=4096, ...}) = 0
    stat("/sys/class/infiniband_verbs/uverbs0", {st_mode=S_IFDIR|0755, st_size=0, ...}) = 0
    open("/sys/class/infiniband_verbs/uverbs0/ibdev", O_RDONLY) = 4
    open("/sys/class/infiniband_verbs/uverbs0/abi_version", O_RDONLY) = 4
    open("/sys/class/infiniband_verbs/uverbs0/device/vendor", O_RDONLY) = 3
    open("/sys/class/infiniband_verbs/uverbs0/device/device", O_RDONLY) = 3
    open("/sys/class/infiniband/mlx4_0/node_type", O_RDONLY) = 3

which is reasonable I think.

 - R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/core: export struct ib_port

[Jason Gunthorpe]

On Wed, Nov 11, 2009 at 04:04:10PM -0800, Roland Dreier wrote:
> 
>  > Maybe give some thought to using a syscall interface through uverbs
>  > for some of this?
> 
> Actually I think for exposing SL-to-VL and other things like that, sysfs
> is pretty good.  Having something usable from both scripts and programs
> seems pretty useful, and having an opaque uverbs interface isn't really
> an improvement (especially when we have to design something extensible
> that device-specific stuff can be put into).

I guess it depends on the purpose, a noticable problem with sysfs is
that there is no good way to be notified when the data changes. PKey,
SL2VL, GID tables, sm_lid etc are all SM dynamic information and many
cases that are using them should probably have code to know when the
SM changes them and make appropriate adjustments.

For instance a long running SMP using program has no way to be
notified when the sm_lid changes, or the GID table changes - but it
can pick up an IB async event for the pkey table changes.. What should
new things do?

It also means we can never have something like ifrename for IB - too
racey with sysfs.

>  > IMHO, sysfs is getting out of hand for rdma:
> 
> I'm not sure how much of a problem this really is...

Neither am I.. But I've seen the various eternal lkml arguments about
sysfs, netlink, syscall, etc and it does seem like the preferred
option is a little bit of all them. It does seem worth asking from
time to time if the rdma stuff in sysfs is appropriate.

>  > $ find /sys/class/infiniband/mlx4_0 -type f | wc -l
>  > 660
> 
> and presumably 512 of those are gid and pkey table entries?

Probably. TBH, those are the ones I find most un-sysfs-like..

>  > $ strace -o /tmp/t /opt/ofa-1.5/sbin/perfquery ; grep sys/ /tmp/t | wc -l
>  > 289
> 
> That seems a little crazy, but maybe it's an app that's doing silly
> stuff?  If I do ibv_rc_pingpong, the only /sys related things I see are:

It is reading the pkey and gid tables for some reason. There is no
other way to get that data except by trundling through sysfs.. Which I
guess really is my point - it isn't so much that the stuff is in sysfs
that is strange, but that it is *only* in sysfs.

>     open("/sys/class/infiniband_verbs/abi_version", O_RDONLY) = 3
>     open("/sys/class/infiniband_verbs", O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC) = 3
>     stat("/sys/class/infiniband_verbs/abi_version", {st_mode=S_IFREG|0444, st_size=4096, ...}) = 0
>     stat("/sys/class/infiniband_verbs/uverbs0", {st_mode=S_IFDIR|0755, st_size=0, ...}) = 0
>     open("/sys/class/infiniband_verbs/uverbs0/ibdev", O_RDONLY) = 4
>     open("/sys/class/infiniband_verbs/uverbs0/abi_version", O_RDONLY) = 4
>     open("/sys/class/infiniband_verbs/uverbs0/device/vendor", O_RDONLY) = 3
>     open("/sys/class/infiniband_verbs/uverbs0/device/device", O_RDONLY) = 3
>     open("/sys/class/infiniband/mlx4_0/node_type", O_RDONLY) = 3
> 
> which is reasonable I think.

Yes, I also think that is pretty much fine.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html