* Re: [PATCH 02/10] iw_cxgb4: Add driver, fw, and hw headers.
From: Steve Wise @ 2010-04-07 22:32 UTC (permalink / raw)
To: Roland Dreier; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <adahbnn0wxl.fsf-BjVyx320WGW9gfZ95n9DRSW4+XlvGpQz@public.gmane.org>
Roland Dreier wrote:
> The intention is that the device just treats wrid as opaque I assume so
> I think the correct fix is to go from __be16 to u16 in the structure
> declarations.
>
>
Shouldn't it be __u16? These structs are part of the firmware to host
driver/lib API.
--
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
^ permalink raw reply
* RE: librdmacm meets libiwarp
From: Sean Hefty @ 2010-04-07 22:26 UTC (permalink / raw)
To: philip.frey-gy3b+zu4XSAfv37vnLkPlQ
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, Bernard Metzler
In-Reply-To: <942482910-1270678218-cardhu_decombobulator_blackberry.rim.net-106991531--vREwkvsugevjL2gL5RxOE1pMnUE8n+ufF6Q+nnUHFY8@public.gmane.org>
>it's nice to see, that you seem to have liked most of
>my ideas and intentions of libiwarp :) thanks for
>making that work a bit more sustainable!
Thanks for the input. I think there could still be a little more work done to
handle completions across shared CQs, plus add in SRQ support.
>I was wondering where I could get a copy of your
>latest code to look at it as a whole and (maybe)
>comment on it.
The code is available from my git tree, in the af_ib branch:
git://git.openfabrics.org/~shefty/librdmacm.git af_ib
- Sean
--
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
^ permalink raw reply
* Re: [PATCH 26/37] librdmacm: set src_addr in rdma_getaddrinfo
From: Jason Gunthorpe @ 2010-04-07 22:22 UTC (permalink / raw)
To: Sean Hefty; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <569EDABAE9F84A029ED7AC84AA5767D6-Zpru7NauK7drdx17CPfAsdBPR1lH4CV8@public.gmane.org>
On Wed, Apr 07, 2010 at 03:10:36PM -0700, Sean Hefty wrote:
> >Not sure what you mean that ACM requires it? Doesn't ACM plug in at
> >the rdma_getaddrinfo stage? If so it can get the source on its own
> >like you did in this patch. I agree that ACM should always return
> >results with the source set, because it is providing path records
> >relative to a specific adaptor.
>
> Yes - the code to set the source could move from librdmacm into ACM.
>
> I can change rdma_getaddrinfo to only set the source address if
> either the user provides one through a hint, or if resolved through
> ACM.
That would be my preference. I think the kernel calls should use a
null source address in the common case and a set source should be an
exceptional case. This matches sockets very well.
I'd see two cases for setting a source address, an app that wants to
control the bind port - this is similar to socket cases, and is
generally an exceptional case.
The other is that an app wants the connection to be usable with a
certain PD. This is more like the DAPL case, as far as I understand it
(ie resources have been allocated against a PD prior to the addresses
being known). This would be best served by having the hints include a
PD and have rdma_getaddrinfo generate a source address that works with
that PD. A PD is more general than a source address - in single HCA
cases a PD will be usable with all ports.
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
^ permalink raw reply
* librdmacm meets libiwarp
From: philip.frey-gy3b+zu4XSAfv37vnLkPlQ @ 2010-04-07 22:11 UTC (permalink / raw)
To: Sean Hefty; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, Bernard Metzler
Hi Sean,
it's nice to see, that you seem to have liked most of
my ideas and intentions of libiwarp :) thanks for
making that work a bit more sustainable!
I was wondering where I could get a copy of your
latest code to look at it as a whole and (maybe)
comment on it.
Thanks,
Philip
--
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
^ permalink raw reply
* RE: [PATCH 26/37] librdmacm: set src_addr in rdma_getaddrinfo
From: Sean Hefty @ 2010-04-07 22:10 UTC (permalink / raw)
To: 'Jason Gunthorpe'; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20100407201417.GJ15629-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
>I don't have a problem with ai_src_addr being set, when necessary, but
>setting it unconditionally seems wrong to me. In most cases the kernel
>should select the source during route resolution, not be forced to
>something in userspace.
Just to be precise, the source is selected during address resolution, and the
existing APIs allow the user to indicate that a specific source should be used.
This is a requirement of some applications.
>Not sure what you mean that ACM requires it? Doesn't ACM plug in at
>the rdma_getaddrinfo stage? If so it can get the source on its own
>like you did in this patch. I agree that ACM should always return
>results with the source set, because it is providing path records
>relative to a specific adaptor.
Yes - the code to set the source could move from librdmacm into ACM.
I can change rdma_getaddrinfo to only set the source address if either the user
provides one through a hint, or if resolved through ACM.
- Sean
--
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
^ permalink raw reply
* Re: [PATCH 31/37] librdmacm: provide abstracted verb calls
From: Steve Wise @ 2010-04-07 22:07 UTC (permalink / raw)
To: Sean Hefty; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <3F9969F0569A431A9B40B8DED7E18FB4-Zpru7NauK7drdx17CPfAsdBPR1lH4CV8@public.gmane.org>
Sean Hefty wrote:
>>> I think it's possible for the function to return without having filled in a
>>>
>> wc.
>>
>> So its busted? Or is this intended behavior?
>>
>
> Depends on the point of view, I guess. :) It would be nice to avoid that
> situation.
>
>
>> Is rdma_get_send_completion() supposed to return exactly one wc? If so
>> then the 2 polls can cause a wc to get silently discarded. I must
>> still not be understanding the intended use?
>>
>
> How can a wc get discarded? Maybe the return code from ibv_poll_cq is confusing
> you? If the first poll finds a wc, ibv_poll_cq returns 1, and we exit the
> function. Otherwise, we rearm the cq, then poll again to make sure that nothing
> got missed.
>
>
Right. I missed that. poll will return 1 if there's a completion
returned. Nevermind :)
>> I would think this should just be:
>>
>> get_cq_event()
>> notify_cq()
>> poll()
>>
>
> This requires arming the CQ up front. I was also trying to avoid the overhead
> of always calling get_cq_event and notify_cq to just pull a completed request
> off of the work queue.
>
>
I was confused on the poll_cq return code (and I've been working in this
code for umpteen years :) ).
>>> The only drawback I see is that it's theoretically possible to build up a
>>>
>> queue
>>
>>> of cq events in the kernel. Not sure how to fix that. Any ideas?
>>>
>>>
>> That can always happen, yes?
>>
>
> It seems like it should be avoidable. Maybe 1 event can queue up, but I think
> we can prevent more by not rearming until that event gets pulled.
>
> If nothing else, I think this discussion shows why we need this sort of wrapper.
> :)
>
Indeed! I like the wrappers.
--
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
^ permalink raw reply
* RE: [PATCH 31/37] librdmacm: provide abstracted verb calls
From: Sean Hefty @ 2010-04-07 22:01 UTC (permalink / raw)
To: 'Steve Wise'; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <4BBCED37.9040708-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>
>> I think it's possible for the function to return without having filled in a
>wc.
>>
>
>So its busted? Or is this intended behavior?
Depends on the point of view, I guess. :) It would be nice to avoid that
situation.
>Is rdma_get_send_completion() supposed to return exactly one wc? If so
>then the 2 polls can cause a wc to get silently discarded. I must
>still not be understanding the intended use?
How can a wc get discarded? Maybe the return code from ibv_poll_cq is confusing
you? If the first poll finds a wc, ibv_poll_cq returns 1, and we exit the
function. Otherwise, we rearm the cq, then poll again to make sure that nothing
got missed.
>I would think this should just be:
>
>get_cq_event()
>notify_cq()
>poll()
This requires arming the CQ up front. I was also trying to avoid the overhead
of always calling get_cq_event and notify_cq to just pull a completed request
off of the work queue.
>> The only drawback I see is that it's theoretically possible to build up a
>queue
>> of cq events in the kernel. Not sure how to fix that. Any ideas?
>>
>That can always happen, yes?
It seems like it should be avoidable. Maybe 1 event can queue up, but I think
we can prevent more by not rearming until that event gets pulled.
If nothing else, I think this discussion shows why we need this sort of wrapper.
:)
--
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
^ permalink raw reply
* Re: [PATCH 05/10] iw_cxgb4: Add connection management functions.
From: Roland Dreier @ 2010-04-07 21:55 UTC (permalink / raw)
To: Steve Wise; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <4BBCFDF9.70306-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>
> Both iw_cxgb3 and iw_cxgb4 load ok concurrently when compiled as modules.
Oh, right. The peer2peer symbol isn't exported so the clash is only if
you try to build them both into the kernel (as I often do as part of my
quick build tests).
--
Roland Dreier <rolandd-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.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
^ permalink raw reply
* Re: [PATCH 02/10] iw_cxgb4: Add driver, fw, and hw headers.
From: Steve Wise @ 2010-04-07 21:50 UTC (permalink / raw)
To: Roland Dreier; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <adahbnn0wxl.fsf-BjVyx320WGW9gfZ95n9DRSW4+XlvGpQz@public.gmane.org>
Roland Dreier wrote:
> You have:
>
> > +struct fw_ri_send_wr {
> ...
> > + __be16 wrid;
>
> > +struct fw_ri_recv_wr {
> ...
> > + __be16 wrid;
>
> But also:
>
> > +static inline void init_wr_hdr(union t4_wr *wqe, u16 wrid,
> > + enum fw_wr_opcodes opcode, u8 flags, u8 len16)
> ...
> > + wqe->send.wrid = wrid;
>
> and similar for recv.wrid in qp.c. sparse correctly warns about this
> endianness clash.
>
> The intention is that the device just treats wrid as opaque I assume so
> I think the correct fix is to go from __be16 to u16 in the structure
> declarations.
>
> - R.
>
Yes, it should be a u16 in the wr structs.
--
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
^ permalink raw reply
* Re: [PATCH 05/10] iw_cxgb4: Add connection management functions.
From: Steve Wise @ 2010-04-07 21:49 UTC (permalink / raw)
To: Roland Dreier; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <adaljcz0x0g.fsf-BjVyx320WGW9gfZ95n9DRSW4+XlvGpQz@public.gmane.org>
Roland Dreier wrote:
> > +int peer2peer = 0;
> > +module_param(peer2peer, int, 0644);
> > +MODULE_PARM_DESC(peer2peer, "Support peer2peer ULPs (default=0)");
>
> If you build iw_cxgb3 and iw_cxgb4 into the kernel, the peer2peer symbol
> names clash. (Same problem occurs if you try to load cxgb3 and cxgb4
> modules at the same time, I think).
Both iw_cxgb3 and iw_cxgb4 load ok concurrently when compiled as modules.
The option was originally intended to be used in more than just cm.c.
So there's a piece of code missing in qp.c. I'll clean this up. I
might make an attribute in c4iw_endpoint that indicates this mode. Then
the qp code won't need the global option and can key off the endpoint
attribute. So I can make this a static as you suggest.
> We can fix it here in cxgb4 by just
> making peer2peer static (and deleting the extern declaration).
>
> However peer2peer is not that great of a name for a global symbol; might
> be good to add a patch to cxgb3 to rename peer2peer to something like
> iwch_peer2peer and using module_param_named()...
>
I'll do this for cxgb3.
Steve.
--
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
^ permalink raw reply
* Re: [PATCH 06/10] iw_cxgb4: Add memory management functions.
From: Roland Dreier @ 2010-04-07 21:40 UTC (permalink / raw)
To: Steve Wise; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20100402192419.14117.40759.stgit-T4OLL4TyM9aNDNWfRnPdfg@public.gmane.org>
> + req->wr.wr_lo = (u64)&wr_wait;
wr_lo is __be64. The cast should be to __force __be64 here I think.
--
Roland Dreier <rolandd-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.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
^ permalink raw reply
* Re: [PATCH 05/10] iw_cxgb4: Add connection management functions.
From: Roland Dreier @ 2010-04-07 21:37 UTC (permalink / raw)
To: Steve Wise; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20100402192413.14117.56138.stgit-T4OLL4TyM9aNDNWfRnPdfg@public.gmane.org>
> + wr_waitp = (struct c4iw_wr_wait *)rpl->data[1];
Sparse complains about this case from __be64 to a pointer. I assume
this is OK but you probably want to stick a __force in there to annotate it.
--
Roland Dreier <rolandd-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.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
^ permalink raw reply
* Re: [PATCH 05/10] iw_cxgb4: Add connection management functions.
From: Roland Dreier @ 2010-04-07 21:36 UTC (permalink / raw)
To: Steve Wise; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20100402192413.14117.56138.stgit-T4OLL4TyM9aNDNWfRnPdfg@public.gmane.org>
> +void _c4iw_free_ep(struct kref *kref)
...
> + ep = container_of(container_of(kref, struct c4iw_ep_common, kref),
> + struct c4iw_ep, com);
sparse warns of some internal container_of variable shadowing itself
here. You can avoid that and write this more simply as:
ep = container_of(kref, struct c4iw_ep, com.kref);
--
Roland Dreier <rolandd-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.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
^ permalink raw reply
* Re: [PATCH 02/10] iw_cxgb4: Add driver, fw, and hw headers.
From: Roland Dreier @ 2010-04-07 21:33 UTC (permalink / raw)
To: Steve Wise; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20100402192357.14117.39150.stgit-T4OLL4TyM9aNDNWfRnPdfg@public.gmane.org>
You have:
> +struct fw_ri_send_wr {
...
> + __be16 wrid;
> +struct fw_ri_recv_wr {
...
> + __be16 wrid;
But also:
> +static inline void init_wr_hdr(union t4_wr *wqe, u16 wrid,
> + enum fw_wr_opcodes opcode, u8 flags, u8 len16)
...
> + wqe->send.wrid = wrid;
and similar for recv.wrid in qp.c. sparse correctly warns about this
endianness clash.
The intention is that the device just treats wrid as opaque I assume so
I think the correct fix is to go from __be16 to u16 in the structure
declarations.
- R.
--
Roland Dreier <rolandd-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.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
^ permalink raw reply
* Re: [PATCH 05/10] iw_cxgb4: Add connection management functions.
From: Roland Dreier @ 2010-04-07 21:32 UTC (permalink / raw)
To: Steve Wise; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20100402192413.14117.56138.stgit-T4OLL4TyM9aNDNWfRnPdfg@public.gmane.org>
> +int peer2peer = 0;
> +module_param(peer2peer, int, 0644);
> +MODULE_PARM_DESC(peer2peer, "Support peer2peer ULPs (default=0)");
If you build iw_cxgb3 and iw_cxgb4 into the kernel, the peer2peer symbol
names clash. (Same problem occurs if you try to load cxgb3 and cxgb4
modules at the same time, I think). We can fix it here in cxgb4 by just
making peer2peer static (and deleting the extern declaration).
However peer2peer is not that great of a name for a global symbol; might
be good to add a patch to cxgb3 to rename peer2peer to something like
iwch_peer2peer and using module_param_named()...
--
Roland Dreier <rolandd-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.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
^ permalink raw reply
* Re: [patch] infiniband: checking the wrong variable
From: Roland Dreier @ 2010-04-07 21:18 UTC (permalink / raw)
To: Dan Carpenter
Cc: Roland Dreier, Sean Hefty, Hal Rosenstock, Tejun Heo,
Christoph Lameter, Jack Morgenstein,
linux-rdma-u79uwXL29TY76Z2rM5mHXA,
kernel-janitors-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20100407093901.GG5157@bicker>
thanks, applied.
--
Roland Dreier <rolandd-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.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
^ permalink raw reply
* Re: [PATCH] RDMA/nes: correct cap.max_inline_data assignment in nes_query_qp
From: Roland Dreier @ 2010-04-07 21:16 UTC (permalink / raw)
To: Chien Tung; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20100325133950.GA9752@ctung-MOBL>
thanks, applied
--
Roland Dreier <rolandd-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.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
^ permalink raw reply
* Re: [PATCH] [for-2.6.34] rdma/cm: set num_paths when manually assigning path records
From: Roland Dreier @ 2010-04-07 21:14 UTC (permalink / raw)
To: Sean Hefty; +Cc: linux-rdma
In-Reply-To: <3946EFA63D2D4C2A93F45CF7CD715B5C-Zpru7NauK7drdx17CPfAsdBPR1lH4CV8@public.gmane.org>
thanks, applied
--
Roland Dreier <rolandd-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.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
^ permalink raw reply
* Re: [PATCH 31/37] librdmacm: provide abstracted verb calls
From: Steve Wise @ 2010-04-07 20:38 UTC (permalink / raw)
To: Sean Hefty; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <411E3A9BECF04B598FFEFFF08505DE6F-Zpru7NauK7drdx17CPfAsdBPR1lH4CV8@public.gmane.org>
Sean Hefty wrote:
>>> +static inline int
>>> +rdma_get_send_comp(struct rdma_cm_id *id, struct ibv_wc *wc)
>>> +{
>>> + struct ibv_cq *cq;
>>> + void *context;
>>> + int ret;
>>> +
>>> + ret = ibv_poll_cq(id->send_cq, 1, wc);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + ret = ibv_req_notify_cq(id->send_cq, 0);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + ret = ibv_poll_cq(id->send_cq, 1, wc);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + ret = ibv_get_cq_event(id->send_cq_channel, &cq, &context);
>>> + if (ret)
>>> + return ret;
>>>
>>>
>> This doesn't look correct. If the send isn't complete by the time the
>> 2nd ibv_poll_cq() completes, then this function will return without
>> having filled in the wc. Or am I missing something? Shouldn't the
>> ibv_get_cq_event() be the first thing this function does? The same
>> issue/question exists for rdma_get_recv_comp().
>>
>
> I think it's possible for the function to return without having filled in a wc.
>
So its busted? Or is this intended behavior?
> If the 2nd poll removes a completion, it can leave a cq event on the channel,
> which a subsequent call could retrieve, but then find the cq empty.
>
> The idea for this call is to abstract poll, notify_cq, and get_cq_event, but
> still provide decent performance. (Scalability is a separate matter. I
> couldn't find a decent way to abstract a CQ shared across QPs or between the
> receive and send queues.)
>
> To avoid returning from the call without a completion, I think the following
> structure works:
>
> poll()
> notify_cq()
> poll()
> while (no completion) {
> get_cq_event()
> poll()
> }
>
Is rdma_get_send_completion() supposed to return exactly one wc? If so
then the 2 polls can cause a wc to get silently discarded. I must
still not be understanding the intended use?
I would think this should just be:
get_cq_event()
notify_cq()
poll()
> The only drawback I see is that it's theoretically possible to build up a queue
> of cq events in the kernel. Not sure how to fix that. Any ideas?
>
>
That can always happen, yes?
Steve.
--
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
^ permalink raw reply
* RE: [PATCH 31/37] librdmacm: provide abstracted verb calls
From: Sean Hefty @ 2010-04-07 20:14 UTC (permalink / raw)
To: 'Steve Wise'; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <4BBCDF57.20303-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>
>> +static inline int
>> +rdma_get_send_comp(struct rdma_cm_id *id, struct ibv_wc *wc)
>> +{
>> + struct ibv_cq *cq;
>> + void *context;
>> + int ret;
>> +
>> + ret = ibv_poll_cq(id->send_cq, 1, wc);
>> + if (ret)
>> + return ret;
>> +
>> + ret = ibv_req_notify_cq(id->send_cq, 0);
>> + if (ret)
>> + return ret;
>> +
>> + ret = ibv_poll_cq(id->send_cq, 1, wc);
>> + if (ret)
>> + return ret;
>> +
>> + ret = ibv_get_cq_event(id->send_cq_channel, &cq, &context);
>> + if (ret)
>> + return ret;
>>
>
>This doesn't look correct. If the send isn't complete by the time the
>2nd ibv_poll_cq() completes, then this function will return without
>having filled in the wc. Or am I missing something? Shouldn't the
>ibv_get_cq_event() be the first thing this function does? The same
>issue/question exists for rdma_get_recv_comp().
I think it's possible for the function to return without having filled in a wc.
If the 2nd poll removes a completion, it can leave a cq event on the channel,
which a subsequent call could retrieve, but then find the cq empty.
The idea for this call is to abstract poll, notify_cq, and get_cq_event, but
still provide decent performance. (Scalability is a separate matter. I
couldn't find a decent way to abstract a CQ shared across QPs or between the
receive and send queues.)
To avoid returning from the call without a completion, I think the following
structure works:
poll()
notify_cq()
poll()
while (no completion) {
get_cq_event()
poll()
}
The only drawback I see is that it's theoretically possible to build up a queue
of cq events in the kernel. Not sure how to fix that. Any ideas?
--
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
^ permalink raw reply
* Re: [PATCH 26/37] librdmacm: set src_addr in rdma_getaddrinfo
From: Jason Gunthorpe @ 2010-04-07 20:14 UTC (permalink / raw)
To: Sean Hefty; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <4BAFD82633A744729F9685D44B250100-Zpru7NauK7drdx17CPfAsdBPR1lH4CV8@public.gmane.org>
On Wed, Apr 07, 2010 at 12:54:56PM -0700, Sean Hefty wrote:
> >I haven't looked through everything you posted to make a suggestion
> >here, but this bothers me..
> >
> >The resources should be allocated after the rdma_bind syscall, prior to
> >listen/accept or connect, IMHO.
> >
> >How does tha rai->ai_src_addr get used to allocate resources anyhow?
>
> Maybe the patch description is off.
>
> All this does (in a very non-sexy way) is set ai_src_addr. It does
> not allocate any hardware resources. A user can provide ai_src_addr
> as input into rdma_bind or rdma_resolve_addr.
>
> The motivation is twofold. First, the user can select the
> rdma_addrinfo for a connection by examining the src/dst address
> pair. This may be desired for failover or performance reasons.
> Second, route resolution may require knowing both the source and
> destination addresses. For example, IB ACM requires both addresses
> as input.
Huumm
I don't have a problem with ai_src_addr being set, when necessary, but
setting it unconditionally seems wrong to me. In most cases the kernel
should select the source during route resolution, not be forced to
something in userspace.
Certainly for AF_INET/6 I don't think this should be done..
Apps doing complex things for failover should supply a source address
in the hints and call rdma_getaddrinfo for each adaptor.
AF_IB has the scope ID in the destination to specify the adaptor for
link-local GIDs, so the source should not often be needed.
Not sure what you mean that ACM requires it? Doesn't ACM plug in at
the rdma_getaddrinfo stage? If so it can get the source on its own
like you did in this patch. I agree that ACM should always return
results with the source set, because it is providing path records
relative to a specific adaptor.
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
^ permalink raw reply
* Re: Ummunotify: progress at last!
From: Jason Gunthorpe @ 2010-04-07 20:05 UTC (permalink / raw)
To: Roland Dreier; +Cc: Jeff Squyres, Linux RDMA List, Brad Benton
In-Reply-To: <ada39z72gww.fsf-BjVyx320WGW9gfZ95n9DRSW4+XlvGpQz@public.gmane.org>
On Wed, Apr 07, 2010 at 12:37:03PM -0700, Roland Dreier wrote:
> > No, there is no mmap. Like this:
> >
> > u64 my_counter = 0;
> >
> > ibv_set_mmu_counter(verbs, &my_counter);
> > [..]
> > while (my_counter != last_my_counter) {
> > last_my_counter = my_counter;
> > ibv_get_mmu_notifications(verbs, ...); // <- I am a memory barrier as well
> > }
> >
> > The kernel 'syscall' ibv_set_mmu_counter would bind the given verbs to
> > the 8 byte counter you specified without having to the mmap thing. As
> > I understand it this is what perfevents does.
>
> I was trying to look at how perf events handles this, and AFAICT it
> looks like kernel/perf_event.c just supports mmap(). Can you expand on
> what you meant here?
>
> (I was trying to figure out how one would handle the case where
> userspace gives us a counter in highmem -- doing kmap_atomic() seems to
> be to only option but then I'm not sure if I want to deal with that...)
I think I was mistaken here, disregard..
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
^ permalink raw reply
* RE: [PATCH 26/37] librdmacm: set src_addr in rdma_getaddrinfo
From: Sean Hefty @ 2010-04-07 19:54 UTC (permalink / raw)
To: 'Jason Gunthorpe'; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20100407192810.GG15629-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
>I haven't looked through everything you posted to make a suggestion
>here, but this bothers me..
>
>The resources should be allocated after the rdma_bind syscall, prior to
>listen/accept or connect, IMHO.
>
>How does tha rai->ai_src_addr get used to allocate resources anyhow?
Maybe the patch description is off.
All this does (in a very non-sexy way) is set ai_src_addr. It does not allocate
any hardware resources. A user can provide ai_src_addr as input into rdma_bind
or rdma_resolve_addr.
The motivation is twofold. First, the user can select the rdma_addrinfo for a
connection by examining the src/dst address pair. This may be desired for
failover or performance reasons. Second, route resolution may require knowing
both the source and destination addresses. For example, IB ACM requires both
addresses as input.
- Sean
--
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
^ permalink raw reply
* RE: [PATCH 22/37] librdmacm: add new call to create id
From: Hefty, Sean @ 2010-04-07 19:42 UTC (permalink / raw)
To: Jason Gunthorpe; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <20100407193402.GH15629-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
>> + * The rdma_cm_id will be set to use synchronous operations (connect,
>> + * listen, and get_request). To convert to synchronous operation, the
> ^^^^^^^^^
>asynchronous?
yes - thanks
--
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
^ permalink raw reply
* Re: [PATCH 31/37] librdmacm: provide abstracted verb calls
From: Steve Wise @ 2010-04-07 19:39 UTC (permalink / raw)
To: Sean Hefty; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <27A4CFF94530491F88C8E528428FF93E-Zpru7NauK7drdx17CPfAsdBPR1lH4CV8@public.gmane.org>
Sean Hefty wrote:
> Provide abstractions to the verb calls to simplify the user
> interface for more casual verbs consumers. Users still have
> access to the full range of verbs functionality by calling
> verbs directly.
>
> Signed-off-by: Sean Hefty <sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>
> Makefile.am | 5 -
> include/rdma/rdma_verbs.h | 287 +++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 290 insertions(+), 2 deletions(-)
>
> diff --git a/Makefile.am b/Makefile.am
> index 8d86045..8aef24a 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -31,7 +31,8 @@ librdmacmincludedir = $(includedir)/rdma $(includedir)/infiniband
>
> librdmacminclude_HEADERS = include/rdma/rdma_cma_abi.h \
> include/rdma/rdma_cma.h \
> - include/infiniband/ib.h
> + include/infiniband/ib.h \
> + include/rdma/rdma_verbs.h
>
> man_MANS = \
> man/rdma_accept.3 \
> @@ -69,7 +70,7 @@ man_MANS = \
> man/rdma_cm.7
>
> EXTRA_DIST = include/rdma/rdma_cma_abi.h include/rdma/rdma_cma.h \
> - include/infiniband/ib.h \
> + include/infiniband/ib.h include/rdma/rdma_verbs.h \
> src/cma.h src/librdmacm.map librdmacm.spec.in $(man_MANS)
>
> dist-hook: librdmacm.spec
> diff --git a/include/rdma/rdma_verbs.h b/include/rdma/rdma_verbs.h
> new file mode 100644
> index 0000000..05964c1
> --- /dev/null
> +++ b/include/rdma/rdma_verbs.h
> @@ -0,0 +1,287 @@
> +/*
> + * Copyright (c) 2010 Intel Corporation. All rights reserved.
> + *
> + * This software is available to you under a choice of one of two
> + * licenses. You may choose to be licensed under the terms of the GNU
> + * General Public License (GPL) Version 2, available from the file
> + * COPYING in the main directory of this source tree, or the
> + * OpenIB.org BSD license below:
> + *
> + * Redistribution and use in source and binary forms, with or
> + * without modification, are permitted provided that the following
> + * conditions are met:
> + *
> + * - Redistributions of source code must retain the above
> + * copyright notice, this list of conditions and the following
> + * disclaimer.
> + *
> + * - Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following
> + * disclaimer in the documentation and/or other materials
> + * provided with the distribution.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
> + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
> + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
> + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> + * SOFTWARE.
> + */
> +
> +#if !defined(RDMA_VERBS_H)
> +#define RDMA_VERBS_H
> +
> +#include <assert.h>
> +#include <infiniband/verbs.h>
> +#include <rdma/rdma_cma.h>
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +/*
> + * Memory registration helpers.
> + */
> +static inline struct ibv_mr *
> +rdma_reg_msgs(struct rdma_cm_id *id, void *addr, size_t length)
> +{
> + return ibv_reg_mr(id->qp->pd, addr, length, IBV_ACCESS_LOCAL_WRITE);
> +}
> +
> +static inline struct ibv_mr *
> +rdma_reg_read(struct rdma_cm_id *id, void *addr, size_t length)
> +{
> + return ibv_reg_mr(id->qp->pd, addr, length, IBV_ACCESS_LOCAL_WRITE|
> + IBV_ACCESS_REMOTE_READ);
> +}
> +
> +static inline struct ibv_mr *
> +rdma_reg_write(struct rdma_cm_id *id, void *addr, size_t length)
> +{
> + return ibv_reg_mr(id->qp->pd, addr, length, IBV_ACCESS_LOCAL_WRITE |
> + IBV_ACCESS_REMOTE_WRITE);
> +}
> +
> +static inline int
> +rdma_dereg_mr(struct ibv_mr *mr)
> +{
> + return ibv_dereg_mr(mr);
> +}
> +
> +
> +/*
> + * Vectored send, receive, and RDMA operations.
> + * Support multiple scatter-gather entries.
> + */
> +static inline int
> +rdma_post_recvv(struct rdma_cm_id *id, void *context, struct ibv_sge *sgl,
> + int nsge)
> +{
> + struct ibv_recv_wr wr, *bad;
> +
> + wr.wr_id = (uintptr_t) context;
> + wr.next = NULL;
> + wr.sg_list = sgl;
> + wr.num_sge = nsge;
> +
> + return ibv_post_recv(id->qp, &wr, &bad);
> +}
> +
> +static inline int
> +rdma_post_sendv(struct rdma_cm_id *id, void *context, struct ibv_sge *sgl,
> + int nsge, int flags)
> +{
> + struct ibv_send_wr wr, *bad;
> +
> + wr.wr_id = (uintptr_t) context;
> + wr.next = NULL;
> + wr.sg_list = sgl;
> + wr.num_sge = nsge;
> + wr.opcode = IBV_WR_SEND;
> + wr.send_flags = flags;
> +
> + return ibv_post_send(id->qp, &wr, &bad);
> +}
> +
> +static inline int
> +rdma_post_readv(struct rdma_cm_id *id, void *context, struct ibv_sge *sgl,
> + int nsge, int flags, uint64_t remote_addr, uint32_t rkey)
> +{
> + struct ibv_send_wr wr, *bad;
> +
> + wr.wr_id = (uintptr_t) context;
> + wr.next = NULL;
> + wr.sg_list = sgl;
> + wr.num_sge = nsge;
> + wr.opcode = IBV_WR_RDMA_READ;
> + wr.send_flags = flags;
> + wr.wr.rdma.remote_addr = remote_addr;
> + wr.wr.rdma.rkey = rkey;
> +
> + return ibv_post_send(id->qp, &wr, &bad);
> +}
> +
> +static inline int
> +rdma_post_writev(struct rdma_cm_id *id, void *context, struct ibv_sge *sgl,
> + int nsge, int flags, uint64_t remote_addr, uint32_t rkey)
> +{
> + struct ibv_send_wr wr, *bad;
> +
> + wr.wr_id = (uintptr_t) context;
> + wr.next = NULL;
> + wr.sg_list = sgl;
> + wr.num_sge = nsge;
> + wr.opcode = IBV_WR_RDMA_WRITE;
> + wr.send_flags = flags;
> + wr.wr.rdma.remote_addr = remote_addr;
> + wr.wr.rdma.rkey = rkey;
> +
> + return ibv_post_send(id->qp, &wr, &bad);
> +}
> +
> +/*
> + * Simple send, receive, and RDMA calls.
> + */
> +static inline int
> +rdma_post_recv(struct rdma_cm_id *id, void *context, void *addr,
> + size_t length, struct ibv_mr *mr)
> +{
> + struct ibv_sge sge;
> +
> + assert((addr >= mr->addr) && ((addr + length) <= (mr->addr + mr->length)));
> + sge.addr = (uint64_t) addr;
> + sge.length = (uint32_t) length;
> + sge.lkey = mr->lkey;
> +
> + return rdma_post_recvv(id, context, &sge, 1);
> +}
> +
> +static inline int
> +rdma_post_send(struct rdma_cm_id *id, void *context, void *addr,
> + size_t length, struct ibv_mr *mr, int flags)
> +{
> + struct ibv_sge sge;
> +
> + sge.addr = (uint64_t) addr;
> + sge.length = (uint32_t) length;
> + sge.lkey = mr ? mr->lkey : 0;
> +
> + return rdma_post_sendv(id, context, &sge, 1, flags);
> +}
> +
> +static inline int
> +rdma_post_read(struct rdma_cm_id *id, void *context, void *addr,
> + size_t length, struct ibv_mr *mr, int flags,
> + uint64_t remote_addr, uint32_t rkey)
> +{
> + struct ibv_sge sge;
> +
> + sge.addr = (uint64_t) addr;
> + sge.length = (uint32_t) length;
> + sge.lkey = mr->lkey;
> +
> + return rdma_post_readv(id, context, &sge, 1, flags, remote_addr, rkey);
> +}
> +
> +static inline int
> +rdma_post_write(struct rdma_cm_id *id, void *context, void *addr,
> + size_t length, struct ibv_mr *mr, int flags,
> + uint64_t remote_addr, uint32_t rkey)
> +{
> + struct ibv_sge sge;
> +
> + sge.addr = (uint64_t) addr;
> + sge.length = (uint32_t) length;
> + sge.lkey = mr ? mr->lkey : 0;
> +
> + return rdma_post_writev(id, context, &sge, 1, flags, remote_addr, rkey);
> +}
> +
> +static inline int
> +rdma_post_ud_send(struct rdma_cm_id *id, void *context, void *addr,
> + size_t length, struct ibv_mr *mr, int flags,
> + struct ibv_ah *ah, uint32_t remote_qpn)
> +{
> + struct ibv_send_wr wr, *bad;
> + struct ibv_sge sge;
> +
> + sge.addr = (uint64_t) addr;
> + sge.length = (uint32_t) length;
> + sge.lkey = mr ? mr->lkey : 0;
> +
> + wr.wr_id = (uintptr_t) context;
> + wr.next = NULL;
> + wr.sg_list = &sge;
> + wr.num_sge = 1;
> + wr.opcode = IBV_WR_SEND;
> + wr.send_flags = flags;
> + wr.wr.ud.ah = ah;
> + wr.wr.ud.remote_qpn = remote_qpn;
> + wr.wr.ud.remote_qkey = RDMA_UDP_QKEY;
> +
> + return ibv_post_send(id->qp, &wr, &bad);
> +}
> +
> +static inline int
> +rdma_get_send_comp(struct rdma_cm_id *id, struct ibv_wc *wc)
> +{
> + struct ibv_cq *cq;
> + void *context;
> + int ret;
> +
> + ret = ibv_poll_cq(id->send_cq, 1, wc);
> + if (ret)
> + return ret;
> +
> + ret = ibv_req_notify_cq(id->send_cq, 0);
> + if (ret)
> + return ret;
> +
> + ret = ibv_poll_cq(id->send_cq, 1, wc);
> + if (ret)
> + return ret;
> +
> + ret = ibv_get_cq_event(id->send_cq_channel, &cq, &context);
> + if (ret)
> + return ret;
>
This doesn't look correct. If the send isn't complete by the time the
2nd ibv_poll_cq() completes, then this function will return without
having filled in the wc. Or am I missing something? Shouldn't the
ibv_get_cq_event() be the first thing this function does? The same
issue/question exists for rdma_get_recv_comp().
Steve.
--
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
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox