Re: [ewg] Re: [PATCH] rdmaoe/libibverbs: handle binary compatibility

2009-12-10 Thread Jason Gunthorpe
On Thu, Dec 10, 2009 at 01:57:11PM -0800, Roland Dreier wrote:

> Maybe I'm wrong but I don't like setting "don't know" magically to IB
> behind the scenes.

Well, it isn't just "don't know" it also means "the kernel doesn't
support the link_layer query". The kernels that don't support
link_layer also only support link_layer == IB for
IBV_TRANSPORT_IB. One proves the other..

So IBV_TRANSPORT_IB ports should always return
IBV_LINK_LAYER_INFINIBAND unless the kernel supports the new API and
says otherwise. You can still have IBV_LINK_LAYER_UNSPECIFIED, but it
can't be 0.

If this isn't done then again compatability with apps is compromised,
what should an app do when it sees IBV_LINK_LAYER_UNSPECIFIED due to
an older kernel and newer libibverbs?

> And there's no reason why iWARP has to run over ethernet in
> principle.

Right.. so existing kernels with the new library should return
IBV_LINK_LAYER_INFINIBAND for ports on IBV_TRANSPORT_IB devices and
IBV_LINK_LAYER_UNSPECIFIED for ports on everything else
(IBV_TRANSPORT_IWARP, IBV_TRANSPORT_UNKNOWN)

Jason
___
ewg mailing list
ewg@lists.openfabrics.org
http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ewg


Re: [ewg] Re: [PATCH] rdmaoe/libibverbs: handle binary compatibility

2009-12-10 Thread Roland Dreier

 > >  #define IB_USER_VERBS_MIN_ABI_VERSION  1
 > > -#define IB_USER_VERBS_MAX_ABI_VERSION  6
 > > +#define IB_USER_VERBS_MAX_ABI_VERSION  7
 >  
 > Whats this about? That seems like it needs a much bigger review,
 > changing the kernel ABI version instantly breaks every existing
 > libibverbs, shouldn't be done without alot of discussion!!

Yes, I've been meaning to bring this up earlier.

There was a time, long ago, when changing this ABI was maybe OK.  I
think even when I first designed the uverbs ABI, having this version
instead of capability bits was a goof.  But I don't have a time machine.

In any case I think at this point we need to stick with ABI version 6
will full backwards compat unless we really really can't.  And I don't
think the changes here are nearly drastic enough that we can't figure
out a way forward without breaking things.

 > Why do you have IBV_LINK_LAYER_UNSPECIFIED ? That seems pointless. Why
 > should existing kernels return UNSPECIFIED when we know they are
 > always IB. I'd say drop IBV_LINK_LAYER_UNSPECIFIED and set
 > IBV_LINK_LAYER_INFINIBAND to 0.

I prefer having the UNSPECIFIED.  Not a strong preference but my
reasoning is that if you have an old kernel that doesn't answer
anything, you're better off letting the app see that.  And there's no
reason why iWARP has to run over ethernet in principle.

Maybe I'm wrong but I don't like setting "don't know" magically to IB
behind the scenes.

 - R.
___
ewg mailing list
ewg@lists.openfabrics.org
http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ewg


RE: [ewg] Re: [PATCH] rdmaoe/libibverbs: handle binary compatibility

