srp_transport: Fix atttribute registration race

srp_transport: Fix atttribute registration race

[Bart Van Assche]

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.

Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
Cc: FUJITA Tomonori <fujita.tomonori-Zyj7fXuS5i5L9jVzuh4AOg@public.gmane.org>
Cc: Brian King <brking-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
Cc: David Dillow <dillowda-1Heg1YXhbW8@public.gmane.org>
Cc: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>
Cc: <stable-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/scsi/scsi_transport_srp.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/scsi_transport_srp.c b/drivers/scsi/scsi_transport_srp.c
index 21a045e..07c4394 100644
--- a/drivers/scsi/scsi_transport_srp.c
+++ b/drivers/scsi/scsi_transport_srp.c
@@ -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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: srp_transport: Fix atttribute registration race

[James Bottomley]

On Fri, 2011-10-21 at 18:57 +0200, Bart Van Assche wrote: 
> 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.

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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: srp_transport: Fix atttribute registration race

[Bart Van Assche]

On Mon, Oct 31, 2011 at 10:33 AM, James Bottomley
<James.Bottomley-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> wrote:
> On Fri, 2011-10-21 at 18:57 +0200, Bart Van Assche wrote:
> > 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.
>
> 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.

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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: srp_transport: Fix atttribute registration race

[Bill Boas]

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: bboas-klaOcWyJdxkshyMvu7JE4pqQE7yCjDx5@public.gmane.org


 
-----Original Message-----
From: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
[mailto:linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Bart Van Assche
Sent: Tuesday, November 01, 2011 1:48 PM
To: James Bottomley
Cc: linux-scsi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Fujita Tomonori;
Brian King; David Dillow; Roland Dreier
Subject: Re: srp_transport: Fix atttribute registration race

On Mon, Oct 31, 2011 at 10:33 AM, James Bottomley
<James.Bottomley-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> wrote:
> On Fri, 2011-10-21 at 18:57 +0200, Bart Van Assche wrote:
> > 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.
>
> 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.

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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: srp_transport: Fix atttribute registration race

[Dave Dillow]

On Sat, Nov 12, 2011 at 11:18:04PM -0500, Bill Boas wrote:
> 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.

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

RE: srp_transport: Fix atttribute registration race

[Paul Grun]

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


> -----Original Message-----
> From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma-
> owner@vger.kernel.org] On Behalf Of Dave Dillow
> Sent: Sunday, November 13, 2011 1:55 PM
> To: Bill Boas
> Cc: 'Bart Van Assche'; 'James Bottomley'; linux-scsi@vger.kernel.org;
> linux-rdma@vger.kernel.org; 'Fujita Tomonori'; 'Brian King'; 'Roland
> Dreier'
> Subject: Re: srp_transport: Fix atttribute registration race
> 
> On Sat, Nov 12, 2011 at 11:18:04PM -0500, Bill Boas wrote:
> > 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.
> 
> 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
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: srp_transport: Fix atttribute registration race

[Or Gerlitz]

On Sun, Nov 13, 2011 at 11:55 PM, Dave Dillow <dillowda-1Heg1YXhbW8@public.gmane.org> wrote:
> SRP uses RDMA, so you cannot use UC mode.

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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: srp_transport: Fix atttribute registration race

[Dave Dillow]

On Mon, Nov 14, 2011 at 04:43:32PM -0500, Or Gerlitz wrote:
> On Sun, Nov 13, 2011 at 11:55 PM, Dave Dillow <dillowda@ornl.gov> wrote:
> > SRP uses RDMA, so you cannot use UC mode.
> 
> per the IB spec, RDMA write is supported for UC

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

Re: srp_transport: Fix atttribute registration race

[Bart Van Assche]

On Mon, Nov 14, 2011 at 10:43 PM, Or Gerlitz <or.gerlitz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Sun, Nov 13, 2011 at 11:55 PM, Dave Dillow <dillowda-1Heg1YXhbW8@public.gmane.org> wrote:
> > SRP uses RDMA, so you cannot use UC mode.
>
> per the IB spec, RDMA write is supported for UC

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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html