On 2021/9/17 11:25, Yunsheng Lin wrote:

On 2021/9/16 18:36, Chengchang Tang wrote:

...

Add new implementation for mmap by using the new mmap entry API.

Perhaps mentions what has changed when using the new API? performance improvement or easy of usage for user?

OK， I will add more description to the commit log.

...

Signed-off-by: Chengchang Tang tangchengchang@huawei.com

drivers/infiniband/hw/hns/hns_roce_device.h | 21 ++++ drivers/infiniband/hw/hns/hns_roce_main.c | 144 +++++++++++++++++++++++++++- include/uapi/rdma/hns-abi.h | 21 +++- 3 files changed, 181 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/hw/hns/hns_roce_device.h b/drivers/infiniband/hw/hns/hns_roce_device.h index 9467c39..772dae2 100644 --- a/drivers/infiniband/hw/hns/hns_roce_device.h +++ b/drivers/infiniband/hw/hns/hns_roce_device.h @@ -225,11 +225,23 @@ struct hns_roce_uar { unsigned long logic_idx; };

+struct user_mmap_entry {

• struct rdma_user_mmap_entry rdma_entry;
• u64 address;
• u8 mmap_flag;

+};

+enum hns_roce_mmap_type {

• HNS_ROCE_MMAP_TYPE_DB = 1,

+};

struct hns_roce_ucontext { struct ib_ucontext ibucontext; struct hns_roce_uar uar; struct list_head page_list; struct mutex page_mutex;

• bool mmap_key_support;
• struct rdma_user_mmap_entry *db_mmap_entry;

};

struct hns_roce_pd { @@ -1049,6 +1061,12 @@ static inline struct hns_roce_srq *to_hr_srq(struct ib_srq *ibsrq) return container_of(ibsrq, struct hns_roce_srq, ibsrq); }

+static inline struct user_mmap_entry *to_hns_mmap(

"struct user_mmap_entry" does not seems like a entry specific to hns? if it is, maybe rename it to hns_user_mmap_entry or something like that?

Yes, I will rename it with prefix hns.

...

• struct rdma_user_mmap_entry *rdma_entry)
  

+{

• return container_of(rdma_entry, struct user_mmap_entry, rdma_entry);

+}

