Re: [PATCH rdma-next 0/4] Introduce dynamic UAR allocation mode

[Leon Romanovsky <leon@kernel.org>]

On Wed, Mar 18, 2020 at 10:21:00AM -0300, Jason Gunthorpe wrote:
> On Wed, Mar 18, 2020 at 03:14:50PM +0200, Leon Romanovsky wrote:
> > On Wed, Mar 18, 2020 at 09:54:59AM -0300, Jason Gunthorpe wrote:
> > > On Wed, Mar 18, 2020 at 02:43:25PM +0200, Leon Romanovsky wrote:
> > > > From: Leon Romanovsky <leonro@mellanox.com>
> > > >
> > > > From Yishai,
> > > >
> > > > This series exposes API to enable a dynamic allocation and management of a
> > > > UAR which now becomes to be a regular uobject.
> > > >
> > > > Moving to that mode enables allocating a UAR only upon demand and drop the
> > > > redundant static allocation of UARs upon context creation.
> > > >
> > > > In addition, it allows master and secondary processes that own the same command
> > > > FD to allocate and manage UARs according to their needs, this can’t be achieved
> > > > today.
> > > >
> > > > As part of this option, QP & CQ creation flows were adapted to support this
> > > > dynamic UAR mode once asked by user space.
> > > >
> > > > Once this mode is asked by mlx5 user space driver on a given context, it will
> > > > be mutual exclusive, means both the static and legacy dynamic modes for using
> > > > UARs will be blocked.
> > > >
> > > > The legacy modes are supported for backward compatible reasons, looking
> > > > forward we expect this new mode to be the default.
> > >
> > > We are starting to accumulate a lot of code that is now old-rdma-core
> > > only.
> >
> > Agree
> >
> > >
> > > I have been wondering if we should add something like
> > >
> > > #if CONFIG_INFINIBAND_MIN_RDMA_CORE_VERSION < 21
> > > #endif
> >
> > From one side it will definitely help to see old code, but from another
> > it will create many ifdef inside of the code with a very little chance
> > of testing. Also we will continue to have the same problem to decide when
> > we can delete this code.
>
> Well, it doesn't have to be an #ifdef, eg just sticking
>
> if (CONFIG_INFINIBAND_MIN_RDMA_CORE_VERSION >= 21)
>      return -ENOPROTOOPT;
>
> at the top of obsolete functions would go a long way

First, how will you set this min_version? hordcoded in the kernel code?
Second, it will work for simple flows, but can be extremely complex
if your code looks like:
if (old_version)
 do something
if (new version)
 do something else

You will need to add logic to handle this -ENOPROTOOPT error value.

>
> > > So we can keep track of what is actually a used code flow and what is
> > > now hard to test legacy code.
> > >
> > > eg this config would also disable the write interface(), turn off
> > > compat write interfaces as they are switched to use ioctl, etc, etc.
> >
> > What about if we introduce one ifdef, let's say CONFIG_INFINIBAND_LEGACY
> > and put everything that will be declared as legacy to that bucket? And
> > once every 5 (???) years delete everything from that bucket.
>
> It is much harder to see what is really old vs only a little old
>
> I'm not sure we can ever completely delete any of this, but at least
> the distros can make an informed choice to either do more detailed
> test of old libraries or disable those code paths.

It will be nice to hear how distros decide to disable/drop the code.

Thanks

>
> Jason