srp_transport: Fix atttribute registration race

Register transport attributes after the attribute array has been set up instead of before. The current code is racy because there is no guarantee that the CPU examining the attribute container will see all values written to the container.

---
 drivers/scsi/scsi_transport_srp.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

index 21a045e..07c4394 100644
@@ -324,13 +324,14 @@ srp_attach_transport(struct srp_function_template *ft)
 	i->rport_attr_cont.ac.attrs = &i->rport_attrs[0];
 	i->rport_attr_cont.ac.class = &srp_rport_class.class;
 	i->rport_attr_cont.ac.match = srp_rport_match;
-	transport_container_register(&i->rport_attr_cont);
 
 	count = 0;
 	SETUP_RPORT_ATTRIBUTE_RD(port_id);
 	SETUP_RPORT_ATTRIBUTE_RD(roles);
 	i->rport_attrs[count] = NULL;
 
+	transport_container_register(&i->rport_attr_cont);
+
 	i->f = ft;
 
 	return &i->t;

1.7.3.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

I don't agree with this change log. As far as the kernel is concerned, nothing happens until that function returns because the only way to use anything is to get a match to succeed, and they all check for ->transportt which will be NULL.

I can accept that it's best practise to initialise something before registering it, because the reverse excites everyone's bogosity sensors, it's just not a bug in this case.

James

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

It's very well possible that I have missed something, but it's not clear to me what's wrong with the patch description ? At least with ib_srp device registration happens from another context than module initialization so it can potentially run on another CPU. I don't see any kind of memory barrier after the initialization of the SRP transport attribute array. So at least in theory and on an architecture with a sufficiently weak memory model (e.g. the POWER architecture) there is no guarantee that these store operations will be observed before an SRP device gets registered. Even more, there is no guarantee that the CPU on which device registration happens will observe these store operations in the same order as these were performed on the CPU that ran srp_attach_transport().

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Bart, James, Dave,

At SC we are running a video streaming demo across a 7000 mile fiber distance using IB extension boxes from Bay Microsystems at 40 Gbps. The video streaming app uses SRP to transmit the streams across this long distance link. Performance is not all what we would have hoped. We think that because SRP uses RC mode between initiator and target that it is the RC behavior over this distance that is impacting performance. Are any of you aware of an SRP implementation that uses UC mode.

We appreciate your respones in advance,

Bill
Bill Boas
Tel: 01-510-375-8840
Email: [email protected]

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Bart Van Assche
Sent: Tuesday, November 01, 2011 1:48 PM
To: James Bottomley
Cc: [email protected]; [email protected]; Fujita Tomonori; Brian King; David Dillow; Roland Dreier
Subject: Re: srp_transport: Fix atttribute registration race

It's very well possible that I have missed something, but it's not clear to me what's wrong with the patch description ? At least with ib_srp device registration happens from another context than module initialization so it can potentially run on another CPU. I don't see any kind of memory barrier after the initialization of the SRP transport attribute array. So at least in theory and on an architecture with a sufficiently weak memory model (e.g. the POWER
architecture) there is no guarantee that these store operations will be observed before an SRP device gets registered. Even more, there is no guarantee that the CPU on which device registration happens will observe these store operations in the same order as these were performed on the CPU that ran srp_attach_transport().

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at
http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

SRP uses RDMA, so you cannot use UC mode.

Your problem sounds a lot like issues we saw when starting to run over 100m+ optical cables; IIRC it required increasing the credits available the link. You should contact Jason Gunthorpe or Makia Minnich. Makia was involved in that investigation at ORNL, but Jason may also be able to tell you off the top of his head.

Dave

Thanks for the suggestions. The problem turned out to be much more pedestrian...the destination MAC ID of packets being injected into the long distance link was wrong and thus many packets were being dropped. We haven't gotten back to trying to run RC over the long distance link. I understand the issues with running SRP over UC, and don't expect to do that. Thanks again,
-Paul Grun

per the IB spec, RDMA write is supported for UC

Or.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Yeah, I read that as UD for some reason...

Agreed. But an SRP target does not only issue RDMA write requests, it also issues RDMA read requests. If I interpret the IB spec correctly, RDMA reads are not supported for UC.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html