On 25/08/2019 11:41, Michal Kalderon wrote:
>>> @@ -515,46 +408,55 @@ static int qp_mmap_entries_setup(struct efa_qp
>> *qp,
>>> struct efa_com_create_qp_params
>> *params,
>>> struct efa_ibv_create_qp_resp *resp) {
>>> + u64 address;
>>> + u64 length;
>>> +
>>> /*
>>> * Once an entry is inserted it might be mmapped, hence cannot be
>>> * cleaned up until dealloc_ucontext.
>>> */
>>> resp->sq_db_mmap_key =
>>
>> Not a big deal, but now it makes more sense to assign qp-
>>> sq_db_mmap_key and assign the response later on.
> ok
>>
>>> - mmap_entry_insert(dev, ucontext, qp,
>>> - dev->db_bar_addr + resp->sq_db_offset,
>>> - PAGE_SIZE, EFA_MMAP_IO_NC);
>>> - if (resp->sq_db_mmap_key == EFA_MMAP_INVALID)
>>> + rdma_user_mmap_entry_insert(&ucontext->ibucontext, qp,
>>> + dev->db_bar_addr +
>>> + resp->sq_db_offset,
>>> + PAGE_SIZE, EFA_MMAP_IO_NC);
>>> + if (resp->sq_db_mmap_key == RDMA_USER_MMAP_INVALID)
>>> return -ENOMEM;
>>> -
>>> + qp->sq_db_mmap_key = resp->sq_db_mmap_key;
>>> resp->sq_db_offset &= ~PAGE_MASK;
>>>
>>> + address = dev->mem_bar_addr + resp->llq_desc_offset;
>>> + length = PAGE_ALIGN(params->sq_ring_size_in_bytes +
>>> + (resp->llq_desc_offset & ~PAGE_MASK));
>>> resp->llq_desc_mmap_key =
>>> - mmap_entry_insert(dev, ucontext, qp,
>>> - dev->mem_bar_addr + resp-
>>> llq_desc_offset,
>>> - PAGE_ALIGN(params-
>>> sq_ring_size_in_bytes +
>>> - (resp->llq_desc_offset &
>> ~PAGE_MASK)),
>>> - EFA_MMAP_IO_WC);
>>> - if (resp->llq_desc_mmap_key == EFA_MMAP_INVALID)
>>> + rdma_user_mmap_entry_insert(&ucontext->ibucontext, qp,
>>> + address,
>>> + length,
>>> + EFA_MMAP_IO_WC);
>>> + if (resp->llq_desc_mmap_key == RDMA_USER_MMAP_INVALID)
>>> return -ENOMEM;
>>> -
>>> + qp->llq_desc_mmap_key = resp->llq_desc_mmap_key;
>>> resp->llq_desc_offset &= ~PAGE_MASK;
>>>
>>> if (qp->rq_size) {
>>> + address = dev->db_bar_addr + resp->rq_db_offset;
>>> resp->rq_db_mmap_key =
>>> - mmap_entry_insert(dev, ucontext, qp,
>>> - dev->db_bar_addr + resp-
>>> rq_db_offset,
>>> - PAGE_SIZE, EFA_MMAP_IO_NC);
>>> - if (resp->rq_db_mmap_key == EFA_MMAP_INVALID)
>>> + rdma_user_mmap_entry_insert(&ucontext-
>>> ibucontext, qp,
>>> + address, PAGE_SIZE,
>>> + EFA_MMAP_IO_NC);
>>> + if (resp->rq_db_mmap_key ==
>> RDMA_USER_MMAP_INVALID)
>>> return -ENOMEM;
>>> -
>>> + qp->rq_db_mmap_key = resp->rq_db_mmap_key;
>>> resp->rq_db_offset &= ~PAGE_MASK;
>>>
>>> + address = virt_to_phys(qp->rq_cpu_addr);
>>> resp->rq_mmap_key =
>>> - mmap_entry_insert(dev, ucontext, qp,
>>> - virt_to_phys(qp->rq_cpu_addr),
>>> - qp->rq_size,
>> EFA_MMAP_DMA_PAGE);
>>> - if (resp->rq_mmap_key == EFA_MMAP_INVALID)
>>> + rdma_user_mmap_entry_insert(&ucontext-
>>> ibucontext, qp,
>>> + address, qp->rq_size,
>>> + EFA_MMAP_DMA_PAGE);
>>> + if (resp->rq_mmap_key == RDMA_USER_MMAP_INVALID)
>>> return -ENOMEM;
>>> + qp->rq_mmap_key = resp->rq_mmap_key;
>>>
>>> resp->rq_mmap_size = qp->rq_size;
>>> }
>>> @@ -775,6 +677,9 @@ struct ib_qp *efa_create_qp(struct ib_pd *ibpd,
>>> DMA_TO_DEVICE);
>>> if (!rq_entry_inserted)
>>
>> Now that we store the keys on the QP object we can remove the
>> rq_entry_inserted variable and test for !qp->rq_mmap_key.
> ok
>>
>>> free_pages_exact(qp->rq_cpu_addr, qp->rq_size);
>>> + else
>>> + rdma_user_mmap_entry_remove(&ucontext-
>>> ibucontext,
>>> + qp->rq_mmap_key);
>>
>> Other entries need to be removed as well, otherwise the refcount won't
>> reach zero. This error flow should now be similar to efa_destroy_qp. I think
>> that means losing the free_pages_exact too.
> Not sure I understand, how can we loose the free_pages_exact ? if the entry wasn’t
> Inserted into the mmap_xa what flow will free the pages ?
You're right.
Still need to remove other entries though.
