From: Jason Gunthorpe <jgg@mellanox.com>
To: Leon Romanovsky <leon@kernel.org>
Cc: Doug Ledford <dledford@redhat.com>,
linux-rdma@vger.kernel.org,
Michael Guralnik <michaelgur@mellanox.com>,
netdev@vger.kernel.org, Saeed Mahameed <saeedm@mellanox.com>,
Yishai Hadas <yishaih@mellanox.com>
Subject: Re: [PATCH rdma-next 0/4] Introduce dynamic UAR allocation mode
Date: Wed, 18 Mar 2020 11:12:08 -0300 [thread overview]
Message-ID: <20200318141208.GM13183@mellanox.com> (raw)
In-Reply-To: <20200318140932.GB126814@unreal>
On Wed, Mar 18, 2020 at 04:09:32PM +0200, Leon Romanovsky wrote:
> On Wed, Mar 18, 2020 at 11:00:01AM -0300, Jason Gunthorpe wrote:
> > On Wed, Mar 18, 2020 at 03:56:31PM +0200, Leon Romanovsky wrote:
> > > 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?
> >
> > Yes, when a rdma-core release obsoletes the code path then it can
> > become annotated.
> >
> > > 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
> >
> > Well, we'd avoid making such complications, it would be something like
> >
> > if (flag & foo) {
> > if (CONFIG_INFINIBAND_MIN_RDMA_CORE_VERSION >= 21)
> > return -ENOPROTOOPT;
> > [keep going as before]
> > }
> >
> > At least we now know this conditional path isn't used / isn't covered
> > by testing
>
> I'm ok with this approach because it helps us to find those dead
> paths, but have last question, shouldn't this be achieved with
> proper documentation of every flag instead of adding CONFIG_..?
How do you mean?
The other half of this idea is to disable obsolete un tested code to
avoid potential bugs. Which requires CONFIG_?
Jason
next prev parent reply other threads:[~2020-03-18 14:12 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-18 12:43 [PATCH rdma-next 0/4] Introduce dynamic UAR allocation mode Leon Romanovsky
2020-03-18 12:43 ` [PATCH mlx5-next 4/4] IB/mlx5: Move to fully dynamic UAR mode once user space supports it Leon Romanovsky
2020-03-18 23:38 ` Saeed Mahameed
2020-03-19 5:55 ` Leon Romanovsky
2020-03-18 12:54 ` [PATCH rdma-next 0/4] Introduce dynamic UAR allocation mode Jason Gunthorpe
2020-03-18 13:14 ` Leon Romanovsky
2020-03-18 13:21 ` Jason Gunthorpe
2020-03-18 13:56 ` Leon Romanovsky
2020-03-18 14:00 ` Jason Gunthorpe
2020-03-18 14:09 ` Leon Romanovsky
2020-03-18 14:12 ` Jason Gunthorpe [this message]
2020-03-18 14:24 ` Leon Romanovsky
2020-03-18 14:39 ` Jason Gunthorpe
2020-03-18 17:07 ` Leon Romanovsky
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200318141208.GM13183@mellanox.com \
--to=jgg@mellanox.com \
--cc=dledford@redhat.com \
--cc=leon@kernel.org \
--cc=linux-rdma@vger.kernel.org \
--cc=michaelgur@mellanox.com \
--cc=netdev@vger.kernel.org \
--cc=saeedm@mellanox.com \
--cc=yishaih@mellanox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).