Re: [PATCH rdma-next v1 1/2] RDMA: Enable Relaxed Ordering by default for kernel ULPs

From: Jason Gunthorpe <jgg@nvidia.com>
To: Leon Romanovsky <leon@kernel.org>
Cc: Doug Ledford <dledford@redhat.com>,
	Avihai Horon <avihaih@nvidia.com>,
	linux-kernel@vger.kernel.org, linux-rdma@vger.kernel.org
Subject: Re: [PATCH rdma-next v1 1/2] RDMA: Enable Relaxed Ordering by default for kernel ULPs
Date: Fri, 28 May 2021 15:27:46 -0300	[thread overview]
Message-ID: <20210528182746.GA3645229@nvidia.com> (raw)
In-Reply-To: <73af770234656d5f884ead5b8d40132d9ed289d6.1621505111.git.leonro@nvidia.com>

On Thu, May 20, 2021 at 01:13:35PM +0300, Leon Romanovsky wrote:
> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> index 05dbc216eb64..b7bda44e9189 100644
> +++ b/include/rdma/ib_verbs.h
> @@ -1440,7 +1440,7 @@ enum ib_access_flags {
>  	IB_ZERO_BASED = IB_UVERBS_ACCESS_ZERO_BASED,
>  	IB_ACCESS_ON_DEMAND = IB_UVERBS_ACCESS_ON_DEMAND,
>  	IB_ACCESS_HUGETLB = IB_UVERBS_ACCESS_HUGETLB,
> -	IB_ACCESS_RELAXED_ORDERING = IB_UVERBS_ACCESS_RELAXED_ORDERING,
> +	IB_ACCESS_DISABLE_RELAXED_ORDERING = IB_UVERBS_ACCESS_OPTIONAL_FIRST,
>  
>  	IB_ACCESS_OPTIONAL = IB_UVERBS_ACCESS_OPTIONAL_RANGE,
>  	IB_ACCESS_SUPPORTED =

IB_ACCESS_SUPPORTED should be deleted too

> -				 IB_ACCESS_SUPPORTED);
> +				 ((IB_UVERBS_ACCESS_HUGETLB << 1) - 1) |
> +					 IB_UVERBS_ACCESS_OPTIONAL_RANGE);

This would do well as a IB_UVERBS_ACCESS_MR_SUPPORTED constant

> @@ -4679,4 +4679,70 @@ static inline u32 rdma_calc_flow_label(u32 lqpn, u32 rqpn)
>  
>  const struct ib_port_immutable*
>  ib_port_immutable_read(struct ib_device *dev, unsigned int port);
> +
> +static inline void process_access_flag(unsigned int *dest_flags,
> +				       unsigned int out_flag,
> +				       unsigned int *src_flags,
> +				       unsigned int in_flag)
> +{
> +	if (!(*src_flags & in_flag))
> +		return;
> +
> +	*dest_flags |= out_flag;
> +	*src_flags &= ~in_flag;
> +}
> +
> +static inline void process_access_flag_inv(unsigned int *dest_flags,
> +					   unsigned int out_flag,
> +					   unsigned int *src_flags,
> +					   unsigned int in_flag)
> +{
> +	if (*src_flags & in_flag) {
> +		*dest_flags &= ~out_flag;
> +		*src_flags &= ~in_flag;
> +		return;
> +	}
> +
> +	*dest_flags |= out_flag;
> +}
> +
> +static inline int copy_mr_access_flags(unsigned int *dest_flags,
> +				       unsigned int src_flags)
> +{
> +	*dest_flags = 0;
> +
> +	process_access_flag(dest_flags, IB_ACCESS_LOCAL_WRITE, &src_flags,
> +			    IB_UVERBS_ACCESS_LOCAL_WRITE);
> +
> +	process_access_flag(dest_flags, IB_ACCESS_REMOTE_WRITE, &src_flags,
> +			    IB_UVERBS_ACCESS_REMOTE_WRITE);
> +
> +	process_access_flag(dest_flags, IB_ACCESS_REMOTE_READ, &src_flags,
> +			    IB_UVERBS_ACCESS_REMOTE_READ);
> +
> +	process_access_flag(dest_flags, IB_ACCESS_REMOTE_ATOMIC, &src_flags,
> +			    IB_UVERBS_ACCESS_REMOTE_ATOMIC);
> +
> +	process_access_flag(dest_flags, IB_ACCESS_MW_BIND, &src_flags,
> +			    IB_UVERBS_ACCESS_MW_BIND);
> +
> +	process_access_flag(dest_flags, IB_ZERO_BASED, &src_flags,
> +			    IB_UVERBS_ACCESS_ZERO_BASED);
> +
> +	process_access_flag(dest_flags, IB_ACCESS_ON_DEMAND, &src_flags,
> +			    IB_UVERBS_ACCESS_ON_DEMAND);
> +
> +	process_access_flag(dest_flags, IB_ACCESS_HUGETLB, &src_flags,
> +			    IB_UVERBS_ACCESS_HUGETLB);
> +
> +	process_access_flag_inv(dest_flags, IB_ACCESS_DISABLE_RELAXED_ORDERING,
> +				&src_flags, IB_UVERBS_ACCESS_RELAXED_ORDERING);

This seems over complicated, why not just:

dst_flags = IB_ACCESS_DISABLE_RELAXED_ORDERING
if (src_flags & IB_UVERBS_ACCESS_LOCAL_WRITE)
    dst_flags |= IB_ACCESS_LOCAL_WRITE;
if (src_flags & IB_UVERBS_ACCESS_RELAXED_ORDERING)
    dst_flags &= ~IB_ACCESS_DISABLE_RELAXED_ORDERING;

if (src_flags & ~IB_UVERBS_ACCESS_MR_SUPPORTED)
  return -EINVAL;

And the QP version is the same as the MR, just with a different
supported flags check

Jason