Re: [RFC rdma 1/3] RDMA/core: Create a common mmap function

Re: [RFC rdma 1/3] RDMA/core: Create a common mmap function

[Jason Gunthorpe]

On Wed, Jul 03, 2019 at 11:19:34AM +0300, Gal Pressman wrote:
> On 03/07/2019 1:31, Jason Gunthorpe wrote:
> >> Seems except Mellanox + hns the mmap flags aren't ABI. 
> >> Also, current Mellanox code seems like it won't benefit from 
> >> mmap cookie helper functions in any case as the mmap function is very specific and the flags used indicate 
> >> the address and not just how to map it.
> > 
> > IMHO, mlx5 has a goofy implementaiton here as it codes all of the object
> > type, handle and cachability flags in one thing.
> 
> Do we need object type flags as well in the generic mmap code?

At the end of the day the driver needs to know what page to map during
the mmap syscall.

mlx5 does this by encoding the page type in the address, and then many
types have seperate lookups based onthe offset for the actual page.

IMHO the single lookup and opaque offset is generally better..

Since the mlx5 scheme is ABI it can't be changed unfortunately.

If you want to do user controlled cachability flags, or not, is a fair
question, but they still become ABI..

I'm wondering if it really makes sense to do that during the mmap, or
if the cachability should be set as part of creating the cookie?

> Another issue is that these flags aren't exposed in an ABI file, so
> a userspace library can't really make use of it in current state.

Woops.

Ah, this is all ABI so you need to dig out of this hole ASAP :)

Jason

RE: [EXT] Re: [RFC rdma 1/3] RDMA/core: Create a common mmap function

[Michal Kalderon]

> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Thursday, July 4, 2019 3:35 PM
> 
> External Email
> 
> ----------------------------------------------------------------------
> On Wed, Jul 03, 2019 at 11:19:34AM +0300, Gal Pressman wrote:
> > On 03/07/2019 1:31, Jason Gunthorpe wrote:
> > >> Seems except Mellanox + hns the mmap flags aren't ABI.
> > >> Also, current Mellanox code seems like it won't benefit from mmap
> > >> cookie helper functions in any case as the mmap function is very
> > >> specific and the flags used indicate the address and not just how to map
> it.
> > >
> > > IMHO, mlx5 has a goofy implementaiton here as it codes all of the
> > > object type, handle and cachability flags in one thing.
> >
> > Do we need object type flags as well in the generic mmap code?
> 
> At the end of the day the driver needs to know what page to map during the
> mmap syscall.
> 
> mlx5 does this by encoding the page type in the address, and then many
> types have seperate lookups based onthe offset for the actual page.
> 
> IMHO the single lookup and opaque offset is generally better..
> 
> Since the mlx5 scheme is ABI it can't be changed unfortunately.
> 
> If you want to do user controlled cachability flags, or not, is a fair question,
> but they still become ABI..
> 
> I'm wondering if it really makes sense to do that during the mmap, or if the
> cachability should be set as part of creating the cookie?
> 
> > Another issue is that these flags aren't exposed in an ABI file, so a
> > userspace library can't really make use of it in current state.
> 
> Woops.
> 
> Ah, this is all ABI so you need to dig out of this hole ASAP :)
>
Jason, I didn't follow - what is all ABI? 
currently EFA implementation encodes the cachability inside the key,
It's not exposed in ABI file and is opaque to user-space. The kernel decides on the cachability
And get's it back in the key when mmap is called. It seems good enough for the current cases.  
Thanks , 
Michal
 
> Jason

Re: [EXT] Re: [RFC rdma 1/3] RDMA/core: Create a common mmap function

[Jason Gunthorpe]

On Fri, Jul 05, 2019 at 03:29:03PM +0000, Michal Kalderon wrote:
> > From: Jason Gunthorpe <jgg@ziepe.ca>
> > Sent: Thursday, July 4, 2019 3:35 PM
> > 
> > External Email
> > 
> > On Wed, Jul 03, 2019 at 11:19:34AM +0300, Gal Pressman wrote:
> > > On 03/07/2019 1:31, Jason Gunthorpe wrote:
> > > >> Seems except Mellanox + hns the mmap flags aren't ABI.
> > > >> Also, current Mellanox code seems like it won't benefit from mmap
> > > >> cookie helper functions in any case as the mmap function is very
> > > >> specific and the flags used indicate the address and not just how to map
> > it.
> > > >
> > > > IMHO, mlx5 has a goofy implementaiton here as it codes all of the
> > > > object type, handle and cachability flags in one thing.
> > >
> > > Do we need object type flags as well in the generic mmap code?
> > 
> > At the end of the day the driver needs to know what page to map during the
> > mmap syscall.
> > 
> > mlx5 does this by encoding the page type in the address, and then many
> > types have seperate lookups based onthe offset for the actual page.
> > 
> > IMHO the single lookup and opaque offset is generally better..
> > 
> > Since the mlx5 scheme is ABI it can't be changed unfortunately.
> > 
> > If you want to do user controlled cachability flags, or not, is a fair question,
> > but they still become ABI..
> > 
> > I'm wondering if it really makes sense to do that during the mmap, or if the
> > cachability should be set as part of creating the cookie?
> > 
> > > Another issue is that these flags aren't exposed in an ABI file, so a
> > > userspace library can't really make use of it in current state.
> > 
> > Woops.
> > 
> > Ah, this is all ABI so you need to dig out of this hole ASAP :)
> >
> Jason, I didn't follow - what is all ABI? 
> currently EFA implementation encodes the cachability inside the key,
> It's not exposed in ABI file and is opaque to user-space. The kernel decides on the cachability
> And get's it back in the key when mmap is called. It seems good
> enough for the current cases.

