| Message ID | 1460410360-13104-3-git-send-email-hch@lst.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Looks fine,
Reviewed-by: Sagi Grimberg <sagi@grimberg.e>
--
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
On Mon, Apr 11, 2016 at 02:32:30PM -0700, Christoph Hellwig wrote: > The new RW API will need this. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > Reviewed-by: Bart Van Assche <bart.vanassche@sandisk.com> > Tested-by: Steve Wise <swise@opengridcomputing.com> I'm not opposed to this change but traditionally QPs are bound to a device not to a single port. How does this change that semantic? Ira > --- > drivers/infiniband/core/cma.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c > index 93ab0ae..6ebaf20 100644 > --- a/drivers/infiniband/core/cma.c > +++ b/drivers/infiniband/core/cma.c > @@ -800,6 +800,7 @@ int rdma_create_qp(struct rdma_cm_id *id, struct ib_pd *pd, > if (id->device != pd->device) > return -EINVAL; > > + qp_init_attr->port_num = id->port_num; > qp = ib_create_qp(pd, qp_init_attr); > if (IS_ERR(qp)) > return PTR_ERR(qp); > -- > 2.1.4 > > -- > 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 -- 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
On Mon, Apr 18, 2016 at 11:14:27PM -0400, Ira Weiny wrote: > On Mon, Apr 11, 2016 at 02:32:30PM -0700, Christoph Hellwig wrote: > > The new RW API will need this. > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > Reviewed-by: Bart Van Assche <bart.vanassche@sandisk.com> > > Tested-by: Steve Wise <swise@opengridcomputing.com> > > I'm not opposed to this change but traditionally QPs are bound to a > device not to a single port. Right, this was done because rdma_protocol_iwarp takes a port number. I think we discussed this once, the core code doesn't actually support different protocols on different ports, so the port_num argument to rdma_protocol_iwarp is redundant. This all starts to look really goofy when multi-port APM is used and the QP's port number changes dynamically at runtime. (I have some experimental patches that do that), I'd rather see all the port_num stuff in this series go away. :( Jason -- 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
On Tue, Apr 19, 2016 at 11:30:32AM -0600, Jason Gunthorpe wrote: > Right, this was done because rdma_protocol_iwarp takes a port number. > > I think we discussed this once, the core code doesn't actually support > different protocols on different ports, so the port_num argument to > rdma_protocol_iwarp is redundant. > > This all starts to look really goofy when multi-port APM is used and > the QP's port number changes dynamically at runtime. (I have some > experimental patches that do that), I'd rather see all the port_num > stuff in this series go away. :( Reall, I would _love_ to kill all that port_num crap. But until we get agreement from Doug and all the core maintainers that we can kill it from the core, and that multi-protocol devices are indeed as silly as they seem I can't.. -- 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
>>> The new RW API will need this. >>> >>> Signed-off-by: Christoph Hellwig <hch@lst.de> >>> Reviewed-by: Bart Van Assche <bart.vanassche@sandisk.com> >>> Tested-by: Steve Wise <swise@opengridcomputing.com> >> >> I'm not opposed to this change but traditionally QPs are bound to a >> device not to a single port. > > Right, this was done because rdma_protocol_iwarp takes a port number. > > I think we discussed this once, the core code doesn't actually support > different protocols on different ports, so the port_num argument to > rdma_protocol_iwarp is redundant. > > This all starts to look really goofy when multi-port APM is used and > the QP's port number changes dynamically at runtime. (I have some > experimental patches that do that), I'd rather see all the port_num > stuff in this series go away. :( HCH and I complained about this per-port distinction in several private conversations. I'd really love to see it go away too. -- 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
On Tue, Apr 19, 2016 at 09:49:03PM +0300, Sagi Grimberg wrote: > > >>>The new RW API will need this. > >>> > >>>Signed-off-by: Christoph Hellwig <hch@lst.de> > >>>Reviewed-by: Bart Van Assche <bart.vanassche@sandisk.com> > >>>Tested-by: Steve Wise <swise@opengridcomputing.com> > >> > >>I'm not opposed to this change but traditionally QPs are bound to a > >>device not to a single port. > > > >Right, this was done because rdma_protocol_iwarp takes a port number. > > > >I think we discussed this once, the core code doesn't actually support > >different protocols on different ports, so the port_num argument to > >rdma_protocol_iwarp is redundant. > > > >This all starts to look really goofy when multi-port APM is used and > >the QP's port number changes dynamically at runtime. (I have some > >experimental patches that do that), I'd rather see all the port_num > >stuff in this series go away. :( > > HCH and I complained about this per-port distinction in several private > conversations. I'd really love to see it go away too. I'm in support of eliminating them. One protocol per device. IB APM hard requires those semantics, and that reflects the reality of all the drivers today. Nothing more is required than sending a patch, IHMO.. Jason -- 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
> On Tue, Apr 19, 2016 at 09:49:03PM +0300, Sagi Grimberg wrote: > > > > >>>The new RW API will need this. > > >>> > > >>>Signed-off-by: Christoph Hellwig <hch@lst.de> > > >>>Reviewed-by: Bart Van Assche <bart.vanassche@sandisk.com> > > >>>Tested-by: Steve Wise <swise@opengridcomputing.com> > > >> > > >>I'm not opposed to this change but traditionally QPs are bound to a > > >>device not to a single port. > > > > > >Right, this was done because rdma_protocol_iwarp takes a port number. > > > > > >I think we discussed this once, the core code doesn't actually support > > >different protocols on different ports, so the port_num argument to > > >rdma_protocol_iwarp is redundant. > > > > > >This all starts to look really goofy when multi-port APM is used and > > >the QP's port number changes dynamically at runtime. (I have some > > >experimental patches that do that), I'd rather see all the port_num > > >stuff in this series go away. :( > > > > HCH and I complained about this per-port distinction in several private > > conversations. I'd really love to see it go away too. > > I'm in support of eliminating them. One protocol per device. > Ditto. > IB APM hard requires those semantics, and that reflects the reality of > all the drivers today. > > Nothing more is required than sending a patch, IHMO.. > I've been trying to sift through the original threads regarding rdma_protocol_iwarp() and friends. I couldn't find anybody advocating hard that the protocol/transport type should be per port. I think this thread has Doug stating it really should be per-device and static. Doug, correct me if I'm wrong... https://lkml.org/lkml/2015/4/10/612 Steve. -- 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
I can offer a trade: once this series is accepted I'll clean up all the port_num arguments in the protocol checks over the whole tree :) -- 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
On Tue, Apr 19, 2016 at 10:05:55PM +0200, 'Christoph Hellwig' wrote: > I can offer a trade: once this series is accepted I'll clean > up all the port_num arguments in the protocol checks over the whole > tree :) Yeah, this series has spun enough, it should land! :) Jason -- 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
> -----Original Message----- > From: 'Christoph Hellwig' [mailto:hch@lst.de] > Sent: Tuesday, April 19, 2016 3:06 PM > To: Steve Wise > Cc: 'Jason Gunthorpe'; 'Sagi Grimberg'; 'Ira Weiny'; 'Christoph Hellwig'; > dledford@redhat.com; bart.vanassche@sandisk.com; linux-rdma@vger.kernel.org; > target-devel@vger.kernel.org > Subject: Re: [PATCH 02/12] IB/cma: pass the port number to ib_create_qp > > I can offer a trade: once this series is accepted I'll clean > up all the port_num arguments in the protocol checks over the whole > tree :) I will help as needed. -- 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
On Tue, Apr 19, 2016 at 03:26:37PM -0500, Steve Wise wrote: > > > > -----Original Message----- > > From: 'Christoph Hellwig' [mailto:hch@lst.de] > > Sent: Tuesday, April 19, 2016 3:06 PM > > To: Steve Wise > > Cc: 'Jason Gunthorpe'; 'Sagi Grimberg'; 'Ira Weiny'; 'Christoph Hellwig'; > > dledford@redhat.com; bart.vanassche@sandisk.com; linux-rdma@vger.kernel.org; > > target-devel@vger.kernel.org > > Subject: Re: [PATCH 02/12] IB/cma: pass the port number to ib_create_qp > > > > I can offer a trade: once this series is accepted I'll clean > > up all the port_num arguments in the protocol checks over the whole > > tree :) > > I will help as needed. > Agreed. -- 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
> > HCH and I complained about this per-port distinction in several private > > conversations. I'd really love to see it go away too. > > I'm in support of eliminating them. One protocol per device. I'm slow reading this thread, but there are devices today (e.g. qlogic) that support multiple protocols (e.g. iwarp, roce, rocev2). Even the qib and opa drivers do, if you include psm as a separate protocol from ib. -- 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
On Thu, Apr 28, 2016 at 07:43:59PM +0000, Hefty, Sean wrote: > > > HCH and I complained about this per-port distinction in several private > > > conversations. I'd really love to see it go away too. > > > > I'm in support of eliminating them. One protocol per device. > > I'm slow reading this thread, but there are devices today > (e.g. qlogic) that support multiple protocols (e.g. iwarp, roce, > rocev2). Even the qib and opa drivers do, if you include psm as a > separate protocol from ib. I see several litmus tests for what kinds of ports can be combined into a device (eg the 'protocol'): 1) Various cap tests are the same on every port. Particularly the iWarp special behaviours we are talking about here. 2) AHs are not port-specific, so the AH addressing format must be defined by the device. Thus IB and iWarp cannot be combined. 3) Verbs APM must work across ports. So eg rocee and IB cannot be combined since they use a different CM process. Multi-port really only exists to support APM, if APM doesn't work then drivers don't need to create multi-port devices. Jason -- 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
On 04/19/2016 02:38 PM, Christoph Hellwig wrote: > On Tue, Apr 19, 2016 at 11:30:32AM -0600, Jason Gunthorpe wrote: >> Right, this was done because rdma_protocol_iwarp takes a port number. >> >> I think we discussed this once, the core code doesn't actually support >> different protocols on different ports, so the port_num argument to >> rdma_protocol_iwarp is redundant. >> >> This all starts to look really goofy when multi-port APM is used and >> the QP's port number changes dynamically at runtime. (I have some >> experimental patches that do that), I'd rather see all the port_num >> stuff in this series go away. :( > > Reall, I would _love_ to kill all that port_num crap. But until > we get agreement from Doug and all the core maintainers that we > can kill it from the core, and that multi-protocol devices are > indeed as silly as they seem I can't.. > No worries, the patchset (and the subsequent series you promised a few emails later in this thread) can proceed ;-)
> I see several litmus tests for what kinds of ports can be combined > into a device (eg the 'protocol'): > > 1) Various cap tests are the same on every port. Particularly the > iWarp special behaviours we are talking about here. > 2) AHs are not port-specific, so the AH addressing format must be > defined by the device. Thus IB and iWarp cannot be combined. > 3) Verbs APM must work across ports. So eg rocee and IB cannot be > combined since they use a different CM process. > > Multi-port really only exists to support APM, if APM doesn't work then > drivers don't need to create multi-port devices. I don't know the details of the qlogic device, but it is entirely possible that it allows different protocols to share resources (PDs, CQs, IP addresses, etc.). I think we need to be careful dismissing multi-protocol devices as silly, or restricting which protocols can run over which port. Restricting all ports on a device to support all protocols is different than restricting a device to supporting a single protocol, and it affects more than APM. - Sean -- 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
On Thu, Apr 28, 2016 at 09:53:52PM +0000, Hefty, Sean wrote: > > I see several litmus tests for what kinds of ports can be combined > > into a device (eg the 'protocol'): > > > > 1) Various cap tests are the same on every port. Particularly the > > iWarp special behaviours we are talking about here. > > 2) AHs are not port-specific, so the AH addressing format must be > > defined by the device. Thus IB and iWarp cannot be combined. > > 3) Verbs APM must work across ports. So eg rocee and IB cannot be > > combined since they use a different CM process. > > > > Multi-port really only exists to support APM, if APM doesn't work then > > drivers don't need to create multi-port devices. > > I don't know the details of the qlogic device, but it is entirely > possible that it allows different protocols to share resources (PDs, > CQs, IP addresses, etc.). I think we need to be careful dismissing > multi-protocol devices as silly, or restricting which protocols can > run over which port. This isn't dismissing them as silly, it is a pragmatic need in the core code that everything associated with a PD have a minimum standard of uniformity - and it is very clear that includes things like the iwarp special cases and the particular format of the AHs. For instance, even if a hardware device can run rocee and iwarp concurrently over a single port, today we absolutely must have different struct ib_devices for the same physical port to be able to plug that into the core stack. Fundamentally we have the wrong model for such hardware. When a PD is created it should set the 'protocol' and select the compatible member ports that belong to the PD. Cap tests and so forth should be done against the PD, not a port or a device. Fixing that is major surgery, and having cap tests to the port is not helping clarify the current situation. > Restricting all ports on a device to support all protocols is > different than restricting a device to supporting a single protocol, > and it affects more than APM. What else is there that is cross port in verbs? Jason -- 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
> This isn't dismissing them as silly, it is a pragmatic need in the > core code that everything associated with a PD have a minimum standard > of uniformity - and it is very clear that includes things like the > iwarp special cases and the particular format of the AHs. I was referring to Christoph's comment "that multi-protocol devices are indeed as silly as they seem". Maybe we're using different meanings for the term 'device'. I'm referring to the physical hardware. > For instance, even if a hardware device can run rocee and iwarp > concurrently over a single port, today we absolutely must have > different struct ib_devices for the same physical port to be able to > plug that into the core stack. > > Fundamentally we have the wrong model for such hardware. When a PD is > created it should set the 'protocol' and select the compatible member > ports that belong to the PD. Cap tests and so forth should be done > against the PD, not a port or a device. I agree that the model is wrong. But this is the first email I've read (and I skip reading a lot) mentioning the PD. My concern is that the discussion mentioned removing multi-protocol support completely, rather than improving it. > Fixing that is major surgery, and having cap tests to the port is not > helping clarify the current situation. > > > Restricting all ports on a device to support all protocols is > > different than restricting a device to supporting a single protocol, > > and it affects more than APM. > > What else is there that is cross port in verbs? I was referring to the sharing of resources (e.g. CQs, MRs) across different protocols on the same device. -- 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
> > On Thu, Apr 28, 2016 at 09:53:52PM +0000, Hefty, Sean wrote: > > > I see several litmus tests for what kinds of ports can be combined > > > into a device (eg the 'protocol'): > > > > > > 1) Various cap tests are the same on every port. Particularly the > > > iWarp special behaviours we are talking about here. > > > 2) AHs are not port-specific, so the AH addressing format must be > > > defined by the device. Thus IB and iWarp cannot be combined. > > > 3) Verbs APM must work across ports. So eg rocee and IB cannot be > > > combined since they use a different CM process. > > > > > > Multi-port really only exists to support APM, if APM doesn't work > > > then drivers don't need to create multi-port devices. > > > > I don't know the details of the qlogic device, but it is entirely > > possible that it allows different protocols to share resources (PDs, > > CQs, IP addresses, etc.). I think we need to be careful dismissing > > multi-protocol devices as silly, or restricting which protocols can > > run over which port. > > This isn't dismissing them as silly, it is a pragmatic need in the core code that > everything associated with a PD have a minimum standard of uniformity - > and it is very clear that includes things like the iwarp special cases and the > particular format of the AHs. > > For instance, even if a hardware device can run rocee and iwarp concurrently > over a single port, today we absolutely must have different struct ib_devices > for the same physical port to be able to plug that into the core stack. > > Fundamentally we have the wrong model for such hardware. When a PD is > created it should set the 'protocol' and select the compatible member ports > that belong to the PD. Cap tests and so forth should be done against the PD, > not a port or a device. > > Fixing that is major surgery, and having cap tests to the port is not helping > clarify the current situation. > > > Restricting all ports on a device to support all protocols is > > different than restricting a device to supporting a single protocol, > > and it affects more than APM. > > What else is there that is cross port in verbs? > Well the statement said nothing about verbs. It said <quote> But until we get agreement from Doug and all the core maintainers that we can kill > it from the core, and that multi-protocol devices are indeed as silly > as they seem </quote> What Sean is again pointing out is that there are devices which support multiple protocols even on the same port. What you say is true for _verbs_ QPs and _verbs_ PDs but not everything is a QP. Also part of the history is that when these immutable capability flags were added we recognized that some devices would be supporting Ethernet (RoCE) on 1 port and IB on the other. I do agree wih you that this is probably better modeled as 2 devices each with a single port. Mellanox how hard will it be to change your drivers to that model? I'm not even sure how the detection of Link Layer works any more. Back in the day it took a config file and was done when the module loaded. But I thought support for autodetection was in the works. Is the driver capable of that now? If so I see a number of issues here with users changing 1 of 2 IB ports from an IB switch to an Ethernet switch and having autodetection have to tear down a single port and create a device with the other. What happens with APM then? :-/ But all that is different from the qib/hfi case where we have 1 port with 2 protocols on it. If we are going to add PSM into the core then I think it is _semantically_ appropriate for users to be able to query for the protocols supported on a port and get back more than 1. I did not oppose the change Christoph suggested but that was before we started talking about adding in PSM... Ira -- 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
On Thu, Apr 28, 2016 at 11:23:33PM +0000, Hefty, Sean wrote: > > This isn't dismissing them as silly, it is a pragmatic need in the > > core code that everything associated with a PD have a minimum standard > > of uniformity - and it is very clear that includes things like the > > iwarp special cases and the particular format of the AHs. > > I was referring to Christoph's comment "that multi-protocol devices > are indeed as silly as they seem". Maybe we're using different > meanings for the term 'device'. I'm referring to the physical > hardware. Oh. I think the rest of this thread uses device to refer to a 'struct ib_device'. > > Fundamentally we have the wrong model for such hardware. When a PD is > > created it should set the 'protocol' and select the compatible member > > ports that belong to the PD. Cap tests and so forth should be done > > against the PD, not a port or a device. > > I agree that the model is wrong. But this is the first email I've > read (and I skip reading a lot) mentioning the PD. The last time this came up we talked about the right place to apply the 'cap' tests and the idea of using the PD or QP was briefly discussed. > My concern is that the discussion mentioned removing multi-protocol > support completely, rather than improving it. No, the topic is to remove the port num from the cap tests. This is clarifying the current capability of the core code, which is that a struct ib_device must have certain uniformity across all ports. The port_num is totally wrong headed and is not the way to support some future multi-protocol hardware within a single struct ib_device. > I was referring to the sharing of resources (e.g. CQs, MRs) across > different protocols on the same device. Hum, can those even realistically be shared? Eg an iWarp and IB MRs have very different semantics, both in terms of the key and how the permission model works. IIRC CQs had some subtle differences too, and of course extracting an address from a WC is very different. This is why I bring up the PD as a 'narrower' container for the required uniformity. Jason -- 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
On Thu, Apr 28, 2016 at 11:25:35PM +0000, Weiny, Ira wrote: > Mellanox how hard will it be to change your drivers to that model? > I'm not even sure how the detection of Link Layer works any more. Hmm, I vaugely remember looking into this and thinking the mlx drivers already did this? It may even be that IB and rocee can do APM and could perhaps be part of the same struct ib_device without breaking the world. I actually have no idea. > But all that is different from the qib/hfi case where we have 1 port > with 2 protocols on it. If we are going to add PSM into the core > then I think it is _semantically_ appropriate for users to be able > to query for the protocols supported on a port and get back more > than 1. That doesn't make sense, the issue here is that we have a variety of verbs 'flavours'. PSM is not a verbs flavour. *If* PSM gets a kAPI (nobody is talking about doing this?) *and* gains multiple incompatible flavours (such as OPA and IB?) then it will need a unique set of cap tests and restrictions on which physical ports can be used together. Jason -- 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
diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c index 93ab0ae..6ebaf20 100644 --- a/drivers/infiniband/core/cma.c +++ b/drivers/infiniband/core/cma.c @@ -800,6 +800,7 @@ int rdma_create_qp(struct rdma_cm_id *id, struct ib_pd *pd, if (id->device != pd->device) return -EINVAL; + qp_init_attr->port_num = id->port_num; qp = ib_create_qp(pd, qp_init_attr); if (IS_ERR(qp)) return PTR_ERR(qp);