by Liran Liss

[permalink] [raw]

Subject: RE: [PATCH v6 02/26] IB/Verbs: Implement raw management helpers

> From: [email protected] [mailto:linux-rdma-
>
> Add raw helpers:
> rdma_tech_ib
> rdma_tech_iboe
> rdma_tech_iwarp
> rdma_ib_or_iboe (transition, clean up later) To help us detect which
> technology the port supported.
>

Replace "rdma_tech_*" with "rdma_protocol_*".

> Cc: Hal Rosenstock <[email protected]>
> Cc: Steve Wise <[email protected]>
> Cc: Tom Talpey <[email protected]>
> Cc: Jason Gunthorpe <[email protected]>
> Cc: Doug Ledford <[email protected]>
> Cc: Ira Weiny <[email protected]>
> Cc: Sean Hefty <[email protected]>
> Signed-off-by: Michael Wang <[email protected]>
> ---
> include/rdma/ib_verbs.h | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h index
> d54f91e..a12e876 100644
> --- a/include/rdma/ib_verbs.h
> +++ b/include/rdma/ib_verbs.h
> @@ -1748,6 +1748,31 @@ int ib_query_port(struct ib_device *device, enum
> rdma_link_layer rdma_port_get_link_layer(struct ib_device *device,
> u8 port_num);
>
> +static inline int rdma_tech_ib(struct ib_device *device, u8 port_num) {
> + return device->query_transport(device, port_num)
> + == RDMA_TRANSPORT_IB;
> +}
> +
> +static inline int rdma_tech_iboe(struct ib_device *device, u8 port_num)
> +{
> + return device->query_transport(device, port_num)
> + == RDMA_TRANSPORT_IBOE;

Remove RDMA_TRANSPORT_IBOE.
In the current code, the test should be: (IB transport && Ethernet link layer).

We can later consider each provider declaring the transports directly.

> +}
> +
> +static inline int rdma_tech_iwarp(struct ib_device *device, u8
> +port_num) {
> + return device->query_transport(device, port_num)
> + == RDMA_TRANSPORT_IWARP;
> +}
> +
> +static inline int rdma_ib_or_iboe(struct ib_device *device, u8
> +port_num) {
> + enum rdma_transport_type tp = device->query_transport(device,
> +port_num);
> +
> + return (tp == RDMA_TRANSPORT_IB || tp ==
> RDMA_TRANSPORT_IBOE); }

Remove RDMA_TRANSPORT_IBOE.
Just test against RDMA_TRANSPORT_IB.

> +
> int ib_query_gid(struct ib_device *device,
> u8 port_num, int index, union ib_gid *gid);
>

2015-04-24 16:00:39

On Mon, 2015-04-27 at 17:16 -0700, Tom Talpey wrote:
> On 4/27/2015 2:52 PM, ira.weiny wrote:
> > On Mon, Apr 27, 2015 at 09:39:05AM +0200, Michael Wang wrote:
> >>
> >>
> >> On 04/24/2015 05:12 PM, Liran Liss wrote:
> >>>> From: [email protected] [mailto:linux-rdma-
> >>>>
> >>> [snip]
> >>>> a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h index
> >>>> 65994a1..d54f91e 100644
> >>>> --- a/include/rdma/ib_verbs.h
> >>>> +++ b/include/rdma/ib_verbs.h
> >>>> @@ -75,10 +75,13 @@ enum rdma_node_type { };
> >>>>
> >>>> enum rdma_transport_type {
> >>>> + /* legacy for users */
> >>>> RDMA_TRANSPORT_IB,
> >>>> RDMA_TRANSPORT_IWARP,
> >>>> RDMA_TRANSPORT_USNIC,
> >>>> - RDMA_TRANSPORT_USNIC_UDP
> >>>> + RDMA_TRANSPORT_USNIC_UDP,
> >>>> + /* new transport */
> >>>> + RDMA_TRANSPORT_IBOE,
> >>>
> >>> Remove RDMA_TRANSPORT_IBOE - it is not a transport.
> >>> ROCE uses IBTA transport.
> >>>
> >>> If any code should test for ROCE should invoke a specific helper, e.g., rdma_protocol_iboe().
> >>> This is what you currently call "rdma_tech_iboe" is patch 02/26.
> >>>
> >>> I think that pretty much everybody agrees that rdma_protocol_*() is a better name than rdma_tech_*(), right?
> >>> So, let's change this.
> >>
> >> Sure, sounds reasonable now, about the IBOE, we still need it to
> >> separate the port support IB/ETH without the check on link-layer,
> >> So what about a new enum on protocol type?
> >>
> >> Like:
> >>
> >> enum rdma_protocol {
> >> RDMA_PROTOCOL_IB,
> >> RDMA_PROTOCOL_IBOE,
> >> RDMA_PROTOCOL_IWARP,
> >> RDMA_PROTOCOL_USNIC_UDP
> >> };
> >>
> >> So we could use query_protocol() to ask device provide the protocol
> >> type, and there will be no mixing with the legacy transport type
> >> anymore :-)
> >
> > I'm ok with that. I like introducing a unique namespace which is clearly
> > different from the previous "transport" one.
>
> I agree the word "transport" takes things into the weeds.
>
> But on the topic of naming protocols, I've been wondering, is there
> some reason that "IBOE" is being used instead of "RoCE"?

Because back in the day, when RoCE was accepted into the kernel, I'm
pretty sure it was prior to the IBTA's final stamp of approval and
before the name was set on RoCE, so IBoE was chosen upstream as the more
"correct" name because it properly denoted what it was deemed to truly
be: IB Verbs over Ethernet.

> The IBOE
> protocol used to exist and is not the same as the currently
> standardized RoCE, right?

I don't believe so. To my knowledge, there was never an IBoE except in
linux upstream parlance.

> Also wondering, why add "UDP" to USNIC, is there a different USNIC?

Yes, there are two transports, one a distinct ethertype and one that
encapsulates USNIC in UDP.

> Naming multiple layers together seems confusing and maybe in the end
> will create more code to deal with the differences. For example, what
> token will RoCEv2 take? RoCE_UDP, RoCE_v2 or ... ?

Uncertain as of now.

--
Doug Ledford <[email protected]>
GPG KeyID: 0E572FDD

Attachments:

signature.asc (819.00 B)
This is a digitally signed message part

2015-04-28 00:53:25

by Tom Talpey

[permalink] [raw]

Subject: Re: [PATCH v6 01/26] IB/Verbs: Implement new callback query_transport()

On 4/27/2015 5:36 PM, Doug Ledford wrote:
> On Mon, 2015-04-27 at 17:16 -0700, Tom Talpey wrote:
>> On 4/27/2015 2:52 PM, ira.weiny wrote:
>>> On Mon, Apr 27, 2015 at 09:39:05AM +0200, Michael Wang wrote:
>>>> On 04/24/2015 05:12 PM, Liran Liss wrote:
>>>>> [snip]
>>>>
>>>> Like:
>>>>
>>>> enum rdma_protocol {
>>>> RDMA_PROTOCOL_IB,
>>>> RDMA_PROTOCOL_IBOE,
>>>> RDMA_PROTOCOL_IWARP,
>>>> RDMA_PROTOCOL_USNIC_UDP
>>>> };
>>>>
>>>> So we could use query_protocol() to ask device provide the protocol
>>>> type, and there will be no mixing with the legacy transport type
>>>> anymore :-)
>>>
>>> I'm ok with that. I like introducing a unique namespace which is clearly
>>> different from the previous "transport" one.
>>
>> I agree the word "transport" takes things into the weeds.
>>
>> But on the topic of naming protocols, I've been wondering, is there
>> some reason that "IBOE" is being used instead of "RoCE"?
>
> Because back in the day, when RoCE was accepted into the kernel, I'm
> pretty sure it was prior to the IBTA's final stamp of approval and
> before the name was set on RoCE, so IBoE was chosen upstream as the more
> "correct" name because it properly denoted what it was deemed to truly
> be: IB Verbs over Ethernet.

Well history is all well and good, but it seems weird to not use the
current, standard name in new code. It confuses me, anyway, because
it seems like IBOE could easily mean something else.

>> Also wondering, why add "UDP" to USNIC, is there a different USNIC?
>
> Yes, there are two transports, one a distinct ethertype and one that
> encapsulates USNIC in UDP.

But this new enum isn't about transport, it's about protocol. So is
there one USNIC protocol, with a raw layering and a separate one with
UDP? Or is it one USNIC protocol with two different framings? Seems
there should be at least the USNIC protocol, without the _UDP
decoration, and I don't see it in the enum.

>
>> Naming multiple layers together seems confusing and maybe in the end
>> will create more code to deal with the differences. For example, what
>> token will RoCEv2 take? RoCE_UDP, RoCE_v2 or ... ?
>
> Uncertain as of now.

Ok, but it's imminent, right? What's the preference/guidance?

2015-04-28 01:25:11

On Tue, 2015-04-28 at 22:11 +0300, Or Gerlitz wrote:
> On Tue, Apr 28, 2015 at 9:56 PM, Jason Gunthorpe
> <[email protected]> wrote:
> > On Mon, Apr 27, 2015 at 09:24:35PM -0400, Doug Ledford wrote:
> >> On Mon, 2015-04-27 at 17:53 -0700, Tom Talpey wrote:
> >
> >> Having some of it refer to things as IBOE and some as ROCE would be
> >> similarly confusing, and switching existing IBOE usage to ROCE would
> >> cause pain to people with out of tree drivers (Lustre is the main one I
> >> know of). There's not a good answer here. There's only less sucky
> >> ones.
> >
> > The tide has already turned, we should ditch iboe:
> >
> > $git grep -i roce_ drivers/infiniband/ | wc -l
> > 91
> > $git grep -i iboe_ drivers/infiniband/ | wc -l
> > 37
> >
> > It isn't really mainline's role to be too concerned about out of tree
> > things like Lustre.
>
> FWIW, note that Lustre is under staging for a while, not sure how
> close they are for actual acceptance.

I thought that was just the client and didn't include the server...

--
Doug Ledford <[email protected]>
GPG KeyID: 0E572FDD

Attachments:

signature.asc (819.00 B)
This is a digitally signed message part

2015-04-28 20:03:50

by Doug Ledford

[permalink] [raw]

Subject: Re: [PATCH v6 01/26] IB/Verbs: Implement new callback query_transport()

On Tue, 2015-04-28 at 12:56 -0600, Jason Gunthorpe wrote:
> On Mon, Apr 27, 2015 at 09:24:35PM -0400, Doug Ledford wrote:
> > On Mon, 2015-04-27 at 17:53 -0700, Tom Talpey wrote:
>
> > Having some of it refer to things as IBOE and some as ROCE would be
> > similarly confusing, and switching existing IBOE usage to ROCE would
> > cause pain to people with out of tree drivers (Lustre is the main one I
> > know of). There's not a good answer here. There's only less sucky
> > ones.
>
> The tide has already turned, we should ditch iboe:
>
> $git grep -i roce_ drivers/infiniband/ | wc -l
> 91
> $git grep -i iboe_ drivers/infiniband/ | wc -l
> 37
>
> It isn't really mainline's role to be too concerned about out of tree
> things like Lustre.

While I generally agree, one need not be totally callous about out of
tree things either.

--
Doug Ledford <[email protected]>
GPG KeyID: 0E572FDD

Attachments:

signature.asc (819.00 B)
This is a digitally signed message part

2015-04-28 20:26:36

by Dave Goodell

[permalink] [raw]

Subject: Re: [PATCH v6 01/26] IB/Verbs: Implement new callback query_transport()

On Apr 28, 2015, at 2:53 PM, Hefty, Sean <[email protected]> wrote:

>> Is the concern here about CM issues or the UDP ports used by the actual
>> usNIC RQs?
>
> UDP port space sharing

For the UDP port used by the usNIC QP, the usnic_verbs kernel driver requires user space to pass a file descriptor of a regular UDP socket down at create_qp time. The reference count on this socket is incremented to make sure that the socket can't disappear out from under us. Then an RX filter is installed in the NIC which matches UDP/IP/Ethernet packets that are destined for the UDP port to which the given socket is already bound. So there is a real UDP socket to make most of the usual things happen in the net stack, but the raw UDP/IP/Ethernet packets get delivered directly to the user space queues by the NIC. E.g., "netstat" and "lsof" show you proper addressing information, though obviously any information related to data-path statistics will not be accurate. At teardown we just reverse the steps.

However, I'm not sure if that's the sort of information you were looking for.

-Dave