Then the key 'offset' should not include cachability information at
all.

Jason

RE: [EXT] Re: [RFC rdma 1/3] RDMA/core: Create a common mmap function

[Michal Kalderon]

> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Friday, July 5, 2019 6:33 PM
> 
> On Fri, Jul 05, 2019 at 03:29:03PM +0000, Michal Kalderon wrote:
> > > From: Jason Gunthorpe <jgg@ziepe.ca>
> > > Sent: Thursday, July 4, 2019 3:35 PM
> > >
> > > External Email
> > >
> > > On Wed, Jul 03, 2019 at 11:19:34AM +0300, Gal Pressman wrote:
> > > > On 03/07/2019 1:31, Jason Gunthorpe wrote:
> > > > >> Seems except Mellanox + hns the mmap flags aren't ABI.
> > > > >> Also, current Mellanox code seems like it won't benefit from
> > > > >> mmap cookie helper functions in any case as the mmap function
> > > > >> is very specific and the flags used indicate the address and
> > > > >> not just how to map
> > > it.
> > > > >
> > > > > IMHO, mlx5 has a goofy implementaiton here as it codes all of
> > > > > the object type, handle and cachability flags in one thing.
> > > >
> > > > Do we need object type flags as well in the generic mmap code?
> > >
> > > At the end of the day the driver needs to know what page to map
> > > during the mmap syscall.
> > >
> > > mlx5 does this by encoding the page type in the address, and then
> > > many types have seperate lookups based onthe offset for the actual
> page.
> > >
> > > IMHO the single lookup and opaque offset is generally better..
> > >
> > > Since the mlx5 scheme is ABI it can't be changed unfortunately.
> > >
> > > If you want to do user controlled cachability flags, or not, is a
> > > fair question, but they still become ABI..
> > >
> > > I'm wondering if it really makes sense to do that during the mmap,
> > > or if the cachability should be set as part of creating the cookie?
> > >
> > > > Another issue is that these flags aren't exposed in an ABI file,
> > > > so a userspace library can't really make use of it in current state.
> > >
> > > Woops.
> > >
> > > Ah, this is all ABI so you need to dig out of this hole ASAP :)
> > >
> > Jason, I didn't follow - what is all ABI?
> > currently EFA implementation encodes the cachability inside the key,
> > It's not exposed in ABI file and is opaque to user-space. The kernel
> > decides on the cachability And get's it back in the key when mmap is
> > called. It seems good enough for the current cases.
> 
> Then the key 'offset' should not include cachability information at all.
> 
Fair enough, so as you stated above the cachabiliy can be set in the cookie. 
Would we still like to leave some bits for future ABI enhancements, requests, from user ? 
Similar to a page type that mlx has ? 

Thanks,
Michal


> Jason

Re: [EXT] Re: [RFC rdma 1/3] RDMA/core: Create a common mmap function

[Jason Gunthorpe]