static inline void hns_roce_write64_k(__le32 val[2], void __iomem *dest) { writeq(*(u64 *)val, dest); @@ -1259,4 +1277,7 @@ int hns_roce_init(struct hns_roce_dev *hr_dev); void hns_roce_exit(struct hns_roce_dev *hr_dev); int hns_roce_fill_res_cq_entry(struct sk_buff *msg, struct ib_cq *ib_cq); +struct rdma_user_mmap_entry *hns_roce_user_mmap_entry_insert(

• struct ib_ucontext *ucontext, u64 address,
  

• size_t length, u8 mmap_flag);
  

#endif /* _HNS_ROCE_DEVICE_H */ diff --git a/drivers/infiniband/hw/hns/hns_roce_main.c b/drivers/infiniband/hw/hns/hns_roce_main.c index 5d39bd0..45643c2 100644 --- a/drivers/infiniband/hw/hns/hns_roce_main.c +++ b/drivers/infiniband/hw/hns/hns_roce_main.c @@ -291,6 +291,80 @@ static int hns_roce_modify_device(struct ib_device *ib_dev, int mask, return 0; }

+struct rdma_user_mmap_entry *hns_roce_user_mmap_entry_insert(

• 	struct ib_ucontext *ucontext, u64 address,
  

• 	size_t length, u8 mmap_flag)
  

+{

• struct user_mmap_entry *entry;
• int ret;
• entry = kzalloc(sizeof(*entry), GFP_KERNEL);
• if (!entry)

• return NULL;
  

• entry->address = address;
• entry->mmap_flag = mmap_flag;
• ret = rdma_user_mmap_entry_insert(ucontext, &entry->rdma_entry,

• 			  length);
  

• if (ret) {

• kfree(entry);
  

• return NULL;
  

• }
• return &entry->rdma_entry;

+}

+static void hns_roce_dealloc_uar_entry(struct hns_roce_ucontext *context) +{

• rdma_user_mmap_entry_remove(context->db_mmap_entry);

+}

+static int hns_roce_alloc_uar_entry(struct ib_ucontext *uctx) +{

• struct hns_roce_ucontext *context = to_hr_ucontext(uctx);
• u64 address;
• if (!context->mmap_key_support)

• return 0;
  

• address = context->uar.pfn << PAGE_SHIFT;
• context->db_mmap_entry =

• hns_roce_user_mmap_entry_insert(uctx, address, PAGE_SIZE,
  

• 				HNS_ROCE_MMAP_TYPE_DB);
  

• if (!context->db_mmap_entry)

• return -ENOMEM;
  

• return 0;

+}

+static void get_ucontext_config(struct hns_roce_ucontext *context,

• 		struct hns_roce_ib_alloc_ucontext *ucmd)
  

+{

• struct hns_roce_dev *hr_dev = to_hr_dev(context->ibucontext.device);
• if (ucmd->comp & HNS_ROCE_ALLOC_UCTX_COMP_CONFIG && hr_dev->hw_rev !=

• 	HNS_ROCE_HW_VER1)
  

• context->mmap_key_support = ucmd->config &
  

• 			    HNS_ROCE_UCTX_SET_MMAP_KEY;
  

context->mmap_key_support seems to be a bool type, and if HNS_ROCE_UCTX_SET_MMAP_KEY is 0x2 instead of 0x1, then we may be assigning non 0/1 to a bool type?

I'm not sure if I understand your question, let me try to explain it.

In subsequent patches, config will be expanded. So, considering the compatibility issues, the bit by bit judgment is relatively safe.

...

+}

+static void ucontext_set_resp(struct ib_ucontext *uctx,

• 	      struct hns_roce_ib_alloc_ucontext_resp *resp)
  

+{

• struct hns_roce_ucontext *context = to_hr_ucontext(uctx);
• struct hns_roce_dev *hr_dev = to_hr_dev(uctx->device);
• resp->qp_tab_size = hr_dev->caps.num_qps;
• resp->cqe_size = hr_dev->caps.cqe_sz;
• resp->srq_tab_size = hr_dev->caps.num_srqs;
• if (context->mmap_key_support) {

• resp->config |= HNS_ROCE_UCTX_CONFIG_MMAP_KEY;
  

• resp->db_mmap_key =
  

• 	rdma_user_mmap_get_offset(context->db_mmap_entry);
  

• }

+}

static int hns_roce_alloc_ucontext(struct ib_ucontext *uctx, struct ib_udata *udata) { @@ -298,24 +372,35 @@ static int hns_roce_alloc_ucontext(struct ib_ucontext *uctx, struct hns_roce_ucontext *context = to_hr_ucontext(uctx); struct hns_roce_ib_alloc_ucontext_resp resp = {}; struct hns_roce_dev *hr_dev = to_hr_dev(uctx->device);

• struct hns_roce_ib_alloc_ucontext ucmd = {};

  if (!hr_dev->active) return -EAGAIN;

• resp.qp_tab_size = hr_dev->caps.num_qps;
• resp.srq_tab_size = hr_dev->caps.num_srqs;

• if (udata->inlen) {

• ret = ib_copy_from_udata(&ucmd, udata,
  

• 			 min(udata->inlen, sizeof(ucmd)));
  

• if (ret)
  

• 	return ret;
  

• }

• get_ucontext_config(context, &ucmd);

  ret = hns_roce_uar_alloc(hr_dev, &context->uar); if (ret) goto error_fail_uar_alloc;

• ret = hns_roce_alloc_uar_entry(uctx);

• if (ret)

• goto error_fail_uar_entry;
  

• if (hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_CQ_RECORD_DB || hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_QP_RECORD_DB) { INIT_LIST_HEAD(&context->page_list); mutex_init(&context->page_mutex); }

• resp.cqe_size = hr_dev->caps.cqe_sz;

• ucontext_set_resp(uctx, &resp);

  ret = ib_copy_to_udata(udata, &resp, min(udata->outlen, sizeof(resp)));

@@ -325,6 +410,9 @@ static int hns_roce_alloc_ucontext(struct ib_ucontext *uctx, return 0;

error_fail_copy_to_udata:

• hns_roce_dealloc_uar_entry(context);

+error_fail_uar_entry: ida_free(&hr_dev->uar_ida.ida, (int)context->uar.logic_idx);

error_fail_uar_alloc: @@ -336,10 +424,12 @@ static void hns_roce_dealloc_ucontext(struct ib_ucontext *ibcontext) struct hns_roce_ucontext *context = to_hr_ucontext(ibcontext); struct hns_roce_dev *hr_dev = to_hr_dev(ibcontext->device);

• hns_roce_dealloc_uar_entry(context);
• ida_free(&hr_dev->uar_ida.ida, (int)context->uar.logic_idx);

}

-static int hns_roce_mmap(struct ib_ucontext *context, +static int hns_roce_legacy_mmap(struct ib_ucontext *context, struct vm_area_struct *vma) { struct hns_roce_dev *hr_dev = to_hr_dev(context->device); @@ -371,6 +461,51 @@ static int hns_roce_mmap(struct ib_ucontext *context, } }

+static int hns_roce_mmap(struct ib_ucontext *uctx, struct vm_area_struct *vma) +{

• struct hns_roce_ucontext *context = to_hr_ucontext(uctx);
• struct hns_roce_dev *hr_dev = to_hr_dev(uctx->device);
• struct ib_device *ibdev = &hr_dev->ib_dev;
• struct rdma_user_mmap_entry *rdma_entry;
• struct user_mmap_entry *entry;
• phys_addr_t pfn;
• pgprot_t prot;
• int ret;
• if (!context->mmap_key_support)

• return hns_roce_legacy_mmap(uctx, vma);
  

• rdma_entry = rdma_user_mmap_entry_get_pgoff(uctx, vma->vm_pgoff);
• if (!rdma_entry) {

• ibdev_err(ibdev, "pgoff[%lu] is invalid entry.\n",
  

• 		vma->vm_pgoff);
  

• return -EINVAL;
  

• }
• entry = to_hns_mmap(rdma_entry);
• pfn = entry->address >> PAGE_SHIFT;
• prot = vma->vm_page_prot;
• switch (entry->mmap_flag) {
• case HNS_ROCE_MMAP_TYPE_DB:

• ret = rdma_user_mmap_io(uctx, vma, pfn,
  

• 			rdma_entry->npages * PAGE_SIZE,
  

• 			pgprot_noncached(prot), rdma_entry);
  

• break;
  

• default:

• ret =  -EINVAL;
  

• }
• rdma_user_mmap_entry_put(rdma_entry);
• return ret;

+}

+static void hns_roce_free_mmap(struct rdma_user_mmap_entry *rdma_entry) +{

• struct user_mmap_entry *entry = to_hns_mmap(rdma_entry);
• kfree(entry);

+}

static int hns_roce_port_immutable(struct ib_device *ib_dev, u32 port_num, struct ib_port_immutable *immutable) { @@ -444,6 +579,7 @@ static const struct ib_device_ops hns_roce_dev_ops = { .get_link_layer = hns_roce_get_link_layer, .get_port_immutable = hns_roce_port_immutable, .mmap = hns_roce_mmap,

• .mmap_free = hns_roce_free_mmap, .modify_device = hns_roce_modify_device, .modify_qp = hns_roce_modify_qp, .query_ah = hns_roce_query_ah,

diff --git a/include/uapi/rdma/hns-abi.h b/include/uapi/rdma/hns-abi.h index 42b1776..eabdfd4 100644 --- a/include/uapi/rdma/hns-abi.h +++ b/include/uapi/rdma/hns-abi.h @@ -83,11 +83,30 @@ struct hns_roce_ib_create_qp_resp { __aligned_u64 cap_flags; };

+enum {

• HNS_ROCE_ALLOC_UCTX_COMP_CONFIG = 1 << 0,

+};

+enum {

• HNS_ROCE_UCTX_CONFIG_MMAP_KEY = 1 << 0,

+};

+enum {

• HNS_ROCE_UCTX_SET_MMAP_KEY = 1 << 0,

+};

We may need better name for the above eunm? It does not seems clear to me what does the above three enum related to each other?

You are right, and how to name them really bothers me.

Let me explain the meaning of these enum mentioned above, and I hope you could provide me some better naming suggestions.

The first one 'HNS_ROCE_ALLOC_UCTX_COMP_CONFIG' is the compatible flag which will be used in comp field. It means the config field is set.

'HNS_ROCE_UCTX_CONFIG_MMAP_KEY' and 'HNS_ROCE_UCTX_SET_MMAP_KEY' are paired. 'SET_MMAP_KEY' means that user mode libhns requires to set the new mmap mode. And 'CONFIG_MMAP_KEY' is the response of the kernel mode driver to the request, it implied that the final configuration in the kernel driver.

...

+struct hns_roce_ib_alloc_ucontext {

• __u32 comp;

comp stands for complete?

No, it means compatibility. Different versions of kernel mode driver and user mode driver can be compatible with each other through this field, while ensuring the scalability of this structure.

...

• __u32 config;

+};

struct hns_roce_ib_alloc_ucontext_resp { __u32 qp_tab_size; __u32 cqe_size; __u32 srq_tab_size;

• __u32 reserved;

• __u8 config;
• __u8 rsv[3];
• __aligned_u64 db_mmap_key;

};

struct hns_roce_ib_alloc_pd_resp {

2.9.5

.

.