How far back does rdma-core X supports the kernel Y?

How far back does rdma-core X supports the kernel Y?

[Amrani, Ram]

Hi All,
I've installed rdma-core-15 on kernel 4.3 and ran into issues.

It seems this kernel doesn't include the following patch:
https://github.com/torvalds/linux/commit/aa744cc01fe0f21dfbe2744d3fd5f2fb3244c9b3

So IB_QP_SMAC is present in kernel 4.3 but not in rdma-core-15.
This caused a failure here:

int ibv_cmd_modify_qp(struct ibv_qp *qp, struct ibv_qp_attr *attr,
                      int attr_mask,
                      struct ibv_modify_qp *cmd, size_t cmd_size)
{
        /*
         * Masks over IBV_QP_DEST_QPN are only supported by
         * ibv_cmd_modify_qp_ex.
         */
        if (attr_mask & ~((IBV_QP_DEST_QPN << 1) - 1))
                return EOPNOTSUPP;
        ...
}

Do we expect these versions to operate?
If not, from what version do we support backward compatibility?
Are there any other backward compatibility issues you are aware of?

Thanks,
Ram

--
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: How far back does rdma-core X supports the kernel Y?

[Jason Gunthorpe]

On Wed, Nov 01, 2017 at 12:07:07PM +0000, Amrani, Ram wrote:
> Hi All,
> I've installed rdma-core-15 on kernel 4.3 and ran into issues.
> 
> It seems this kernel doesn't include the following patch:
> https://github.com/torvalds/linux/commit/aa744cc01fe0f21dfbe2744d3fd5f2fb3244c9b3
> 
> So IB_QP_SMAC is present in kernel 4.3 but not in rdma-core-15.

> This caused a failure here:
>
> int ibv_cmd_modify_qp(struct ibv_qp *qp, struct ibv_qp_attr *attr,
>                       int attr_mask,
>                       struct ibv_modify_qp *cmd, size_t cmd_size)
> {
>         /*
>          * Masks over IBV_QP_DEST_QPN are only supported by
>          * ibv_cmd_modify_qp_ex.
>          */
>         if (attr_mask & ~((IBV_QP_DEST_QPN << 1) - 1))
>                 return EOPNOTSUPP;

WTF is this?

Based on the comment it should be:

         if (attr_mask & ~((1ULL << (IBV_QP_DEST_QPN + 1)) - 1))

To match the comment. And other places have the same mistake.

This was broken recently by 3ca7a1031486b588a7fbe0d262f8f89738215b58

Bodong??

> Do we expect these versions to operate?
> If not, from what version do we support backward compatibility?
> Are there any other backward compatibility issues you are aware of?

rdma-core should work with all kernel versions, anything else is a bad
bug.

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

RE: How far back does rdma-core X supports the kernel Y?

[Amrani, Ram]

> WTF is this?
> 
> Based on the comment it should be:
> 
>          if (attr_mask & ~((1ULL << (IBV_QP_DEST_QPN + 1)) - 1))
> 
> To match the comment. And other places have the same mistake.
> 
> This was broken recently by 3ca7a1031486b588a7fbe0d262f8f89738215b58
> 
> Bodong??
> 

Oh yeah, that too.
But my point is that even after I've fixed this (I haven't checked other locations),
then backwardly compatibility is foo because the above check fails.
If you take a look at that kernel you can see it sets IB_QP_SMAC in attr_mask
and returns it to the user space. When modify QP is invoked it fails on that
check (IB_QP_DEST_QPN < IB_QP_SMAC).
This happens perhaps for the other bits. Haven't checked it yet.

For compatibility, we should pass the bits as they are (i.e. not fail the check), and ignore them
in later kernels.
Also, currently they are marked in ib_verbs.h as reserved. This is misleading as
it hints they are for future use. We should mark them as don't ever use.

If that sounds right to you, I can fix it. But it'll have wait till end of next week because
I'm not in the office.

Ram

--
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: How far back does rdma-core X supports the kernel Y?

[Yishai Hadas]

