Received-SPF: pass (google.com: domain of linux-kernel+bounces-171832-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Tue, 7 May 2024 13:32:48 -0300
From: Jason Gunthorpe <jgg@ziepe.ca>
To: Chengchang Tang <tangchengchang@huawei.com>
Cc: Junxian Huang <huangjunxian6@hisilicon.com>, leon@kernel.org,
	linux-rdma@vger.kernel.org, linuxarm@huawei.com,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH for-next] RDMA/hns: Support flexible WQE buffer page size
Message-ID: <20240507163248.GD4718@ziepe.ca>
References: <20240430092845.4058786-1-huangjunxian6@hisilicon.com>
 <20240430134113.GU231144@ziepe.ca>
 <fac4927b-16ed-d801-fb47-182f2aca355c@huawei.com>
 <20240506151112.GE901876@ziepe.ca>
 <90d51f0f-724f-fbba-9519-1c022c65c5e2@huawei.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <90d51f0f-724f-fbba-9519-1c022c65c5e2@huawei.com>

On Tue, May 07, 2024 at 10:21:09PM +0800, Chengchang Tang wrote:
> 
> 
> On 2024/5/6 23:11, Jason Gunthorpe wrote:
> > On Mon, May 06, 2024 at 02:47:01PM +0800, Chengchang Tang wrote:
> >>
> >>
> >> On 2024/4/30 21:41, Jason Gunthorpe wrote:
> >>> On Tue, Apr 30, 2024 at 05:28:45PM +0800, Junxian Huang wrote:
> >>>> From: Chengchang Tang <tangchengchang@huawei.com>
> >>>>
> >>>> Currently, driver fixedly allocates 4K pages for userspace WQE buffer
> >>>> and results in HW reading WQE with a granularity of 4K even in a 64K
> >>>> system. HW has to switch pages every 4K, leading to a loss of performance.
> >>>
> >>>> In order to improve performance, add support for userspace to allocate
> >>>> flexible WQE buffer page size between 4K to system PAGESIZE.
> >>>> @@ -90,7 +90,8 @@ struct hns_roce_ib_create_qp {
> >>>>  	__u8    log_sq_bb_count;
> >>>>  	__u8    log_sq_stride;
> >>>>  	__u8    sq_no_prefetch;
> >>>> -	__u8    reserved[5];
> >>>> +	__u8    pageshift;
> >>>> +	__u8    reserved[4];
> >>>
> >>> It doesn't make any sense to pass in a pageshift from userspace.
> >>>
> >>> Kernel should detect whatever underlying physical contiguity userspace
> >>> has been able to create and configure the hardware optimally. The umem
> >>> already has all the tools to do this trivially.
> >>>
> >>> Why would you need to specify anything?
> >>>
> >>
> >> For hns roce, QPs requires three wqe buffers, namely SQ wqe buffer, RQ wqe
> >> buffer and EXT_SGE buffer.  Due to HW constraints, they need to be configured
> >> with the same page size. The memory of these three buffers is allocated by
> >> the user-mode driver now. The user-mode driver will calculate the size of
> >> each region and align them to the page size. Finally, the driver will merge
> >> the memories of these three regions together, apply for a memory with
> >> continuous virtual addresses, and send the address to the kernel-mode driver
> >> (during this process, the user-mode driver and the kernel-mode driver only
> >> exchange addresses, but not the the sizes of these three areas or other
> >> information).
> > 
> > So you get a umem and the driver is slicing it up. What is the
> > problem? The kernel has the umem and the kernel knows the uniform page
> > size of that umem.
> 
> Currently, because the user-mode driver and the kernel-mode driver only
> exchange addresses, from the perspective of the kernel-mode driver, if the
> page size is not negotiated, it cannot even calculate the size of each region,
> and thus cannot complete ib_umem_get().

That seems like what you should correct instead of sending in a page
shift.

> Of course, we can add some information to be passed to the kernel mode driver,
> such as size and offset of each region, but is there any essential difference
> between this and directly passing page shift?

Yes, userspace can reliably compute the start, length and any
offsets. It cannot reliably compute any sort of page size for DMA.

> >> Since the three regions share one umem, through umem's tools, such as
> >> ib_umem_find_best_pgsz(), they will eventually calculate the best page size
> >> of the entire umem, not each region. 
> > 
> > That is what you want, you said? Each region has to have the same page
> > size. So the global page size of the umem is the correct one?
> 
> No, the global page size may be bigger than the page size of each region.
> If we use the global page size, the hardware may have out-of-bounds
> access.

Because the page size may be the entire umem?

But this is simple to deal with, the API even has it built in. Just be
sure to limit the page size bit map to only include sizes you can
handle based on the smallest sub region and possibly region
alignment. It should be a couple of bit ops.

> > Even if it is difficult, this has to be done like this. You can't pass
> > a page size in from userspace, there is no good way for userspace to
> > do this correctly in all cases.
> 
> Userspace may indeed go wrong, but in the current scenario, the page size is
> only set within the allowed range [4k, 64k], and its errors only affects the
> current QP. Is this acceptable?

It is bad uAPI design to allow for such a failure mode.

Jason