On Fri, Jul 05, 2019 at 05:24:18PM +0000, Michal Kalderon wrote:
> > From: Jason Gunthorpe <jgg@ziepe.ca>
> > Sent: Friday, July 5, 2019 6:33 PM
> > 
> > On Fri, Jul 05, 2019 at 03:29:03PM +0000, Michal Kalderon wrote:
> > > > From: Jason Gunthorpe <jgg@ziepe.ca>
> > > > Sent: Thursday, July 4, 2019 3:35 PM
> > > >
> > > > External Email
> > > >
> > > > On Wed, Jul 03, 2019 at 11:19:34AM +0300, Gal Pressman wrote:
> > > > > On 03/07/2019 1:31, Jason Gunthorpe wrote:
> > > > > >> Seems except Mellanox + hns the mmap flags aren't ABI.
> > > > > >> Also, current Mellanox code seems like it won't benefit from
> > > > > >> mmap cookie helper functions in any case as the mmap function
> > > > > >> is very specific and the flags used indicate the address and
> > > > > >> not just how to map
> > > > it.
> > > > > >
> > > > > > IMHO, mlx5 has a goofy implementaiton here as it codes all of
> > > > > > the object type, handle and cachability flags in one thing.
> > > > >
> > > > > Do we need object type flags as well in the generic mmap code?
> > > >
> > > > At the end of the day the driver needs to know what page to map
> > > > during the mmap syscall.
> > > >
> > > > mlx5 does this by encoding the page type in the address, and then
> > > > many types have seperate lookups based onthe offset for the actual
> > page.
> > > >
> > > > IMHO the single lookup and opaque offset is generally better..
> > > >
> > > > Since the mlx5 scheme is ABI it can't be changed unfortunately.
> > > >
> > > > If you want to do user controlled cachability flags, or not, is a
> > > > fair question, but they still become ABI..
> > > >
> > > > I'm wondering if it really makes sense to do that during the mmap,
> > > > or if the cachability should be set as part of creating the cookie?
> > > >
> > > > > Another issue is that these flags aren't exposed in an ABI file,
> > > > > so a userspace library can't really make use of it in current state.
> > > >
> > > > Woops.
> > > >
> > > > Ah, this is all ABI so you need to dig out of this hole ASAP :)
> > > >
> > > Jason, I didn't follow - what is all ABI?
> > > currently EFA implementation encodes the cachability inside the key,
> > > It's not exposed in ABI file and is opaque to user-space. The kernel
> > > decides on the cachability And get's it back in the key when mmap is
> > > called. It seems good enough for the current cases.
> > 
> > Then the key 'offset' should not include cachability information at all.
> > 
> Fair enough, so as you stated above the cachabiliy can be set in the cookie. 
> Would we still like to leave some bits for future ABI enhancements, requests, from user ? 
> Similar to a page type that mlx has ? 

Doesn't make sense to mix and match, the page_type was just some way
to avoid tracking cookies in some cases. If we are always having a
cookie then the cookie should indicate the type based on how it was
created. Totally opaque

Jason

Re: [EXT] Re: [RFC rdma 1/3] RDMA/core: Create a common mmap function

[Gal Pressman]

On 05/07/2019 20:35, Jason Gunthorpe wrote:
> On Fri, Jul 05, 2019 at 05:24:18PM +0000, Michal Kalderon wrote:
>>> From: Jason Gunthorpe <jgg@ziepe.ca>
>>> Sent: Friday, July 5, 2019 6:33 PM
>>>
>>> On Fri, Jul 05, 2019 at 03:29:03PM +0000, Michal Kalderon wrote:
>>>>> From: Jason Gunthorpe <jgg@ziepe.ca>
>>>>> Sent: Thursday, July 4, 2019 3:35 PM
>>>>>
>>>>> External Email
>>>>>
>>>>> On Wed, Jul 03, 2019 at 11:19:34AM +0300, Gal Pressman wrote:
>>>>>> On 03/07/2019 1:31, Jason Gunthorpe wrote:
>>>>>>>> Seems except Mellanox + hns the mmap flags aren't ABI.
>>>>>>>> Also, current Mellanox code seems like it won't benefit from
>>>>>>>> mmap cookie helper functions in any case as the mmap function
>>>>>>>> is very specific and the flags used indicate the address and
>>>>>>>> not just how to map
>>>>> it.
>>>>>>>
>>>>>>> IMHO, mlx5 has a goofy implementaiton here as it codes all of
>>>>>>> the object type, handle and cachability flags in one thing.
>>>>>>
>>>>>> Do we need object type flags as well in the generic mmap code?
>>>>>
>>>>> At the end of the day the driver needs to know what page to map
>>>>> during the mmap syscall.
>>>>>
>>>>> mlx5 does this by encoding the page type in the address, and then
>>>>> many types have seperate lookups based onthe offset for the actual
>>> page.
>>>>>
>>>>> IMHO the single lookup and opaque offset is generally better..
>>>>>
>>>>> Since the mlx5 scheme is ABI it can't be changed unfortunately.
>>>>>
>>>>> If you want to do user controlled cachability flags, or not, is a
>>>>> fair question, but they still become ABI..
>>>>>
>>>>> I'm wondering if it really makes sense to do that during the mmap,
>>>>> or if the cachability should be set as part of creating the cookie?
>>>>>
>>>>>> Another issue is that these flags aren't exposed in an ABI file,
>>>>>> so a userspace library can't really make use of it in current state.
>>>>>
>>>>> Woops.
>>>>>
>>>>> Ah, this is all ABI so you need to dig out of this hole ASAP :)
>>>>>
>>>> Jason, I didn't follow - what is all ABI?
>>>> currently EFA implementation encodes the cachability inside the key,
>>>> It's not exposed in ABI file and is opaque to user-space. The kernel
>>>> decides on the cachability And get's it back in the key when mmap is
>>>> called. It seems good enough for the current cases.
>>>
>>> Then the key 'offset' should not include cachability information at all.
>>>
>> Fair enough, so as you stated above the cachabiliy can be set in the cookie. 
>> Would we still like to leave some bits for future ABI enhancements, requests, from user ? 
>> Similar to a page type that mlx has ? 
> 
> Doesn't make sense to mix and match, the page_type was just some way
> to avoid tracking cookies in some cases. If we are always having a
> cookie then the cookie should indicate the type based on how it was
> created. Totally opaque

