Re: [PATCH v2 for-next] RDMA/hns: Add a new mmap implementation

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, Oct 22, 2021 at 05:46:19PM +0800, Wenpeng Liang wrote:
> > Just write
> > 
> >  if (entry->mmap_type != HNS_ROCE_MMAP_TYPE_TPTR)
> >     prot = pgprot_noncached(prot);
> >  ret = rdma_user_mmap_io(uctx, vma, pfn,
> > 			rdma_entry->npages * PAGE_SIZE,
> > 			pgprot_noncached(prot), rdma_entry);
> > 
> > No need for the big case statement
> > 
> 
> In the future, we will have multiple new features that will add
> new branches to this switch. We hope to reduce the coupling
> between subsequent patches, so we hope to keep this switch.

Why? The only choice that should be made at mmap time is if this is
noncached or not - the address and everything else should be setup
during the creation of the entry.

> >>  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;
> > 
> > I'm confused, this doesn't change the uAPI, so why add this stuff?
> > This should go in a later patch?
> > 
> 
> The related userspace has also been modified, the link is:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.spinics.net%2Flists%2Flinux-rdma%2Fmsg106056.html&data=04%7C01%7Cjgg%40nvidia.com%7Cffac1b9f0afb458a4da308d99540ce99%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637704927843133708%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=BAszHlXQWWh%2F8hrb%2F8qEStm3VBrhvbMshfPN3pc6wKI%3D&reserved=0
> 
> I don’t know if I understood your question correctly. These fields
> are used for compatibility, so they are necessary. The user space
> and kernel space of different versions are handshake through 'config'.

That is for later patches, this patch doesn't seem to change the uAPI
at all. The compat mmaps remain at the same offsets they were always
at, there is nothing to negotiate at this point.

Move it to a later series?

Jason