2009-12-10 Thread Sean Hefty
>@@ -306,7 +314,7 @@ static struct pingpong_context *pp_init_ctx(struct
>ibv_device *ib_dev, int size,
>   return NULL;
>   }
>
>-  memset(ctx->buf, 0, size);
>+  memset(ctx->buf, 0x7b + is_server, size);
>
>   ctx->context = ibv_open_device(ib_dev);
>   if (!ctx->context) {
>@@ -481,6 +489,7 @@ static void usage(const char *argv0)
>   printf("  -n, --iters=number of exchanges (default 1000)\n");
>   printf("  -l, --sl=  service level value\n");
>   printf("  -e, --events   sleep on CQ events (default poll)\n");
>+  printf("  -g, --gid= gid of the other port\n");

It seems easier for the user if the tests discovered its local GID and exchanged
it with the remote side like it does with the LID and QPN.
 
>diff --git a/include/infiniband/kern-abi.h b/include/infiniband/kern-abi.h
>index 0db083a..8ef8844 100644

>
> /*
>@@ -223,7 +224,8 @@ struct ibv_query_port_resp {
>   __u8  active_width;
>   __u8  active_speed;
>   __u8  phys_state;
>-  __u8  reserved[3];
>+  __u8  link_layer;
>+  __u8  reserved[2];
> };

Should we define the network layer too?  Right now we have IB transport, which
assumes IB network and link; iWarp transport, which almost assumes IP network
and Ethernet; and RDMAoE, which may or may not have a network (but requires the
L3 address) and Ethernet (or is it DCB) link.

- Sean

___
ewg mailing list
ewg@lists.openfabrics.org
http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ewg


Re: [ewg] Re: [PATCH] rdmaoe/libibverbs: handle binary compatibility

2009-12-10 Thread Jason Gunthorpe
On Thu, Dec 10, 2009 at 11:14:55PM +0200, Eli Cohen wrote:
> On Thu, Dec 10, 2009 at 10:33:53AM -0700, Jason Gunthorpe wrote:
> > Could you prepare this based on Roland's tree? This patch won't apply.
> > 
> 
> I quote two patches, one for libibverbs based on 74638ac, and the
> other for libmlx4 based on 444f634. I changed the padding handling as
> you requested for libibverbs. You also need to apply a patch to the
> kernel driver to match the new values for link_layer. I put it here
> too.

Seems like this will work to me. I think you need to split this patch
if you want Roland to apply it. I'd suggest

- Change the library API for ibv_port_attr to include a link_layer
- Change the kernel API to retrieve link_layer
- Add ibv_cmd_get_mac and other stuff to support RDMAoE
- Update verbs examples to support RDMAoE
- Update man pages (you missed these)

>--- a/include/infiniband/kern-abi.h
>+++ b/include/infiniband/kern-abi.h
>@@ -46,7 +46,7 @@
>  * The minimum and maximum kernel ABI that we can handle.
>  */
>  #define IB_USER_VERBS_MIN_ABI_VERSION  1
> -#define IB_USER_VERBS_MAX_ABI_VERSION  6
> +#define IB_USER_VERBS_MAX_ABI_VERSION  7
 
Whats this about? That seems like it needs a much bigger review,
changing the kernel ABI version instantly breaks every existing
libibverbs, shouldn't be done without alot of discussion!!

> +enum {
> + IBV_LINK_LAYER_UNSPECIFIED,
> + IBV_LINK_LAYER_INFINIBAND,
> + IBV_LINK_LAYER_ETHERNET,
> +};

Why do you have IBV_LINK_LAYER_UNSPECIFIED ? That seems pointless. Why
should existing kernels return UNSPECIFIED when we know they are
always IB. I'd say drop IBV_LINK_LAYER_UNSPECIFIED and set
IBV_LINK_LAYER_INFINIBAND to 0.

What are iWarp devices going to return? Seems like the verbs library
should probably force link_layer to ethernet for iwarp devices, for
compatability.

> diff --git a/src/cmd.c b/src/cmd.c
> index cbd5288..5183d59 100644
> +++ b/src/cmd.c
> @@ -162,6 +162,7 @@ int ibv_cmd_query_device(struct ibv_context *context,
>   return 0;
>  }
>  
> +#include 
>  int ibv_cmd_query_port(struct ibv_context *context, uint8_t port_num,
>  struct ibv_port_attr *port_attr,
>  struct ibv_query_port *cmd, size_t cmd_size)

Extra include?

>@@ -86,6 +86,7 @@ default_symver(__ibv_query_device, ibv_query_device);
> int __ibv_query_port(struct ibv_context *context, uint8_t port_num,
> struct ibv_port_attr *port_attr)
> {
>+   port_attr->link_layer = IBV_LINK_LAYER_UNSPECIFIED;
>return context->ops.query_port(context, port_num, port_attr);
> }

Seems like this should be just
 return ___ibv_query_port(context,port_num,port_attr);

> diff --git a/drivers/infiniband/core/uverbs_cmd.c 
> b/drivers/infiniband/core/uverbs_cmd.c
> index 012aadf..d592bd2 100644
> +++ b/drivers/infiniband/core/uverbs_cmd.c
> @@ -452,7 +452,8 @@ ssize_t ib_uverbs_query_port(struct ib_uverbs_file *file,
>   resp.active_width= attr.active_width;
>   resp.active_speed= attr.active_speed;
>   resp.phys_state  = attr.phys_state;
> - resp.transport   = attr.transport;
> + resp.transport   = attr.transport == RDMA_TRANSPORT_RDMAOE ?
> + IB_LINK_LAYER_ETHERNET : IB_LINK_LAYER_INFINIBAND;

Are you going to change the kernel patches to use the new link_layer
name?

Jason
___
ewg mailing list
ewg@lists.openfabrics.org
http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ewg


Re: [ewg] Re: [PATCH] rdmaoe/libibverbs: handle binary compatibility

2009-12-10 Thread Eli Cohen
On Thu, Dec 10, 2009 at 10:33:53AM -0700, Jason Gunthorpe wrote:
> Could you prepare this based on Roland's tree? This patch won't apply.
> 

I quote two patches, one for libibverbs based on 74638ac, and the
other for libmlx4 based on 444f634. I changed the padding handling as
you requested for libibverbs. You also need to apply a patch to the
kernel driver to match the new values for link_layer. I put it here
too.

libibverbs:


diff --git a/examples/devinfo.c b/examples/devinfo.c
index 84f95c7..393ec04 100644
--- a/examples/devinfo.c
+++ b/examples/devinfo.c
@@ -184,6 +184,19 @@ static int print_all_port_gids(struct ibv_context *ctx, 
uint8_t port_num, int tb
return rc;
 }
 
+static const char *link_layer_str(uint8_t link_layer)
+{
+   switch (link_layer) {
+   case IBV_LINK_LAYER_UNSPECIFIED:
+   case IBV_LINK_LAYER_INFINIBAND:
+   return "IB";
+   case IBV_LINK_LAYER_ETHERNET:
+   return "Ethernet";
+   default:
+   return "Unknown";
+   }
+}
+
 static int print_hca_cap(struct ibv_device *ib_dev, uint8_t ib_port)
 {
struct ibv_context *ctx;
@@ -284,6 +297,7 @@ static int print_hca_cap(struct ibv_device *ib_dev, uint8_t 
ib_port)
printf("\t\t\tsm_lid:\t\t\t%d\n", port_attr.sm_lid);
printf("\t\t\tport_lid:\t\t%d\n", port_attr.lid);
printf("\t\t\tport_lmc:\t\t0x%02x\n", port_attr.lmc);
+   printf("\t\t\tlink_layer:\t\t%s\n", 
link_layer_str(port_attr.link_layer));
 
if (verbose) {
printf("\t\t\tmax_msg_sz:\t\t0x%x\n", 
port_attr.max_msg_sz);
diff --git a/examples/pingpong.c b/examples/pingpong.c
index b916f59..d4a46e4 100644
--- a/examples/pingpong.c
+++ b/examples/pingpong.c
@@ -31,6 +31,8 @@
  */
 
 #include "pingpong.h"
+#include 
+#include 
 
 enum ibv_mtu pp_mtu_to_enum(int mtu)
 {
@@ -53,3 +55,10 @@ uint16_t pp_get_local_lid(struct ibv_context *context, int 
port)
 
return attr.lid;
 }
+
+int pp_get_port_info(struct ibv_context *context, int port,
+struct ibv_port_attr *attr)
+{
+   return ibv_query_port(context, port, attr);
+}
+
diff --git a/examples/pingpong.h b/examples/pingpong.h
index 71d7c3f..16d3466 100644
--- a/examples/pingpong.h
+++ b/examples/pingpong.h
@@ -37,5 +37,7 @@
 
 enum ibv_mtu pp_mtu_to_enum(int mtu);
 uint16_t pp_get_local_lid(struct ibv_context *context, int port);
+int pp_get_port_info(struct ibv_context *context, int port,
+struct ibv_port_attr *attr);
 
 #endif /* IBV_PINGPONG_H */
diff --git a/examples/rc_pingpong.c b/examples/rc_pingpong.c
index fa969e0..4d0bd0d 100644
--- a/examples/rc_pingpong.c
+++ b/examples/rc_pingpong.c
@@ -67,6 +67,8 @@ struct pingpong_context {
int  size;
int  rx_depth;
int  pending;
+   struct ibv_port_attr portinfo;
+   union ibv_giddgid;
 };
 
 struct pingpong_dest {
@@ -94,6 +96,12 @@ static int pp_connect_ctx(struct pingpong_context *ctx, int 
port, int my_psn,
.port_num   = port
}
};
+
+   if (ctx->dgid.global.interface_id) {
+   attr.ah_attr.is_global = 1;
+   attr.ah_attr.grh.hop_limit = 1;
+   attr.ah_attr.grh.dgid = ctx->dgid;
+   }
if (ibv_modify_qp(ctx->qp, &attr,
  IBV_QP_STATE  |
  IBV_QP_AV |
@@ -289,11 +297,11 @@ out:
 
 static struct pingpong_context *pp_init_ctx(struct ibv_device *ib_dev, int 
size,
int rx_depth, int port,
-   int use_event)
+   int use_event, int is_server)
 {
struct pingpong_context *ctx;
 
-   ctx = malloc(sizeof *ctx);
+   ctx = calloc(1, sizeof *ctx);
if (!ctx)
return NULL;
 
@@ -306,7 +314,7 @@ static struct pingpong_context *pp_init_ctx(struct 
ibv_device *ib_dev, int size,
return NULL;
}
 
-   memset(ctx->buf, 0, size);
+   memset(ctx->buf, 0x7b + is_server, size);
 
ctx->context = ibv_open_device(ib_dev);
if (!ctx->context) {
@@ -481,6 +489,7 @@ static void usage(const char *argv0)
printf("  -n, --iters=number of exchanges (default 1000)\n");
printf("  -l, --sl=  service level value\n");
printf("  -e, --events   sleep on CQ events (default poll)\n");
+   printf("  -g, --gid= gid of the other port\n");
 }
 
 int main(int argc, char *argv[])
@@ -504,6 +513,7 @@ int main(int argc, char *argv[])
int  rcnt, scnt;
int  num_cq_events = 0;
int  sl = 0;
+   char*grh = NULL;
 
srand48(getpid() * time(NULL));
 
@@ -520,10 +530,11 @@ int ma

[ewg] Re: [PATCH] rdmaoe/libibverbs: handle binary compatibility

2009-12-10 Thread Jason Gunthorpe
On Thu, Dec 10, 2009 at 07:05:36PM +0200, Eli Cohen wrote:

> here is the patch I prepared based on the discussions we had. The patch is 
> based on the last
> rdmaoe/libibverbs patch I sent. libmlx4 was modified too, a trivial
> change that changes name. Both fixes were push to OFED. I will send a
> new patch set in a few days.

Could you prepare this based on Roland's tree? This patch won't apply.

Why are things still going into OFED so quickly? Shouldn't Roland
accept this stuff before it gets to OFED - or at least review it once..

> +static inline int ___ibv_query_port(struct ibv_context *context,
> + uint8_t port_num,
> + struct ibv_port_attr *port_attr)
> +{
> + uint8_t *padp;
> + int padsize;
> +
> + port_attr->link_layer = IBV_LINK_LAYER_UNSPECIFIED;
> + padp = &port_attr->link_layer + sizeof port_attr->link_layer;
> + padsize = sizeof(int) - ((unsigned long)padp & (sizeof(int) - 1));
> + memset(padp, 0, padsize);
> +
> + return context->ops.query_port(context, port_num, port_attr);
> +}

You should explicity declare the padding, like ibv_query_port_resp
does, the above is very fragile.

Jason
___
ewg mailing list
ewg@lists.openfabrics.org
http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ewg


[ewg] New Perftest release version 1.2.3

2009-12-10 Thread Ido Shamai

Hey Everyone ,

New perftest release is out (Not included in Ofed_1_5 rc 4).

* Git can be cloned from - git://git.openfabrics.org/~shamoya/perftest.git
* Tarball can be downloaded from - 
http://www.openfabrics.org/downloads/perftest/perftest-1.2.3-0.8.g196d994.tar.gz



list of changes:
  *  Multicast feature in ib_send_bw and ib_send_lat - one group with 
up to 56 qps attached to it + all previous flags.

  *  -M flag - specify the MGID of the Mcast group from command line.
  *  Modified the inline for all micro benchmarks.
  *  Release methods in send_bw and send_lat.
  *  Fixed 2 bugs in read_bw.c

* Waiting for replys/remarks etc..

Regards
Ido
___
ewg mailing list
ewg@lists.openfabrics.org
http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ewg


[ewg] [PATCH] rdmaoe/libibverbs: handle binary compatibility

2009-12-10 Thread Eli Cohen
Hi,
here is the patch I prepared based on the discussions we had. The patch is 
based on the last
rdmaoe/libibverbs patch I sent. libmlx4 was modified too, a trivial
change that changes name. Both fixes were push to OFED. I will send a
new patch set in a few days.

The introduction of a new field to struct ibv_port_attr to distinguish between
the different link layers supported (IB or Ethernet), which is located in the
padding area of the struct, poses a binary compatibilty issue between new
applications (compiled against the new version of the struct) and older
libraries, becuase old libraries do not initialize the padding area. To
overcome this, ibv_query_port is redefined to call an inline function that sets
the link_layer field to a predefined value which is interpreted as IB (the
previous only value for this field). The patch also clears any padding left
should we need this in the future. This patch also changes the previous field
name from protocol to link_layer, and also modifies all the examples to adapt
to the change.

Solution suggested by:
Roland Dreier 
Jason Gunthorpe 
Signed-off-by: Eli Cohen 

Signed-off-by: Eli Cohen 
---
 examples/devinfo.c|   15 +++
 examples/rc_pingpong.c|2 +-
 examples/srq_pingpong.c   |2 +-
 examples/uc_pingpong.c|2 +-
 examples/ud_pingpong.c|2 +-
 include/infiniband/kern-abi.h |2 +-
 include/infiniband/verbs.h|   33 -
 src/cmd.c |2 +-
 src/verbs.c   |1 +
 9 files changed, 38 insertions(+), 23 deletions(-)

diff --git a/examples/devinfo.c b/examples/devinfo.c
index 88e0557..393ec04 100644
--- a/examples/devinfo.c
+++ b/examples/devinfo.c
@@ -184,15 +184,14 @@ static int print_all_port_gids(struct ibv_context *ctx, 
uint8_t port_num, int tb
return rc;
 }
 
-static const char *transport_type_str(enum rdma_transport_type type)
+static const char *link_layer_str(uint8_t link_layer)
 {
-   switch (type) {
-   case RDMA_TRANSPORT_IB:
+   switch (link_layer) {
+   case IBV_LINK_LAYER_UNSPECIFIED:
+   case IBV_LINK_LAYER_INFINIBAND:
return "IB";
-   case RDMA_TRANSPORT_IWARP:
-   return "IWARP";
-   case RDMA_TRANSPORT_RDMAOE:
-   return "RDMAOE";
+   case IBV_LINK_LAYER_ETHERNET:
+   return "Ethernet";
default:
return "Unknown";
}
@@ -298,7 +297,7 @@ static int print_hca_cap(struct ibv_device *ib_dev, uint8_t 
ib_port)
printf("\t\t\tsm_lid:\t\t\t%d\n", port_attr.sm_lid);
printf("\t\t\tport_lid:\t\t%d\n", port_attr.lid);
printf("\t\t\tport_lmc:\t\t0x%02x\n", port_attr.lmc);
-   printf("\t\t\ttrasnport_type:\t\t%s\n", 
transport_type_str(port_attr.transport));
+   printf("\t\t\tlink_layer:\t\t%s\n", 
link_layer_str(port_attr.link_layer));
 
if (verbose) {
printf("\t\t\tmax_msg_sz:\t\t0x%x\n", 
port_attr.max_msg_sz);
diff --git a/examples/rc_pingpong.c b/examples/rc_pingpong.c
index e0cde29..4d0bd0d 100644
--- a/examples/rc_pingpong.c
+++ b/examples/rc_pingpong.c
@@ -653,7 +653,7 @@ int main(int argc, char *argv[])
}
 
my_dest.lid = ctx->portinfo.lid;
-   if (ctx->portinfo.transport == RDMA_TRANSPORT_RDMAOE) {
+   if (ctx->portinfo.link_layer == IBV_LINK_LAYER_ETHERNET) {
if (!grh) {
fprintf(stderr, "Must supply remote gid\n");
return 1;
diff --git a/examples/srq_pingpong.c b/examples/srq_pingpong.c
index bd10f90..eda9013 100644
--- a/examples/srq_pingpong.c
+++ b/examples/srq_pingpong.c
@@ -744,7 +744,7 @@ int main(int argc, char *argv[])
for (i = 0; i < num_qp; ++i) {
my_dest[i].qpn = ctx->qp[i]->qp_num;
my_dest[i].psn = lrand48() & 0xff;
-   if (ctx->portinfo.transport == RDMA_TRANSPORT_RDMAOE) {
+   if (ctx->portinfo.link_layer == IBV_LINK_LAYER_ETHERNET) {
if (!grh) {
fprintf(stderr, "Must supply remote gid\n");
return 1;
diff --git a/examples/uc_pingpong.c b/examples/uc_pingpong.c
index 6cfffd2..2bc7da5 100644
--- a/examples/uc_pingpong.c
+++ b/examples/uc_pingpong.c
@@ -641,7 +641,7 @@ int main(int argc, char *argv[])
}
 
my_dest.lid = ctx->portinfo.lid;
-   if (ctx->portinfo.transport == RDMA_TRANSPORT_RDMAOE) {
+   if (ctx->portinfo.link_layer == IBV_LINK_LAYER_ETHERNET) {
if (!grh) {
fprintf(stderr, "Must supply remote gid\n");
return 1;
diff --git a/examples/ud_pingpong.c b/examples/ud_pingpong.c
index 33601a3..e30d6d6 100644
--- a/examples/ud_pingpong.c
+++ b/examples/ud_pingpong.c
@@ -641,7 +641,7 @@ int main(int argc, char *argv[])
 
m

[ewg] [PATCH v2] mlx4_core: Cleanup bug in __mlx4_init_one()

2009-12-10 Thread Eli Cohen
If mlx4_init_port_info() fails, cleanup the initialized ports only.

Signed-off-by: Eli Cohen 
---
Changes: rebase the patch on Roland's for-next branch

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

diff --git a/drivers/net/mlx4/main.c b/drivers/net/mlx4/main.c
index 291a505..3cf56d9 100644
--- a/drivers/net/mlx4/main.c
+++ b/drivers/net/mlx4/main.c
@@ -1174,7 +1174,7 @@ static int __mlx4_init_one(struct pci_dev *pdev, const 
struct pci_device_id *id)
return 0;
 
 err_port:
-   for (port = 1; port <= dev->caps.num_ports; port++)
+   for (--port; port >= 1; --port)
mlx4_cleanup_port_info(&priv->port[port]);
 
mlx4_cleanup_mcg_table(dev);
-- 
1.6.5.5

___
ewg mailing list
ewg@lists.openfabrics.org
http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ewg


Re: [ewg] [PATCH] mlx4: Fix bug in mlx4_ib_mcg_attach

2009-12-10 Thread Eli Cohen
On Wed, Dec 09, 2009 at 02:49:47PM -0800, Roland Dreier wrote:
> 
> I don't see anything touching this code there.  The patch that
> introduced this code upstream, 521e575b ("IB/mlx4: Add support for
> blocking multicast loopback packets") doesn't have this bug and I don't
> see anything else that changed that area of the code.
> 

Your branch, my bug :-) I put it in one of my commits.
___
ewg mailing list
ewg@lists.openfabrics.org
http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ewg


Re: [ewg] [PATCH] IPoIB: synchronize timer deletion with completion handler

2009-12-10 Thread Eli Cohen
On Wed, Dec 09, 2009 at 02:39:49PM -0800, Roland Dreier wrote:
> 
>  > When calling del_timer_sync on priv->poll_timer, it is necessary to prevent
>  > farther arming of the timer which can be done by a completion handler. 
> Though
>  > it is harmless since the timer will eventually stop being rearmed, it is 
> better
>  > practice to avoid calling the timer function after it is deleted. This 
> patch
>  > handles this by using a new flag that is checked before arming the timer.
> 
> have you seen this in practice?  If it can happen then it's not
> harmless, since the module could be unloaded with the timer pending.
> However I don't see how it could happen, since we only seem to delete
> the timer after we know that no more completions are coming (except for
> the case where we decide that the hardware is wedged but it really only
> takes a *long* time to respond at exactly the wrong time, and we somehow
> get a completion between the del_timer_sync and the modify QP to reset
> state -- which is so unlikely it seems not worth adding this extra
> complexity for -- maybe we could add the del_timer_sync to after we
> delete the CQ or something if you're really worried)

No, I haven't actually seen this happening but I think it could.
Consider this scenario:

1. ipoib_send() calls ib_req_notify_cq()
2. ipoib_ib_dev_stop() gets called
3. All pending WR complete; interrupt is generated but not yet
serviced.
4. ipoib_ib_dev_stop() calls del_timer_sync() and passes it.
5. Completion handler is finally serviced - it schedules
ipoib_ib_tx_timer_func()
6. IPoIB unloads and we're in trouble.

Not so likely to happen indeed. Your suggestion to defer deleting the
timer until after the CQ is destroyed will also solve this.
___
ewg mailing list
ewg@lists.openfabrics.org
http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ewg


[ewg] ofa_1_5_kernel 20091210-0200 daily build status

2009-12-10 Thread Vladimir Sokolovsky (Mellanox)
This email was generated automatically, please do not reply


git_url: git://git.openfabrics.org/ofed_1_5/linux-2.6.git
git_branch: ofed_kernel_1_5

Common build parameters: 

Passed:
Passed on i686 with linux-2.6.21.1
Passed on i686 with linux-2.6.19
Passed on i686 with linux-2.6.18
Passed on i686 with linux-2.6.24
Passed on i686 with linux-2.6.26
Passed on i686 with linux-2.6.22
Passed on i686 with linux-2.6.27
Passed on x86_64 with linux-2.6.16.60-0.54.5-smp
Passed on x86_64 with linux-2.6.16.60-0.21-smp
Passed on x86_64 with linux-2.6.18
Passed on x86_64 with linux-2.6.18-128.el5
Passed on x86_64 with linux-2.6.18-164.el5
Passed on x86_64 with linux-2.6.20
Passed on x86_64 with linux-2.6.19
Passed on x86_64 with linux-2.6.18-93.el5
Passed on x86_64 with linux-2.6.24
Passed on x86_64 with linux-2.6.21.1
Passed on x86_64 with linux-2.6.22
Passed on x86_64 with linux-2.6.26
Passed on x86_64 with linux-2.6.27
Passed on x86_64 with linux-2.6.25
Passed on x86_64 with linux-2.6.27.19-5-smp
Passed on x86_64 with linux-2.6.9-89.ELsmp
Passed on x86_64 with linux-2.6.9-78.ELsmp
Passed on x86_64 with linux-2.6.9-67.ELsmp
Passed on ia64 with linux-2.6.21.1
Passed on ia64 with linux-2.6.18
Passed on ia64 with linux-2.6.19
Passed on ia64 with linux-2.6.23
Passed on ia64 with linux-2.6.24
Passed on ia64 with linux-2.6.22
Passed on ia64 with linux-2.6.26
Passed on ia64 with linux-2.6.25
Passed on ppc64 with linux-2.6.18
Passed on ppc64 with linux-2.6.19

Failed:
___
ewg mailing list
ewg@lists.openfabrics.org
http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ewg