I'm fine with removing the cachability flags from the ABI, but I don't see how
the page types can be added without exposing them in the key.

If we want to mmap something that's not a QP/CQ/... how can we do that? I guess
only by returning some key in alloc_ucontext?

RE: [EXT] Re: [RFC rdma 1/3] RDMA/core: Create a common mmap function

[Michal Kalderon]

> From: Gal Pressman <galpress@amazon.com>
> Sent: Sunday, July 7, 2019 9:41 AM
> 
> On 05/07/2019 20:35, Jason Gunthorpe wrote:
> > On Fri, Jul 05, 2019 at 05:24:18PM +0000, Michal Kalderon wrote:
> >>> From: Jason Gunthorpe <jgg@ziepe.ca>
> >>> Sent: Friday, July 5, 2019 6:33 PM
> >>>
> >>> On Fri, Jul 05, 2019 at 03:29:03PM +0000, Michal Kalderon wrote:
> >>>>> From: Jason Gunthorpe <jgg@ziepe.ca>
> >>>>> Sent: Thursday, July 4, 2019 3:35 PM
> >>>>>
> >>>>> External Email
> >>>>>
> >>>>> On Wed, Jul 03, 2019 at 11:19:34AM +0300, Gal Pressman wrote:
> >>>>>> On 03/07/2019 1:31, Jason Gunthorpe wrote:
> >>>>>>>> Seems except Mellanox + hns the mmap flags aren't ABI.
> >>>>>>>> Also, current Mellanox code seems like it won't benefit from
> >>>>>>>> mmap cookie helper functions in any case as the mmap function
> >>>>>>>> is very specific and the flags used indicate the address and
> >>>>>>>> not just how to map
> >>>>> it.
> >>>>>>>
> >>>>>>> IMHO, mlx5 has a goofy implementaiton here as it codes all of
> >>>>>>> the object type, handle and cachability flags in one thing.
> >>>>>>
> >>>>>> Do we need object type flags as well in the generic mmap code?
> >>>>>
> >>>>> At the end of the day the driver needs to know what page to map
> >>>>> during the mmap syscall.
> >>>>>
> >>>>> mlx5 does this by encoding the page type in the address, and then
> >>>>> many types have seperate lookups based onthe offset for the actual
> >>> page.
> >>>>>
> >>>>> IMHO the single lookup and opaque offset is generally better..
> >>>>>
> >>>>> Since the mlx5 scheme is ABI it can't be changed unfortunately.
> >>>>>
> >>>>> If you want to do user controlled cachability flags, or not, is a
> >>>>> fair question, but they still become ABI..
> >>>>>
> >>>>> I'm wondering if it really makes sense to do that during the mmap,
> >>>>> or if the cachability should be set as part of creating the cookie?
> >>>>>
> >>>>>> Another issue is that these flags aren't exposed in an ABI file,
> >>>>>> so a userspace library can't really make use of it in current state.
> >>>>>
> >>>>> Woops.
> >>>>>
> >>>>> Ah, this is all ABI so you need to dig out of this hole ASAP :)
> >>>>>
> >>>> Jason, I didn't follow - what is all ABI?
> >>>> currently EFA implementation encodes the cachability inside the
> >>>> key, It's not exposed in ABI file and is opaque to user-space. The
> >>>> kernel decides on the cachability And get's it back in the key when
> >>>> mmap is called. It seems good enough for the current cases.
> >>>
> >>> Then the key 'offset' should not include cachability information at all.
> >>>
> >> Fair enough, so as you stated above the cachabiliy can be set in the
> cookie.
> >> Would we still like to leave some bits for future ABI enhancements,
> requests, from user ?
> >> Similar to a page type that mlx has ?
> >
> > Doesn't make sense to mix and match, the page_type was just some way
> > to avoid tracking cookies in some cases. If we are always having a
> > cookie then the cookie should indicate the type based on how it was
> > created. Totally opaque
> 
> I'm fine with removing the cachability flags from the ABI, but I don't see how
> the page types can be added without exposing them in the key.
> 
> If we want to mmap something that's not a QP/CQ/... how can we do that? I
> guess only by returning some key in alloc_ucontext?

Right. Every call to mmap should be backed up by a cookie in the driver.