On 11/1/2017 6:39 PM, Jason Gunthorpe wrote:
> On Wed, Nov 01, 2017 at 12:07:07PM +0000, Amrani, Ram wrote:
>> Hi All,
>> I've installed rdma-core-15 on kernel 4.3 and ran into issues.
>>
>> It seems this kernel doesn't include the following patch:
>> https://github.com/torvalds/linux/commit/aa744cc01fe0f21dfbe2744d3fd5f2fb3244c9b3
>>
>> So IB_QP_SMAC is present in kernel 4.3 but not in rdma-core-15.

Was it ever appeared at verbs.h in libibverbs ? this value from kernel 
4.3 (IB_QP_SMAC = (1<<21)) appears in current kernel as IB_QP_RESERVED1.

>> This caused a failure here:
>>
>> int ibv_cmd_modify_qp(struct ibv_qp *qp, struct ibv_qp_attr *attr,
>>                        int attr_mask,
>>                        struct ibv_modify_qp *cmd, size_t cmd_size)
>> {
>>          /*
>>           * Masks over IBV_QP_DEST_QPN are only supported by
>>           * ibv_cmd_modify_qp_ex.
>>           */
>>          if (attr_mask & ~((IBV_QP_DEST_QPN << 1) - 1))
>>                  return EOPNOTSUPP;
> 
> WTF is this?
> 
> Based on the comment it should be:
> 
>           if (attr_mask & ~((1ULL << (IBV_QP_DEST_QPN + 1)) - 1))

The above rdma-core code is correct, look at verbs.h:
IBV_QP_DEST_QPN = 1 << 20,
--
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: How far back does rdma-core X supports the kernel Y?

[Jason Gunthorpe]

On Wed, Nov 01, 2017 at 05:27:22PM +0000, Amrani, Ram wrote:
> > WTF is this?
> > 
> > Based on the comment it should be:
> > 
> >          if (attr_mask & ~((1ULL << (IBV_QP_DEST_QPN + 1)) - 1))
> > 
> > To match the comment. And other places have the same mistake.

> > This was broken recently by 3ca7a1031486b588a7fbe0d262f8f89738215b58
> > 
> > Bodong??
> > 
> 
> Oh yeah, that too.
> But my point is that even after I've fixed this (I haven't checked other locations),
> then backwardly compatibility is foo because the above check fails.

> If you take a look at that kernel you can see it sets IB_QP_SMAC in attr_mask
> and returns it to the user space. When modify QP is invoked it fails on that
> check (IB_QP_DEST_QPN < IB_QP_SMAC).

rdma-cm sets it?

> This happens perhaps for the other bits. Haven't checked it yet.

Yes, that test should pass all the historical bits supported by
all older kernels.

// Anything including and beyond RATE_LIMIT must use the _ex API
if (attr_mask & ~((1ULL << IBV_QP_RATE_LIMIT) - 1))

> Also, currently they are marked in ib_verbs.h as reserved. This is misleading as
> it hints they are for future use. We should mark them as don't ever use.

We have tended to use reserved to mean dont use. Someone should
inspect the git history before trying to use a reserved value..

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

Re: How far back does rdma-core X supports the kernel Y?

[Jason Gunthorpe]

> >          if (attr_mask & ~((1ULL << (IBV_QP_DEST_QPN + 1)) - 1))
> 
> The above rdma-core code is correct, look at verbs.h:
> IBV_QP_DEST_QPN = 1 << 20,

Okay, then it should be

 if (attr_mask & ~(IB_QP_RATE_LIMIT - 1))

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

Re: How far back does rdma-core X supports the kernel Y?

[Yishai Hadas]

On 11/1/2017 8:00 PM, Jason Gunthorpe wrote:
>>>           if (attr_mask & ~((1ULL << (IBV_QP_DEST_QPN + 1)) - 1))
>>
>> The above rdma-core code is correct, look at verbs.h:
>> IBV_QP_DEST_QPN = 1 << 20,
> 
> Okay, then it should be
> 
>   if (attr_mask & ~(IB_QP_RATE_LIMIT - 1))
> 

Why the library should support values that were never exposed in verbs.h ?

In addition, running over latest few kernel with your suggestion will 
fail any way [1] as those bits are marked as RESERVED in IB core and 
aren't really supported.

[1]
http://elixir.free-electrons.com/linux/v4.14-rc7/source/drivers/infiniband/core/uverbs_cmd.c#L2041
--
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: How far back does rdma-core X supports the kernel Y?

[Jason Gunthorpe]

