On 2018/3/1 5:34, Jason Gunthorpe wrote:
> On Mon, Feb 26, 2018 at 03:43:09PM +0800, Liuyixian (Eason) wrote:
>>
>>
>> On 2018/2/16 7:23, Jason Gunthorpe wrote:
>>> On Mon, Feb 12, 2018 at 06:14:08PM +0800, Yixian Liu wrote:
>>>> +int hns_roce_db_map_user(struct hns_roce_ucontext *context, unsigned long virt,
>>>> + struct hns_roce_db *db)
>>>> +{
>>>> + struct hns_roce_user_db_page *db_page;
>>>> + int ret = 0;
>>>> +
>>>> + mutex_lock(&context->db_page_mutex);
>>>> +
>>>> + list_for_each_entry(db_page, &context->db_page_list, list)
>>>> + if (db_page->user_virt == (virt & PAGE_MASK))
>>>> + goto found;
>>>> +
>>>> + db_page = kmalloc(sizeof(*db_page), GFP_KERNEL);
>>>> + if (!db_page) {
>>>> + ret = -ENOMEM;
>>>> + goto out;
>>>> + }
>>>> +
>>>> + db_page->user_virt = (virt & PAGE_MASK);
>>>> + db_page->refcount_t = 0;
>>>> + db_page->umem = ib_umem_get(&context->ibucontext, virt & PAGE_MASK,
>>>> + PAGE_SIZE, 0, 0);
>>>
>>> It seems quite odd to call something 'doorbell' that exists in system
>>> memory? doorbell should exist in BAR memory..
>>>
>>
>> I will rename db_page to page to avoid confusion in v5.
>
> Don't forget to do the user space side too.
>
> Seems like you should have some kind of name for this, but I don't
> really know what it is for..
>
I have already changed db_page to page in v5 which has been send out yesterday.
I name it db_page previously for implying that the page is related record doorbell.
As you mentioned, doorbell is in BAR memory while record doorbell is in system memory.
How about change all the dbs to recdb, where actually they meant record db,
such as db_page->recdb_page, db->recdb, etc.
>>>> + if (IS_ERR(db_page->umem)) {
>>>> + ret = PTR_ERR(db_page->umem);
>>>> + kfree(db_page);
>>>> + goto out;
>>>> + }
>>>> +
>>>> + list_add(&db_page->list, &context->db_page_list);
>>>> +
>>>> +found:
>>>> + db->dma = sg_dma_address(db_page->umem->sg_head.sgl) +
>>>> + (virt & ~PAGE_MASK);
>>>> + db->u.user_page = db_page;
>>>> + ++db_page->refcount_t;
>>>
>>> Since user space controls this it should be a refcount_t
>>
>> Do you mean I should pass the refcount_t from user space to kernel and
>> here use the user space refcount_t?
>
> No.. I mean:
> ++db_page->refcount_t;
>
> Should be:
> refcount_inc(&db_page->refcount);
>
> To prevent userspace from overflowing the refcount and causing
> something bad from happening.
>
Okay, I see, thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
