On 12-Dec-18 12:24, Shamir Rabinovitch wrote:
> On Wed, Dec 12, 2018 at 11:09:05AM +0200, Gal Pressman wrote:
>> Introduce a 'flags' field to create address handle callback and add a
>> flag that marks whether the callback is executed in an atomic context or
>> not.
>> This will allow drivers to wait for completion instead of polling for it
>> when it is allowed.
>>
>> Signed-off-by: Gal Pressman <galpress@xxxxxxxxxx>
>> ---
>> drivers/infiniband/core/cm.c | 2 +-
>> drivers/infiniband/core/sa_query.c | 3 ++-
>> drivers/infiniband/core/verbs.c | 15 ++++++++++-----
>> drivers/infiniband/hw/bnxt_re/ib_verbs.c | 1 +
>> drivers/infiniband/hw/bnxt_re/ib_verbs.h | 1 +
>> drivers/infiniband/hw/hfi1/mad.c | 2 +-
>> drivers/infiniband/hw/hns/hns_roce_ah.c | 1 +
>> drivers/infiniband/hw/hns/hns_roce_device.h | 1 +
>> drivers/infiniband/hw/mlx4/ah.c | 4 ++--
>> drivers/infiniband/hw/mlx4/mad.c | 4 ++--
>> drivers/infiniband/hw/mlx4/mlx4_ib.h | 2 +-
>> drivers/infiniband/hw/mlx5/ah.c | 2 +-
>> drivers/infiniband/hw/mlx5/mlx5_ib.h | 2 +-
>> drivers/infiniband/hw/mthca/mthca_mad.c | 2 +-
>> drivers/infiniband/hw/mthca/mthca_provider.c | 1 +
>> drivers/infiniband/hw/ocrdma/ocrdma_ah.c | 2 +-
>> drivers/infiniband/hw/ocrdma/ocrdma_ah.h | 2 +-
>> drivers/infiniband/hw/qedr/verbs.c | 2 +-
>> drivers/infiniband/hw/qedr/verbs.h | 2 +-
>> drivers/infiniband/hw/qib/qib_verbs.c | 2 +-
>> drivers/infiniband/hw/usnic/usnic_ib_verbs.c | 1 +
>> drivers/infiniband/hw/usnic/usnic_ib_verbs.h | 1 +
>> drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.c | 3 ++-
>> drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.h | 2 +-
>> drivers/infiniband/sw/rdmavt/ah.c | 2 ++
>> drivers/infiniband/sw/rdmavt/ah.h | 1 +
>> drivers/infiniband/sw/rxe/rxe_verbs.c | 1 +
>> drivers/infiniband/ulp/ipoib/ipoib_ib.c | 2 +-
>> drivers/infiniband/ulp/opa_vnic/opa_vnic_vema.c | 2 +-
>> include/rdma/ib_verbs.h | 10 +++++++++-
>> 30 files changed, 52 insertions(+), 26 deletions(-)
>
> Have you missed 'ib_uverbs_create_ah' ?
>
> I ask this because you created new flag between uverbs and rdma layers
> and it seems that you are not 100% consistent here where you pass this
> flag from uverbs in error path of ib_uverbs_create_ah (patch 2/4) but
> not in the valid path.
>
> I'd expect you to pass this flag from ib_uverbs_create_ah to
> rdma_create_user_ah as well.
rdma_create_ah and rdma_destroy_ah aren't exactly symmetric, _rdma_create_ah and
rdma_destroy_ah are.
_rdma_create_ah is called from two functions:
1. rdma_create_ah, which has the 'flags' parameter, since it's called both from
atomic and non-atomic context.
2. rdma_create_user_ah, which doesn't need the 'flags' parameter, since it's
always called in non-atomic context, hence always passes
RDMA_CREATE_AH_SLEEPABLE flag to _rdma_create_ah.
The function you mentioned (ib_uverbs_create_ah) doesn't need the flags on
create (since it calls rdma_creat_user_ah) and needs it on destroy.
>
>>
>> diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
>> index edb2cb758be7..cf5b3c4314bb 100644
>> --- a/drivers/infiniband/core/cm.c
>> +++ b/drivers/infiniband/core/cm.c
>> @@ -343,7 +343,7 @@ static int cm_alloc_msg(struct cm_id_private *cm_id_priv,
>> ret = -ENODEV;
>> goto out;
>> }
>> - ah = rdma_create_ah(mad_agent->qp->pd, &av->ah_attr);
>> + ah = rdma_create_ah(mad_agent->qp->pd, &av->ah_attr, 0);
>> if (IS_ERR(ah)) {
>> ret = PTR_ERR(ah);
>> goto out;
>> diff --git a/drivers/infiniband/core/sa_query.c b/drivers/infiniband/core/sa_query.c
>> index be5ba5e15496..fb96d9df1fdd 100644
>> --- a/drivers/infiniband/core/sa_query.c
>> +++ b/drivers/infiniband/core/sa_query.c
>> @@ -2276,7 +2276,8 @@ static void update_sm_ah(struct work_struct *work)
>> cpu_to_be64(IB_SA_WELL_KNOWN_GUID));
>> }
>>
>> - new_ah->ah = rdma_create_ah(port->agent->qp->pd, &ah_attr);
>> + new_ah->ah = rdma_create_ah(port->agent->qp->pd, &ah_attr,
>> + RDMA_CREATE_AH_SLEEPABLE);
>> if (IS_ERR(new_ah->ah)) {
>> pr_warn("Couldn't create new SM AH\n");
>> kfree(new_ah);
>> diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
>> index 178899e3ce73..6fd8179ce82e 100644
>> --- a/drivers/infiniband/core/verbs.c
>> +++ b/drivers/infiniband/core/verbs.c
>> @@ -475,14 +475,17 @@ rdma_update_sgid_attr(struct rdma_ah_attr *ah_attr,
>>
>> static struct ib_ah *_rdma_create_ah(struct ib_pd *pd,
>> struct rdma_ah_attr *ah_attr,
>> + u32 flags,
>> struct ib_udata *udata)
>> {
>
> Why you do not add the creation flags to rdma_ah_attr?
>
> Something like
>
> struct rdma_ah_attr {
> [...]
> u8 ah_flags;
> u32 ah_creation_flags;
> [...]
> }
>
> It is already set to 0 in ib_uverbs_create_ah.
>
> You just need to make sure the rest of the none user path (such as mad)
> also set this to 0.
I considered it, but sleepable is not an attribute of the address handle itself,
it's an attribute of the function call context so placing it as an extra
parameter made more sense to me.
>
>> struct ib_ah *ah;
>>
>> + might_sleep_if(flags & RDMA_CREATE_AH_SLEEPABLE);
>> +
>> if (!pd->device->create_ah)
>> return ERR_PTR(-EOPNOTSUPP);
>>
>> - ah = pd->device->create_ah(pd, ah_attr, udata);
>> + ah = pd->device->create_ah(pd, ah_attr, flags, udata);
>>