On Wed, Nov 01, 2017 at 08:22:37PM +0200, Yishai Hadas wrote:
> On 11/1/2017 8:00 PM, Jason Gunthorpe wrote:
> >>>          if (attr_mask & ~((1ULL << (IBV_QP_DEST_QPN + 1)) - 1))
> >>
> >>The above rdma-core code is correct, look at verbs.h:
> >>IBV_QP_DEST_QPN = 1 << 20,
> >
> >Okay, then it should be
> >
> >  if (attr_mask & ~(IB_QP_RATE_LIMIT - 1))
> >
> 
> Why the library should support values that were never exposed in verbs.h ?

I think Ram is explaining that the kernel rdma_cm provides the
attr_mask to use with modify_qp, and that the 4.3 kernel sets the
three now reserved bits. The expectation is that user space will flow
those bits and values from the kernel rdma_cm interface back to
uverbs.

Userspace never looks at the attr_mask, and it does not matter that
the values were never in verbs.h.

rdma-core is expected to work on 4.3, so if 4.3 is using this kind of
path, we cannot block it in userspace.

At least, I think that is what Ram is saying.

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

Re: How far back does rdma-core X supports the kernel Y?

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 921 bytes --]

On Wed, Nov 01, 2017 at 11:33:24AM -0600, Jason Gunthorpe wrote:
> On Wed, Nov 01, 2017 at 05:27:22PM +0000, Amrani, Ram wrote:
>
> > Also, currently they are marked in ib_verbs.h as reserved. This is misleading as
> > it hints they are for future use. We should mark them as don't ever use.
>
> We have tended to use reserved to mean dont use. Someone should
> inspect the git history before trying to use a reserved value..

Why don't we properly export these defines in uapi/rdma/ib_user_verbs.h?

The fact that change in internal to kernel value breaks user space
shows that interface between user-space (rdma-core) <-> kernel needs
improvement in sharing of the headers.

Thanks

>
> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: How far back does rdma-core X supports the kernel Y?

[Amrani, Ram]

> > >Okay, then it should be
> > >
> > >  if (attr_mask & ~(IB_QP_RATE_LIMIT - 1))
> > >

I'm OK with this

> I think Ram is explaining that the kernel rdma_cm provides the
> attr_mask to use with modify_qp, and that the 4.3 kernel sets the
> three now reserved bits. The expectation is that user space will flow
> those bits and values from the kernel rdma_cm interface back to
> uverbs.
> 
> Userspace never looks at the attr_mask, and it does not matter that
> the values were never in verbs.h.
> 
> rdma-core is expected to work on 4.3, so if 4.3 is using this kind of
> path, we cannot block it in userspace.
> 
> At least, I think that is what Ram is saying.
> 
> Jason

Exactly.
Using IB_QP_RATE_LIMIT instead of IB_QP_DEST_QPN solves this.

As I've said I can create a patch but it'll have to wait to e/o next week.
What are the other locations that you've seen similar errors?

Thanks,
Ram

--
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: How far back does rdma-core X supports the kernel Y?

[Amrani, Ram]

> We have tended to use reserved to mean dont use. Someone should
> inspect the git history before trying to use a reserved value..
> 

When reserved are used in structures, usually for alignment, they are
freely taken when a new field needs adding.
Taking this logic to enums is bad.
I agree that in enums it does raise a brow and it is evident this should
be checked.
Still, I think it is a better practice to give a more meaningful name
and/or comment to differentiate. E.g.:

IB_QP_XXX_DONT_USE /* Kept for backward compatibility only */

Thanks,
Ram

--
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: How far back does rdma-core X supports the kernel Y?

[Jason Gunthorpe]

On Thu, Nov 02, 2017 at 07:01:58AM +0000, Amrani, Ram wrote:
> > We have tended to use reserved to mean dont use. Someone should
> > inspect the git history before trying to use a reserved value..
> > 
> 
> When reserved are used in structures, usually for alignment, they are
> freely taken when a new field needs adding.

Even then you should check the git history to figure out why the field
is reserved. Padding ones are OK to steal, but we also have deprecated
struct members. :\

> Still, I think it is a better practice to give a more meaningful name
> and/or comment to differentiate. E.g.:
> 
> IB_QP_XXX_DONT_USE /* Kept for backward compatibility only */

It would be good to do this, yes.

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